[Qemu-devel] [PATCH v2] Optimize screendump

[Qemu-devel] [PATCH v2] Optimize screendump

[Avi Kivity]

When running kvm-autotest, fputc() is often the second highest (sometimes #1)
function showing up in a profile.  This is due to fputc() locking the file
for every byte written.

Optimize by buffering a line's worth of pixels and writing that out in a
single call.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

v2: drop unportable fputc_unlocked

 hw/vga.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index d5bc582..97c96bf 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2349,15 +2349,19 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
     uint32_t v;
     int y, x;
     uint8_t r, g, b;
+    int ret;
+    char *linebuf, *pbuf;
 
     f = fopen(filename, "wb");
     if (!f)
         return -1;
     fprintf(f, "P6\n%d %d\n%d\n",
             ds->width, ds->height, 255);
+    linebuf = qemu_malloc(ds->width * 3);
     d1 = ds->data;
     for(y = 0; y < ds->height; y++) {
         d = d1;
+        pbuf = linebuf;
         for(x = 0; x < ds->width; x++) {
             if (ds->pf.bits_per_pixel == 32)
                 v = *(uint32_t *)d;
@@ -2369,13 +2373,16 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
                 (ds->pf.gmax + 1);
             b = ((v >> ds->pf.bshift) & ds->pf.bmax) * 256 /
                 (ds->pf.bmax + 1);
-            fputc(r, f);
-            fputc(g, f);
-            fputc(b, f);
+            *pbuf++ = r;
+            *pbuf++ = g;
+            *pbuf++ = b;
             d += ds->pf.bytes_per_pixel;
         }
         d1 += ds->linesize;
+        ret = fwrite(linebuf, 1, pbuf - linebuf, f);
+        (void)ret;
     }
+    qemu_free(linebuf);
     fclose(f);
     return 0;
 }
-- 
1.7.5.3

Re: [Qemu-devel] [PATCH v2] Optimize screendump

[Jan Kiszka]

On 2011-06-20 10:12, Avi Kivity wrote:
> When running kvm-autotest, fputc() is often the second highest (sometimes #1)
> function showing up in a profile.  This is due to fputc() locking the file
> for every byte written.
> 
> Optimize by buffering a line's worth of pixels and writing that out in a
> single call.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> 
> v2: drop unportable fputc_unlocked
> 
>  hw/vga.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vga.c b/hw/vga.c
> index d5bc582..97c96bf 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2349,15 +2349,19 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>      uint32_t v;
>      int y, x;
>      uint8_t r, g, b;
> +    int ret;
> +    char *linebuf, *pbuf;
>  
>      f = fopen(filename, "wb");
>      if (!f)
>          return -1;
>      fprintf(f, "P6\n%d %d\n%d\n",
>              ds->width, ds->height, 255);
> +    linebuf = qemu_malloc(ds->width * 3);
>      d1 = ds->data;
>      for(y = 0; y < ds->height; y++) {
>          d = d1;
> +        pbuf = linebuf;
>          for(x = 0; x < ds->width; x++) {
>              if (ds->pf.bits_per_pixel == 32)
>                  v = *(uint32_t *)d;
> @@ -2369,13 +2373,16 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>                  (ds->pf.gmax + 1);
>              b = ((v >> ds->pf.bshift) & ds->pf.bmax) * 256 /
>                  (ds->pf.bmax + 1);
> -            fputc(r, f);
> -            fputc(g, f);
> -            fputc(b, f);
> +            *pbuf++ = r;
> +            *pbuf++ = g;
> +            *pbuf++ = b;
>              d += ds->pf.bytes_per_pixel;
>          }
>          d1 += ds->linesize;
> +        ret = fwrite(linebuf, 1, pbuf - linebuf, f);
> +        (void)ret;
>      }
> +    qemu_free(linebuf);
>      fclose(f);
>      return 0;
>  }

Unrelated to this patch, but why is this function located in vga.c and
not in console.c?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH v2] Optimize screendump

[Avi Kivity]

On 06/20/2011 03:33 PM, Jan Kiszka wrote:
> >  --- a/hw/vga.c
> >  +++ b/hw/vga.c
> >  @@ -2349,15 +2349,19 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)

> Unrelated to this patch, but why is this function located in vga.c and
> not in console.c?

It's located in omap_lcdc.c  as well.  But it needs to be fully 
generalized to be moved out (handle all PixelFormats).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2] Optimize screendump

