[Qemu-devel] [PATCH 0/2] screendump fixups

[Qemu-devel] [PATCH 0/2] screendump fixups

[Gerd Hoffmann]

  Hi,

After checking again I figured the displaychangelistener for screendumps
isn't needed.  Sorry alon.  So lets just zap it.  Also the console
switching can be optimized further, when no switching was needed (the
common case) we can just go ahead and write out the dump without a full
displaysurface redraw.

cheers,
  Gerd

Gerd Hoffmann (2):
  vga: simplify screendump
  optimize screendump for the common non-switch case

 console.c       |   10 +++++++---
 console.h       |    2 +-
 hw/qxl.c        |    4 ++--
 hw/vga.c        |   46 +++++++---------------------------------------
 hw/vmware_vga.c |    4 ++--
 5 files changed, 19 insertions(+), 47 deletions(-)

[Qemu-devel] [PATCH 1/2] vga: simplify screendump

[Gerd Hoffmann]

The displaychangelistener isn't needed at all, we can simply save the
image when vga_hw_update is done instead of hooking into the update
process.
---
 hw/vga.c |   36 +-----------------------------------
 1 files changed, 1 insertions(+), 35 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index c1029db..f8f30f8 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -163,8 +163,6 @@ static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
 static void vga_screen_dump(void *opaque, const char *filename);
-static const char *screen_dump_filename;
-static DisplayChangeListener *screen_dump_dcl;
 
 static void vga_update_memory_access(VGACommonState *s)
 {
@@ -2364,22 +2362,6 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
 /********************************************************/
 /* vga screen dump */
 
-static void vga_save_dpy_update(DisplayState *ds,
-                                int x, int y, int w, int h)
-{
-    if (screen_dump_filename) {
-        ppm_save(screen_dump_filename, ds->surface);
-    }
-}
-
-static void vga_save_dpy_resize(DisplayState *s)
-{
-}
-
-static void vga_save_dpy_refresh(DisplayState *s)
-{
-}
-
 int ppm_save(const char *filename, struct DisplaySurface *ds)
 {
     FILE *f;
@@ -2423,29 +2405,13 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
     return 0;
 }
 
-static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
-{
-    DisplayChangeListener *dcl;
-
-    dcl = g_malloc0(sizeof(DisplayChangeListener));
-    dcl->dpy_update = vga_save_dpy_update;
-    dcl->dpy_resize = vga_save_dpy_resize;
-    dcl->dpy_refresh = vga_save_dpy_refresh;
-    register_displaychangelistener(ds, dcl);
-    return dcl;
-}
-
 /* save the vga display in a PPM image even if no display is
    available */
 static void vga_screen_dump(void *opaque, const char *filename)
 {
     VGACommonState *s = opaque;
 
-    if (!screen_dump_dcl)
-        screen_dump_dcl = vga_screen_dump_init(s->ds);
-
-    screen_dump_filename = filename;
     vga_invalidate_display(s);
     vga_hw_update();
-    screen_dump_filename = NULL;
+    ppm_save(filename, s->ds->surface);
 }
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] optimize screendump for the common non-switch case

[Gerd Hoffmann]

switch console only if needed, also pass down whenever the console was
switched or not because a displaysurface redraw is only needed in case
the console was switched.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 console.c       |   10 +++++++---
 console.h       |    2 +-
 hw/qxl.c        |    4 ++--
 hw/vga.c        |   10 ++++++----
 hw/vmware_vga.c |    4 ++--
 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/console.c b/console.c
