[Qemu-devel] [PATCH] fix screendump

[Qemu-devel] [PATCH] fix screendump

[Stefano Stabellini]

Hi all,
this patch fixes the screendump functionality that was recently broken;
it must be applied *after* PATCH 5, 6 and 7 of the original displaystate
change patch series.
In fact the other patches make much easier to solve the screendump
problem because they make the console switching mechanism more robust.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/hw/vga.c b/hw/vga.c
index f376ca6..50bd85f 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2533,8 +2533,6 @@ int pci_vga_init(PCIBus *bus, uint8_t *vga_ram_base,
 /********************************************************/
 /* vga screen dump */
 
-static int vga_save_w, vga_save_h;
-
 static void vga_save_dpy_update(DisplayState *s,
                                 int x, int y, int w, int h)
 {
@@ -2548,30 +2546,39 @@ static void vga_save_dpy_refresh(DisplayState *s)
 {
 }
 
-int ppm_save(const char *filename, uint8_t *data,
-             int w, int h, int linesize)
+int ppm_save(const char *filename, struct DisplaySurface *ds)
 {
     FILE *f;
     uint8_t *d, *d1;
-    unsigned int v;
+    uint32_t v;
     int y, x;
+    uint8_t r, g, b;
 
     f = fopen(filename, "wb");
     if (!f)
         return -1;
     fprintf(f, "P6\n%d %d\n%d\n",
-            w, h, 255);
-    d1 = data;
-    for(y = 0; y < h; y++) {
+            ds->width, ds->height, 255);
+    d1 = ds->data;
+    for(y = 0; y < ds->height; y++) {
         d = d1;
-        for(x = 0; x < w; x++) {
-            v = *(uint32_t *)d;
-            fputc((v >> 16) & 0xff, f);
-            fputc((v >> 8) & 0xff, f);
-            fputc((v) & 0xff, f);
-            d += 4;
+        for(x = 0; x < ds->width; x++) {
+            if (ds->pf.bits_per_pixel == 32)
+                v = *(uint32_t *)d;
+            else
+                v = (uint32_t) (*(uint16_t *)d);
+            r = ((v >> ds->pf.rshift) & ds->pf.rmax) * 256 /
+                (ds->pf.rmax + 1);
+            g = ((v >> ds->pf.gshift) & ds->pf.gmax) * 256 /
+                (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);
+            d += ds->pf.bytes_per_pixel;
         }
-        d1 += linesize;
+        d1 += ds->linesize;
     }
     fclose(f);
     return 0;
@@ -2613,15 +2620,13 @@ static void vga_screen_dump_common(VGAState *s, const char *filename,
     dcl.dpy_resize = vga_save_dpy_resize;
     dcl.dpy_refresh = vga_save_dpy_refresh;
     register_displaychangelistener(ds, &dcl);
-    ds->surface = qemu_create_displaysurface(ds_get_width(saved_ds),
-            ds_get_height(saved_ds), 32, 4 * ds_get_width(saved_ds));
+    ds->surface = qemu_create_displaysurface(w, h, 32, 4 * w);
 
     s->ds = ds;
     s->graphic_mode = -1;
     vga_update_display(s);
 
-    ppm_save(filename, ds_get_data(ds), vga_save_w, vga_save_h,
-            ds_get_linesize(ds));
+    ppm_save(filename, ds->surface);
 
     qemu_free_displaysurface(ds->surface);
     s->ds = saved_ds;
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 5d06eed..f97e98f 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -202,8 +202,7 @@ void vga_dirty_log_stop(VGAState *s);
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);
 void vga_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val);
 void vga_invalidate_scanlines(VGAState *s, int y1, int y2);
-int ppm_save(const char *filename, uint8_t *data,
-             int w, int h, int linesize);
+int ppm_save(const char *filename, struct DisplaySurface *ds);
 
 void vga_draw_cursor_line_8(uint8_t *d1, const uint8_t *src1,
                             int poffset, int w,
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 5b60074..950a98c 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -975,7 +975,10 @@ static void vmsvga_screen_dump(void *opaque, const char *filename)
     }
 
     if (s->depth == 32) {
-        ppm_save(filename, s->vram, s->width, s->height, ds_get_linesize(s->ds));
+        DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
+                s->height, 32, ds_get_linesize(s->ds), s->vram);
+        ppm_save(filename, ds);
+        qemu_free(ds);
     }
 }
 

Re: [Qemu-devel] [PATCH] fix screendump

[Anthony Liguori]

Stefano Stabellini wrote:
> Hi all,
> this patch fixes the screendump functionality that was recently broken;
> it must be applied *after* PATCH 5, 6 and 7 of the original displaystate
> change patch series.
>   

Hrm, I dislike that, this series is really not bisecting friendly.  
What's the decision on the PPC fix?  Are we going with Laurent's fix or 
yours?

I'll start testing the rest of the series and the new patches.

Regards,

Anthony Liguori

> In fact the other patches make much easier to solve the screendump
> problem because they make the console switching mechanism more robust.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> diff --git a/hw/vga.c b/hw/vga.c
> index f376ca6..50bd85f 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2533,8 +2533,6 @@ int pci_vga_init(PCIBus *bus, uint8_t *vga_ram_base,
>  /********************************************************/
>  /* vga screen dump */
>  
> -static int vga_save_w, vga_save_h;
> -
>  static void vga_save_dpy_update(DisplayState *s,
>                                  int x, int y, int w, int h)
>  {
> @@ -2548,30 +2546,39 @@ static void vga_save_dpy_refresh(DisplayState *s)
>  {
>  }
>  
> -int ppm_save(const char *filename, uint8_t *data,
> -             int w, int h, int linesize)
> +int ppm_save(const char *filename, struct DisplaySurface *ds)
>  {
>      FILE *f;
>      uint8_t *d, *d1;
> -    unsigned int v;
> +    uint32_t v;
>      int y, x;
> +    uint8_t r, g, b;
>  
>      f = fopen(filename, "wb");
>      if (!f)
>          return -1;
>      fprintf(f, "P6\n%d %d\n%d\n",
> -            w, h, 255);
> -    d1 = data;
> -    for(y = 0; y < h; y++) {
> +            ds->width, ds->height, 255);
> +    d1 = ds->data;
> +    for(y = 0; y < ds->height; y++) {
>          d = d1;
> -        for(x = 0; x < w; x++) {
> -            v = *(uint32_t *)d;
> -            fputc((v >> 16) & 0xff, f);
> -            fputc((v >> 8) & 0xff, f);
> -            fputc((v) & 0xff, f);
> -            d += 4;
> +        for(x = 0; x < ds->width; x++) {
> +            if (ds->pf.bits_per_pixel == 32)
> +                v = *(uint32_t *)d;
> +            else
> +                v = (uint32_t) (*(uint16_t *)d);
> +            r = ((v >> ds->pf.rshift) & ds->pf.rmax) * 256 /
> +                (ds->pf.rmax + 1);
> +            g = ((v >> ds->pf.gshift) & ds->pf.gmax) * 256 /
> +                (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);
> +            d += ds->pf.bytes_per_pixel;
>          }
> -        d1 += linesize;
> +        d1 += ds->linesize;
>      }
>      fclose(f);
>      return 0;
> @@ -2613,15 +2620,13 @@ static void vga_screen_dump_common(VGAState *s, const char *filename,
>      dcl.dpy_resize = vga_save_dpy_resize;
>      dcl.dpy_refresh = vga_save_dpy_refresh;
>      register_displaychangelistener(ds, &dcl);
> -    ds->surface = qemu_create_displaysurface(ds_get_width(saved_ds),
> -            ds_get_height(saved_ds), 32, 4 * ds_get_width(saved_ds));
> +    ds->surface = qemu_create_displaysurface(w, h, 32, 4 * w);
>  
>      s->ds = ds;
>      s->graphic_mode = -1;
>      vga_update_display(s);
>  
> -    ppm_save(filename, ds_get_data(ds), vga_save_w, vga_save_h,
> -            ds_get_linesize(ds));
> +    ppm_save(filename, ds->surface);
>  
>      qemu_free_displaysurface(ds->surface);
>      s->ds = saved_ds;
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 5d06eed..f97e98f 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -202,8 +202,7 @@ void vga_dirty_log_stop(VGAState *s);
>  uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);
>  void vga_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val);
>  void vga_invalidate_scanlines(VGAState *s, int y1, int y2);
> -int ppm_save(const char *filename, uint8_t *data,
> -             int w, int h, int linesize);
> +int ppm_save(const char *filename, struct DisplaySurface *ds);
>  
>  void vga_draw_cursor_line_8(uint8_t *d1, const uint8_t *src1,
>                              int poffset, int w,
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index 5b60074..950a98c 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -975,7 +975,10 @@ static void vmsvga_screen_dump(void *opaque, const char *filename)
>      }
>  
>      if (s->depth == 32) {
> -        ppm_save(filename, s->vram, s->width, s->height, ds_get_linesize(s->ds));
> +        DisplaySurface *ds = qemu_create_displaysurface_from(s->width,
> +                s->height, 32, ds_get_linesize(s->ds), s->vram);
> +        ppm_save(filename, ds);
> +        qemu_free(ds);
>      }
>  }
>  
>
>
>   

Re: [Qemu-devel] [PATCH] fix screendump

[Anthony Liguori]

Stefano Stabellini wrote:
> Hi all,
> this patch fixes the screendump functionality that was recently broken;
> it must be applied *after* PATCH 5, 6 and 7 of the original displaystate
> change patch series.
> In fact the other patches make much easier to solve the screendump
> problem because they make the console switching mechanism more robust.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>   

Applied.  Thanks.

That should be the whole series now (modulo the PPC fix).  Thanks for 
all the work you put into this!  Especially considering the delay over 
the holiday and the subsequent rebasing.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] fix screendump

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> Hi all,
>> this patch fixes the screendump functionality that was recently broken;
>> it must be applied *after* PATCH 5, 6 and 7 of the original displaystate
>> change patch series.
>> In fact the other patches make much easier to solve the screendump
>> problem because they make the console switching mechanism more robust.
>>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>   
> 
> Applied.  Thanks.
> 
> That should be the whole series now (modulo the PPC fix).  Thanks for
> all the work you put into this!  Especially considering the delay over
> the holiday and the subsequent rebasing.
> 

Thank you, for your work on this as well.

[Qemu-devel] [PATCH] fix screendump

[Gerd Hoffmann]

Commit 45efb16124efef51de5157afc31984b5a47700f9 optimized a bit too
much.  We can skip the vga_invalidate_display() in case no console
switch happened because we don't need a full redraw then.  We can *not*
skip vga_hw_update() though, because the screen content will be stale
then in case nobody else calls vga_hw_update().

Trigger: vga textmode with vnc display and no client connected.

Reported-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/blizzard.c  |    4 +---
 hw/omap_lcdc.c |    5 ++---
 hw/vga.c       |    2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/blizzard.c b/hw/blizzard.c
index c7d844d..29074c4 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -937,9 +937,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
 {
     BlizzardState *s = (BlizzardState *) opaque;
 
-    if (cswitch) {
-        blizzard_update_display(opaque);
-    }
+    blizzard_update_display(opaque);
     if (s && ds_get_data(s->state))
         ppm_save(filename, s->state->surface);
 }
diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
index f172093..4a08e9d 100644
--- a/hw/omap_lcdc.c
+++ b/hw/omap_lcdc.c
@@ -267,9 +267,8 @@ static int ppm_save(const char *filename, uint8_t *data,
 static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
 {
     struct omap_lcd_panel_s *omap_lcd = opaque;
-    if (cswitch) {
-        omap_update_display(opaque);
-    }
+
+    omap_update_display(opaque);
     if (omap_lcd && ds_get_data(omap_lcd->state))
         ppm_save(filename, ds_get_data(omap_lcd->state),
                 omap_lcd->width, omap_lcd->height,
diff --git a/hw/vga.c b/hw/vga.c
index 5994f43..16546ef 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2413,7 +2413,7 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
 
     if (cswitch) {
         vga_invalidate_display(s);
-        vga_hw_update();
     }
+    vga_hw_update();
     ppm_save(filename, s->ds->surface);
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH] fix screendump

[Alon Levy]

On Thu, Mar 01, 2012 at 08:34:40AM +0100, Gerd Hoffmann wrote:
> Commit 45efb16124efef51de5157afc31984b5a47700f9 optimized a bit too
> much.  We can skip the vga_invalidate_display() in case no console
> switch happened because we don't need a full redraw then.  We can *not*
> skip vga_hw_update() though, because the screen content will be stale
> then in case nobody else calls vga_hw_update().
> 
> Trigger: vga textmode with vnc display and no client connected.
> 

Reviewed-by: Alon Levy <alevy@redhat.com>

> Reported-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/blizzard.c  |    4 +---
>  hw/omap_lcdc.c |    5 ++---
>  hw/vga.c       |    2 +-
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index c7d844d..29074c4 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -937,9 +937,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
>  {
>      BlizzardState *s = (BlizzardState *) opaque;
>  
> -    if (cswitch) {
> -        blizzard_update_display(opaque);
> -    }
> +    blizzard_update_display(opaque);
>      if (s && ds_get_data(s->state))
>          ppm_save(filename, s->state->surface);
>  }
> diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> index f172093..4a08e9d 100644
> --- a/hw/omap_lcdc.c
> +++ b/hw/omap_lcdc.c
> @@ -267,9 +267,8 @@ static int ppm_save(const char *filename, uint8_t *data,
>  static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
>  {
>      struct omap_lcd_panel_s *omap_lcd = opaque;
> -    if (cswitch) {
> -        omap_update_display(opaque);
> -    }
> +
> +    omap_update_display(opaque);
>      if (omap_lcd && ds_get_data(omap_lcd->state))
>          ppm_save(filename, ds_get_data(omap_lcd->state),
>                  omap_lcd->width, omap_lcd->height,
> diff --git a/hw/vga.c b/hw/vga.c
> index 5994f43..16546ef 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2413,7 +2413,7 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
>  
>      if (cswitch) {
>          vga_invalidate_display(s);
> -        vga_hw_update();
>      }
> +    vga_hw_update();
>      ppm_save(filename, s->ds->surface);
>  }
> -- 
> 1.7.1
> 
> 

Re: [Qemu-devel] [PATCH] fix screendump

[Avi Kivity]

On 03/01/2012 09:34 AM, Gerd Hoffmann wrote:
> Commit 45efb16124efef51de5157afc31984b5a47700f9 optimized a bit too
> much.  We can skip the vga_invalidate_display() in case no console
> switch happened because we don't need a full redraw then.  We can *not*
> skip vga_hw_update() though, because the screen content will be stale
> then in case nobody else calls vga_hw_update().
>
> Trigger: vga textmode with vnc display and no client connected.
>
> Reported-by: Avi Kivity <avi@redhat.com>

-and-Tested.  Thanks.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

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

Re: [Qemu-devel] [PATCH] fix screendump

[Jan Kiszka]

On 2012-03-01 08:34, Gerd Hoffmann wrote:
> Commit 45efb16124efef51de5157afc31984b5a47700f9 optimized a bit too
> much.  We can skip the vga_invalidate_display() in case no console
> switch happened because we don't need a full redraw then.  We can *not*
> skip vga_hw_update() though, because the screen content will be stale
> then in case nobody else calls vga_hw_update().
> 
> Trigger: vga textmode with vnc display and no client connected.
> 
> Reported-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

I find this in qemu-kvm but not in upstream. Forgotten or just sleeping
in some queue?

Jan

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

Re: [Qemu-devel] [PATCH] fix screendump

[Blue Swirl]

Thanks, applied.

On Thu, Mar 1, 2012 at 07:34, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Commit 45efb16124efef51de5157afc31984b5a47700f9 optimized a bit too
> much.  We can skip the vga_invalidate_display() in case no console
> switch happened because we don't need a full redraw then.  We can *not*
> skip vga_hw_update() though, because the screen content will be stale
> then in case nobody else calls vga_hw_update().
>
> Trigger: vga textmode with vnc display and no client connected.
>
> Reported-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/blizzard.c  |    4 +---
>  hw/omap_lcdc.c |    5 ++---
>  hw/vga.c       |    2 +-
>  3 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/blizzard.c b/hw/blizzard.c
> index c7d844d..29074c4 100644
> --- a/hw/blizzard.c
> +++ b/hw/blizzard.c
> @@ -937,9 +937,7 @@ static void blizzard_screen_dump(void *opaque, const char *filename,
>  {
>     BlizzardState *s = (BlizzardState *) opaque;
>
> -    if (cswitch) {
> -        blizzard_update_display(opaque);
> -    }
> +    blizzard_update_display(opaque);
>     if (s && ds_get_data(s->state))
>         ppm_save(filename, s->state->surface);
>  }
> diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c
> index f172093..4a08e9d 100644
> --- a/hw/omap_lcdc.c
> +++ b/hw/omap_lcdc.c
> @@ -267,9 +267,8 @@ static int ppm_save(const char *filename, uint8_t *data,
>  static void omap_screen_dump(void *opaque, const char *filename, bool cswitch)
>  {
>     struct omap_lcd_panel_s *omap_lcd = opaque;
> -    if (cswitch) {
> -        omap_update_display(opaque);
> -    }
> +
> +    omap_update_display(opaque);
>     if (omap_lcd && ds_get_data(omap_lcd->state))
>         ppm_save(filename, ds_get_data(omap_lcd->state),
>                 omap_lcd->width, omap_lcd->height,
> diff --git a/hw/vga.c b/hw/vga.c
> index 5994f43..16546ef 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2413,7 +2413,7 @@ static void vga_screen_dump(void *opaque, const char *filename, bool cswitch)
>
>     if (cswitch) {
>         vga_invalidate_display(s);
> -        vga_hw_update();
>     }
> +    vga_hw_update();
>     ppm_save(filename, s->ds->surface);
>  }
> --
> 1.7.1
>
>