[Stefan Hajnoczi]

On Mon, Jun 20, 2011 at 9:12 AM, Avi Kivity <avi@redhat.com> wrote:
> When running kvm-autotest, fputc() is often the second highest (sometimes #1)
> function showing up in a profile.  This is due to fputc() locking the file
> for every byte written.
>
> Optimize by buffering a line's worth of pixels and writing that out in a
> single call.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>
> v2: drop unportable fputc_unlocked
>
>  hw/vga.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v2] Optimize screendump

[Anthony Liguori]

On 06/20/2011 03:12 AM, Avi Kivity wrote:
> When running kvm-autotest, fputc() is often the second highest (sometimes #1)
> function showing up in a profile.  This is due to fputc() locking the file
> for every byte written.
>
> Optimize by buffering a line's worth of pixels and writing that out in a
> single call.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>
> v2: drop unportable fputc_unlocked
>
>   hw/vga.c |   13 ++++++++++---
>   1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/vga.c b/hw/vga.c
> index d5bc582..97c96bf 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2349,15 +2349,19 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>       uint32_t v;
>       int y, x;
>       uint8_t r, g, b;
> +    int ret;
> +    char *linebuf, *pbuf;
>
>       f = fopen(filename, "wb");
>       if (!f)
>           return -1;
>       fprintf(f, "P6\n%d %d\n%d\n",
>               ds->width, ds->height, 255);
> +    linebuf = qemu_malloc(ds->width * 3);
>       d1 = ds->data;
>       for(y = 0; y<  ds->height; y++) {
>           d = d1;
> +        pbuf = linebuf;
>           for(x = 0; x<  ds->width; x++) {
>               if (ds->pf.bits_per_pixel == 32)
>                   v = *(uint32_t *)d;
> @@ -2369,13 +2373,16 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>                   (ds->pf.gmax + 1);
>               b = ((v>>  ds->pf.bshift)&  ds->pf.bmax) * 256 /
>                   (ds->pf.bmax + 1);
> -            fputc(r, f);
> -            fputc(g, f);
> -            fputc(b, f);
> +            *pbuf++ = r;
> +            *pbuf++ = g;
> +            *pbuf++ = b;
>               d += ds->pf.bytes_per_pixel;
>           }
>           d1 += ds->linesize;
> +        ret = fwrite(linebuf, 1, pbuf - linebuf, f);
> +        (void)ret;
>       }
> +    qemu_free(linebuf);
>       fclose(f);
>       return 0;
>   }

Re: [Qemu-devel] [PATCH v2] Optimize screendump

[Andreas Färber]

Am 20.06.2011 um 15:11 schrieb Avi Kivity:

> On 06/20/2011 03:33 PM, Jan Kiszka wrote:
>> >  --- a/hw/vga.c
>> >  +++ b/hw/vga.c
>> >  @@ -2349,15 +2349,19 @@ int ppm_save(const char *filename,  
>> struct DisplaySurface *ds)
>
>> Unrelated to this patch, but why is this function located in vga.c  
>> and
>> not in console.c?
>
> It's located in omap_lcdc.c  as well.  But it needs to be fully  
> generalized to be moved out (handle all PixelFormats).

For the record, there's a similar function in tcx.c as well, and I  
have one coming in ibm8514.c.

Andreas

Re: [Qemu-devel] [PATCH v2] Optimize screendump

[Blue Swirl]

On Thu, Jun 23, 2011 at 12:22 AM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 20.06.2011 um 15:11 schrieb Avi Kivity:
>
>> On 06/20/2011 03:33 PM, Jan Kiszka wrote:
>>>
>>> >  --- a/hw/vga.c
>>> >  +++ b/hw/vga.c
>>> >  @@ -2349,15 +2349,19 @@ int ppm_save(const char *filename, struct
>>> > DisplaySurface *ds)
>>
>>> Unrelated to this patch, but why is this function located in vga.c and
>>> not in console.c?
>>
>> It's located in omap_lcdc.c  as well.  But it needs to be fully
>> generalized to be moved out (handle all PixelFormats).
>
> For the record, there's a similar function in tcx.c as well, and I have one
> coming in ibm8514.c.

The screen dumpers generate their output based on the current state of
the graphics card and the VRAM, this is why they are device specific.

A generic screen dumper (if possible) would read the data from display
surface. Maybe this should be done at the SDL/VNC/Spice/curses/dummy
level, but the output shouldn't change depending on the back end in
question.