index 135394f..b3bd14a 100644
--- a/console.c
+++ b/console.c
@@ -176,17 +176,21 @@ void vga_hw_invalidate(void)
 void vga_hw_screen_dump(const char *filename)
 {
     TextConsole *previous_active_console;
+    bool cswitch;
 
     previous_active_console = active_console;
+    cswitch = previous_active_console && previous_active_console->index != 0;
 
     /* There is currently no way of specifying which screen we want to dump,
        so always dump the first one.  */
-    console_select(0);
+    if (cswitch) {
+        console_select(0);
+    }
     if (consoles[0] && consoles[0]->hw_screen_dump) {
-        consoles[0]->hw_screen_dump(consoles[0]->hw, filename);
+        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
     }
 
-    if (previous_active_console) {
+    if (cswitch) {
         console_select(previous_active_console->index);
     }
 }
diff --git a/console.h b/console.h
index 6ba0d5d..1fc8ea3 100644
--- a/console.h
+++ b/console.h
@@ -340,7 +340,7 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
 
 typedef void (*vga_hw_update_ptr)(void *);
 typedef void (*vga_hw_invalidate_ptr)(void *);
-typedef void (*vga_hw_screen_dump_ptr)(void *, const char *);
+typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
 typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
 
 DisplayState *graphic_console_init(vga_hw_update_ptr update,
diff --git a/hw/qxl.c b/hw/qxl.c
index e38bb29..f4c68ab 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1438,7 +1438,7 @@ static void qxl_hw_invalidate(void *opaque)
     vga->invalidate(vga);
 }
 
-static void qxl_hw_screen_dump(void *opaque, const char *filename)
+static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
 {
     PCIQXLDevice *qxl = opaque;
     VGACommonState *vga = &qxl->vga;
@@ -1450,7 +1450,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename)
         ppm_save(filename, qxl->ssd.ds->surface);
         break;
     case QXL_MODE_VGA:
-        vga->screen_dump(vga, filename);
+        vga->screen_dump(vga, filename, cswitch);
         break;
     default:
         break;
diff --git a/hw/vga.c b/hw/vga.c
index f8f30f8..5994f43 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -162,7 +162,7 @@ static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
-static void vga_screen_dump(void *opaque, const char *filename);
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
 
 static void vga_update_memory_access(VGACommonState *s)
 {
@@ -2407,11 +2407,13 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
 
 /* save the vga display in a PPM image even if no display is
    available */
-static void vga_screen_dump(void *opaque, const char *filename)
+static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
 {
     VGACommonState *s = opaque;
 
-    vga_invalidate_display(s);
-    vga_hw_update();
+    if (cswitch) {
+        vga_invalidate_display(s);
+        vga_hw_update();
+    }
     ppm_save(filename, s->ds->surface);
 }
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index f8afa3c..142d9f4 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1003,11 +1003,11 @@ static void vmsvga_invalidate_display(void *opaque)
 
 /* save the vga display in a PPM image even if no display is
    available */
-static void vmsvga_screen_dump(void *opaque, const char *filename)
+static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
 {
     struct vmsvga_state_s *s = opaque;
     if (!s->enable) {
-        s->vga.screen_dump(&s->vga, filename);
+        s->vga.screen_dump(&s->vga, filename, cswitch);
         return;
     }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] vga: simplify screendump

[Alon Levy]

On Thu, Feb 23, 2012 at 10:22:37AM +0100, Gerd Hoffmann wrote:
> The displaychangelistener isn't needed at all, we can simply save the
> image when vga_hw_update is done instead of hooking into the update
> process.

ACK.

> ---
>  hw/vga.c |   36 +-----------------------------------
>  1 files changed, 1 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index c1029db..f8f30f8 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -163,8 +163,6 @@ static uint16_t expand2[256];
>  static uint8_t expand4to8[16];
>  
>  static void vga_screen_dump(void *opaque, const char *filename);
> -static const char *screen_dump_filename;
> -static DisplayChangeListener *screen_dump_dcl;
>  
>  static void vga_update_memory_access(VGACommonState *s)
>  {
> @@ -2364,22 +2362,6 @@ void vga_init_vbe(VGACommonState *s, MemoryRegion *system_memory)
>  /********************************************************/
>  /* vga screen dump */
>  
> -static void vga_save_dpy_update(DisplayState *ds,
> -                                int x, int y, int w, int h)
> -{
> -    if (screen_dump_filename) {
> -        ppm_save(screen_dump_filename, ds->surface);
> -    }
> -}
> -
> -static void vga_save_dpy_resize(DisplayState *s)
> -{
> -}
> -
> -static void vga_save_dpy_refresh(DisplayState *s)
> -{
> -}
> -
>  int ppm_save(const char *filename, struct DisplaySurface *ds)
>  {
>      FILE *f;
> @@ -2423,29 +2405,13 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>      return 0;
>  }
>  
> -static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
> -{
> -    DisplayChangeListener *dcl;
> -
> -    dcl = g_malloc0(sizeof(DisplayChangeListener));
> -    dcl->dpy_update = vga_save_dpy_update;
> -    dcl->dpy_resize = vga_save_dpy_resize;
> -    dcl->dpy_refresh = vga_save_dpy_refresh;
> -    register_displaychangelistener(ds, dcl);
> -    return dcl;
> -}
> -
>  /* save the vga display in a PPM image even if no display is
>     available */
>  static void vga_screen_dump(void *opaque, const char *filename)
>  {
>      VGACommonState *s = opaque;
>  
> -    if (!screen_dump_dcl)
> -        screen_dump_dcl = vga_screen_dump_init(s->ds);
> -
> -    screen_dump_filename = filename;
>      vga_invalidate_display(s);
>      vga_hw_update();
> -    screen_dump_filename = NULL;
> +    ppm_save(filename, s->ds->surface);
>  }
> -- 
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH 2/2] optimize screendump for the common non-switch case

[Alon Levy]

On Thu, Feb 23, 2012 at 10:22:38AM +0100, Gerd Hoffmann wrote:
> switch console only if needed, also pass down whenever the console was
> switched or not because a displaysurface redraw is only needed in case
> the console was switched.
> 

You've missed some implementations of the vga_hw_screen_dump_ptr, see

$ git grep -A3 graphic_console_init | grep screen | grep -v -w screen_dump
hw/blizzard.c-                                 blizzard_screen_dump, NULL, s);
hw/g364fb.c-                                 g364fb_screen_dump, NULL, s);
hw/jazz_led.c-                                 jazz_led_screen_dump,
hw/omap_dss.c-                                    omap_invalidate_display, omap_screen_dump, s);
hw/omap_lcdc.c-                                    omap_screen_dump, NULL, s);
hw/pxa2xx_lcd.c-                                 pxa2xx_screen_dump, NULL, s);
hw/qxl.c-                                   qxl_hw_screen_dump, qxl_hw_text_update, qxl);
hw/tcx.c-                                     tcx24_screen_dump, NULL, s);
hw/tcx.c-                                     tcx_screen_dump, NULL, s);
hw/vmware_vga.c-                                     vmsvga_screen_dump,

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  console.c       |   10 +++++++---
>  console.h       |    2 +-
>  hw/qxl.c        |    4 ++--
>  hw/vga.c        |   10 ++++++----
>  hw/vmware_vga.c |    4 ++--
>  5 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/console.c b/console.c
> index 135394f..b3bd14a 100644
> --- a/console.c
> +++ b/console.c
> @@ -176,17 +176,21 @@ void vga_hw_invalidate(void)
>  void vga_hw_screen_dump(const char *filename)
>  {
>      TextConsole *previous_active_console;
> +    bool cswitch;
>  
>      previous_active_console = active_console;
> +    cswitch = previous_active_console && previous_active_console->index != 0;
>  
>      /* There is currently no way of specifying which screen we want to dump,
>         so always dump the first one.  */
> -    console_select(0);
> +    if (cswitch) {
> +        console_select(0);
> +    }
>      if (consoles[0] && consoles[0]->hw_screen_dump) {
> -        consoles[0]->hw_screen_dump(consoles[0]->hw, filename);
> +        consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch);
>      }
>  
> -    if (previous_active_console) {
> +    if (cswitch) {
>          console_select(previous_active_console->index);
>      }
>  }
> diff --git a/console.h b/console.h
> index 6ba0d5d..1fc8ea3 100644
> --- a/console.h
> +++ b/console.h
> @@ -340,7 +340,7 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch)
>  
>  typedef void (*vga_hw_update_ptr)(void *);
>  typedef void (*vga_hw_invalidate_ptr)(void *);
> -typedef void (*vga_hw_screen_dump_ptr)(void *, const char *);
> +typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
>  typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
>  
>  DisplayState *graphic_console_init(vga_hw_update_ptr update,
> diff --git a/hw/qxl.c b/hw/qxl.c
> index e38bb29..f4c68ab 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1438,7 +1438,7 @@ static void qxl_hw_invalidate(void *opaque)
>      vga->invalidate(vga);
>  }
>  
> -static void qxl_hw_screen_dump(void *opaque, const char *filename)
> +static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch)
>  {
>      PCIQXLDevice *qxl = opaque;
>      VGACommonState *vga = &qxl->vga;
> @@ -1450,7 +1450,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename)
>          ppm_save(filename, qxl->ssd.ds->surface);
>          break;
>      case QXL_MODE_VGA:
> -        vga->screen_dump(vga, filename);
> +        vga->screen_dump(vga, filename, cswitch);
>          break;
>      default:
>          break;
> diff --git a/hw/vga.c b/hw/vga.c
> index f8f30f8..5994f43 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -162,7 +162,7 @@ static uint32_t expand4[256];
>  static uint16_t expand2[256];
>  static uint8_t expand4to8[16];
>  
> -static void vga_screen_dump(void *opaque, const char *filename);
> +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch);
>  
>  static void vga_update_memory_access(VGACommonState *s)
>  {
> @@ -2407,11 +2407,13 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>  
>  /* save the vga display in a PPM image even if no display is
>     available */
> -static void vga_screen_dump(void *opaque, const char *filename)
> +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
>  {
>      VGACommonState *s = opaque;
>  
> -    vga_invalidate_display(s);
> -    vga_hw_update();
> +    if (cswitch) {
> +        vga_invalidate_display(s);
> +        vga_hw_update();
> +    }
>      ppm_save(filename, s->ds->surface);
>  }
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index f8afa3c..142d9f4 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1003,11 +1003,11 @@ static void vmsvga_invalidate_display(void *opaque)
>  
>  /* save the vga display in a PPM image even if no display is
>     available */
> -static void vmsvga_screen_dump(void *opaque, const char *filename)
> +static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch)
>  {
>      struct vmsvga_state_s *s = opaque;
>      if (!s->enable) {
> -        s->vga.screen_dump(&s->vga, filename);
> +        s->vga.screen_dump(&s->vga, filename, cswitch);
>          return;
>      }
>  
> -- 
> 1.7.1
>