From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53011) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0BHH-0004Sr-77 for qemu-devel@nongnu.org; Wed, 22 Feb 2012 07:26:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S0BHB-0002Ud-BX for qemu-devel@nongnu.org; Wed, 22 Feb 2012 07:26:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:15618) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0BHB-0002UT-3P for qemu-devel@nongnu.org; Wed, 22 Feb 2012 07:26:29 -0500 Date: Wed, 22 Feb 2012 13:26:16 +0100 From: Alon Levy Message-ID: <20120222122616.GB607@garlic.redhat.com> References: <1329860377-6284-1-git-send-email-alevy@redhat.com> <1329860377-6284-5-git-send-email-alevy@redhat.com> <4F44CD10.2040103@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F44CD10.2040103@redhat.com> Subject: Re: [Qemu-devel] [RFC v4 4/9] qxl: screen_dump in vga: do a single ppm_save List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: elmarco@redhat.com, yhalperi@redhat.com, qemu-devel@nongnu.org, spice-devel@freedesktop.org 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