Re: [Qemu-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save

[Alon Levy]

On Wed, Feb 22, 2012 at 12:10:08PM +0100, Gerd Hoffmann wrote:
> On 02/21/12 22:39, Alon Levy wrote:
> > Using vga->screen_dump results in a number of calls to ppm_save,
> > instead of a single one.
> 
> Can you investigate why?

oh, I know why. vga_screen_dump implementation:

    screen_dump_filename = filename;
    vga_invalidate_display(s);
->  vga_hw_update();
    screen_dump_filename = NULL;

And the only user of screen_dump_filename is:

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);
    }
}

vga_save_dpy_update is called on any dpy_update, registered after the
first vga_screen_dump call.

Since there are a number of callers to dpy_update: vga_draw_text calls
it potentially once every line, vga_draw_graphic the same.
vga_draw_blank calls it once.

> 
> > Lacking time to test all the possible users of
> > vga->screen_dump, avoid the redundant calls by doing the vga_hw_update+
> > ppm_save in qxl_hw_screen_dump.
> 
> I'd prefer to fix the root cause instead of adding workarounds.
> 

This seems to work, only tested with -vga qxl and in vga mode:


diff --git a/hw/vga.c b/hw/vga.c
index c1029db..51f20c1 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;
@@ -2389,10 +2371,12 @@ int ppm_save(const char *filename, struct 
DisplaySurface *ds)
     uint8_t r, g, b;
     int ret;
     char *linebuf, *pbuf;
+    static int calls;
 
     f = fopen(filename, "wb");
     if (!f)
         return -1;
+    fprintf(stderr, "ppm_save %d\n", ++calls);
     fprintf(f, "P6\n%d %d\n%d\n",
             ds->width, ds->height, 255);
     linebuf = g_malloc(ds->width * 3);
@@ -2423,29 +2407,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);
 }

> cheers,
>   Gerd