diff mbox

make vga screen_dump use DisplayState properly

Message ID alpine.DEB.2.00.0908111538170.28872@kaball-desktop
State Superseded
Headers show

Commit Message

Stefano Stabellini Aug. 11, 2009, 3:18 p.m. UTC 
Hi all,
currently the vga screen_dump code doesn't use the DisplayState
interface properly and tries to replace it temporarily while taking the
screenshot.
A better approach is to register a DisplayChangeListener, call
vga_hw_update, and finally write the ppm in the next call from dpy_update.

Testing is appreciated.


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

---

Comments

Anthony Liguori Aug. 11, 2009, 3:42 p.m. UTC | #1 
Stefano Stabellini wrote:
> Hi all,
> currently the vga screen_dump code doesn't use the DisplayState
> interface properly and tries to replace it temporarily while taking the
> screenshot.
> A better approach is to register a DisplayChangeListener, call
> vga_hw_update, and finally write the ppm in the next call from dpy_update.
>
> Testing is appreciated.
>   

Does this fix kvm-autotest Avi?

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> ---
>
> diff --git a/hw/vga.c b/hw/vga.c
> index 4a0f197..3882f20 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -150,6 +150,8 @@ static uint16_t expand2[256];
>  static uint8_t expand4to8[16];
>  
>  static void vga_screen_dump(void *opaque, const char *filename);
> +static char *screen_dump_filename;
> +static DisplayChangeListener *screen_dump_dcl;
>  
>  static void vga_dumb_update_retrace_info(VGAState *s)
>  {
> @@ -2548,9 +2550,13 @@ device_init(vga_register);
>  /********************************************************/
>  /* vga screen dump */
>  
> -static void vga_save_dpy_update(DisplayState *s,
> +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);
> +        screen_dump_filename = NULL;
> +    }
>  }
>  
>  static void vga_save_dpy_resize(DisplayState *s)
> @@ -2599,70 +2605,16 @@ int ppm_save(const char *filename, struct DisplaySurface *ds)
>      return 0;
>  }
>  
> -static void vga_screen_dump_blank(VGAState *s, const char *filename)
> -{
> -    FILE *f;
> -    unsigned int y, x, w, h;
> -    unsigned char blank_sample[3] = { 0, 0, 0 };
> -
> -    w = s->last_scr_width;
> -    h = s->last_scr_height;
> -
> -    f = fopen(filename, "wb");
> -    if (!f)
> -        return;
> -    fprintf(f, "P6\n%d %d\n%d\n", w, h, 255);
> -    for (y = 0; y < h; y++) {
> -        for (x = 0; x < w; x++) {
> -            fwrite(blank_sample, 3, 1, f);
> -        }
> -    }
> -    fclose(f);
> -}
> -
> -static void vga_screen_dump_common(VGAState *s, const char *filename,
> -                                   int w, int h)
> -{
> -    DisplayState *saved_ds, ds1, *ds = &ds1;
> -    DisplayChangeListener dcl;
> -
> -    /* XXX: this is a little hackish */
> -    vga_invalidate_display(s);
> -    saved_ds = s->ds;
> -
> -    memset(ds, 0, sizeof(DisplayState));
> -    memset(&dcl, 0, 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);
> -    ds->allocator = &default_allocator;
> -    ds->surface = qemu_create_displaysurface(ds, w, h);
> -
> -    s->ds = ds;
> -    s->graphic_mode = -1;
> -    vga_update_display(s);
> -
> -    ppm_save(filename, ds->surface);
> -
> -    qemu_free_displaysurface(ds);
> -    s->ds = saved_ds;
> -}
> -
> -static void vga_screen_dump_graphic(VGAState *s, const char *filename)
> +static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
>  {
> -    int w, h;
> +    DisplayChangeListener *dcl;
>  
> -    s->get_resolution(s, &w, &h);
> -    vga_screen_dump_common(s, filename, w, h);
> -}
> -
> -static void vga_screen_dump_text(VGAState *s, const char *filename)
> -{
> -    int w, h, cwidth, cheight;
> -
> -    vga_get_text_resolution(s, &w, &h, &cwidth, &cheight);
> -    vga_screen_dump_common(s, filename, w * cwidth, h * cheight);
> +    dcl = qemu_mallocz(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
> @@ -2671,11 +2623,11 @@ static void vga_screen_dump(void *opaque, const char *filename)
>  {
>      VGAState *s = (VGAState *)opaque;
>  
> -    if (!(s->ar_index & 0x20))
> -        vga_screen_dump_blank(s, filename);
> -    else if (s->gr[6] & 1)
> -        vga_screen_dump_graphic(s, filename);
> -    else
> -        vga_screen_dump_text(s, filename);
> +    if (!screen_dump_dcl)
> +        screen_dump_dcl = vga_screen_dump_init(s->ds);
> +
> +    screen_dump_filename = (char *)filename;
>      vga_invalidate_display(s);
> +    vga_hw_update();
>  }
> +
>
>
>
Stefano Stabellini Aug. 11, 2009, 4:25 p.m. UTC | #2 
On Tue, 11 Aug 2009, Avi Kivity wrote:
> On 08/11/2009 06:42 PM, Anthony Liguori wrote:
> > Stefano Stabellini wrote:
> >> Hi all,
> >> currently the vga screen_dump code doesn't use the DisplayState
> >> interface properly and tries to replace it temporarily while taking the
> >> screenshot.
> >> A better approach is to register a DisplayChangeListener, call
> >> vga_hw_update, and finally write the ppm in the next call from 
> >> dpy_update.
> >>
> >> Testing is appreciated.
> >
> > Does this fix kvm-autotest Avi?
> 
> It does.  Stefano, thanks for the quick response.
> 

You are welcome :)
Avi Kivity Aug. 11, 2009, 4:27 p.m. UTC | #3 
On 08/11/2009 06:42 PM, Anthony Liguori wrote:
> Stefano Stabellini wrote:
>> Hi all,
>> currently the vga screen_dump code doesn't use the DisplayState
>> interface properly and tries to replace it temporarily while taking the
>> screenshot.
>> A better approach is to register a DisplayChangeListener, call
>> vga_hw_update, and finally write the ppm in the next call from 
>> dpy_update.
>>
>> Testing is appreciated.
>
> Does this fix kvm-autotest Avi?

It does.  Stefano, thanks for the quick response.
Alexandre CABROL PERALES Aug. 12, 2009, 9:25 a.m. UTC | #4 
Avi Kivity a écrit :
> On 08/11/2009 06:42 PM, Anthony Liguori wrote:
>>> Testing is appreciated.
>>
>> Does this fix kvm-autotest Avi?
> 
> It does.  Stefano, thanks for the quick response.
> 

Hi all i just tested:
commit 0bd8246bfec1dfb2eb959f52db535572c0260f4c
Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date:   Mon Aug 3 16:14:39 2009 +0100



I tried to install windows nt 4.0

with qemu -vga cirrus
and on screen resize qemu crash with :
Exception on floating point

I did:
gdb ./qemu
run -s -vga cirrus -hda /dev/vg/winnt

and on crash i got:

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7f66c93d26e0 (LWP 16679)]
0x0000000000457632 in cirrus_bitblt_start (s=0x2598a58) at
/opt/qemu-unstable/hw/cirrus_vga.c:725
725         sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;

Could it be possible to solve it ?
Stefano Stabellini Aug. 12, 2009, 11:30 a.m. UTC | #5 
On Wed, 12 Aug 2009, Alexandre CABROL PERALES wrote:
> Avi Kivity a écrit :
> > On 08/11/2009 06:42 PM, Anthony Liguori wrote:
> >>> Testing is appreciated.
> >>
> >> Does this fix kvm-autotest Avi?
> > 
> > It does.  Stefano, thanks for the quick response.
> > 
> 
> Hi all i just tested:
> commit 0bd8246bfec1dfb2eb959f52db535572c0260f4c
> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Date:   Mon Aug 3 16:14:39 2009 +0100
> 
> 
> 
> I tried to install windows nt 4.0
> 
> with qemu -vga cirrus
> and on screen resize qemu crash with :
> Exception on floating point

I am sorry but my patch wasn't meant to fix your problem.


> I did:
> gdb ./qemu
> run -s -vga cirrus -hda /dev/vg/winnt
> 
> and on crash i got:
> 
> Program received signal SIGFPE, Arithmetic exception.
> [Switching to Thread 0x7f66c93d26e0 (LWP 16679)]
> 0x0000000000457632 in cirrus_bitblt_start (s=0x2598a58) at
> /opt/qemu-unstable/hw/cirrus_vga.c:725
> 725         sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
> 
> Could it be possible to solve it ?

Sure it can, but I haven't had the time to look at it yet.
Alexandre CABROL PERALES Aug. 12, 2009, 2:36 p.m. UTC | #6 
Stefano Stabellini a écrit :
> On Wed, 12 Aug 2009, Alexandre CABROL PERALES wrote:
> 
> I am sorry but my patch wasn't meant to fix your problem.
>>
>> Could it be possible to solve it ?
> 
> Sure it can, but I haven't had the time to look at it yet.

Thank's in advance.

regards
Alexandre CABROL PERALES Aug. 19, 2009, 12:21 p.m. UTC | #7 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I found the changeset which initiate the bug.

When i run windows nt (cirrus vga) with :
http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=b4fbd8798aeb7870221769576973aeed909d304b
It works well.

But when i try with:
http://git.savannah.gnu.org/cgit/qemu.git/commit/?id=2bec46dc97571a3c34b18fe4ca198e7bfbdca41f

I have only green background and nothing more.

So i guess that regression bug is due to : vga optimisation.

Can you find few time to solve it please?

I have extra question is it possible to have extra display size/color
with cirrus vga card: 1280x1024 32bit (True Color)

Until now when i use it i have only 1280x1024 65536bits.

If it can help we had to modify vmx file on VMWare to solve it by adding:
svga.maxWidth= "1600"
svga.maxHeight= "1200"
svga.vramSize= "7680000"



Thank you in advance.
- --
Alexandre CABROL PERALES
- --
Ingenieur Securite des Systemes d'Information
Mob. :    06.98.82.03.06
Mail :    alexandre.cabrol@artal.fr
Key fingerprint = 1E6B B8DF 5001 A6A8 E057  9D31 7B3B EAB1 4AE4 8953
- --
ARTAL Technologies

Rue Pierre-Gilles de Gennes
Ens."La Rue", Bat. 9, BP 38138
31681 Labege cedex

Tel. :    05.61.00.39.30
Fax :     05.61.00.20.43


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkqL7jEACgkQezvqsUrkiVPkFQCfQ3Lpd+iOEd/7tmqFXNlIObfe
8e8AoKCgZ7MKs+0iaLM+v5vWL6CmMFku
=NSuV
-----END PGP SIGNATURE-----
diff mbox

Patch

diff --git a/hw/vga.c b/hw/vga.c
index 4a0f197..3882f20 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -150,6 +150,8 @@  static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
 static void vga_screen_dump(void *opaque, const char *filename);
+static char *screen_dump_filename;
+static DisplayChangeListener *screen_dump_dcl;
 
 static void vga_dumb_update_retrace_info(VGAState *s)
 {
@@ -2548,9 +2550,13 @@  device_init(vga_register);
 /********************************************************/
 /* vga screen dump */
 
-static void vga_save_dpy_update(DisplayState *s,
+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);
+        screen_dump_filename = NULL;
+    }
 }
 
 static void vga_save_dpy_resize(DisplayState *s)
@@ -2599,70 +2605,16 @@  int ppm_save(const char *filename, struct DisplaySurface *ds)
     return 0;
 }
 
-static void vga_screen_dump_blank(VGAState *s, const char *filename)
-{
-    FILE *f;
-    unsigned int y, x, w, h;
-    unsigned char blank_sample[3] = { 0, 0, 0 };
-
-    w = s->last_scr_width;
-    h = s->last_scr_height;
-
-    f = fopen(filename, "wb");
-    if (!f)
-        return;
-    fprintf(f, "P6\n%d %d\n%d\n", w, h, 255);
-    for (y = 0; y < h; y++) {
-        for (x = 0; x < w; x++) {
-            fwrite(blank_sample, 3, 1, f);
-        }
-    }
-    fclose(f);
-}
-
-static void vga_screen_dump_common(VGAState *s, const char *filename,
-                                   int w, int h)
-{
-    DisplayState *saved_ds, ds1, *ds = &ds1;
-    DisplayChangeListener dcl;
-
-    /* XXX: this is a little hackish */
-    vga_invalidate_display(s);
-    saved_ds = s->ds;
-
-    memset(ds, 0, sizeof(DisplayState));
-    memset(&dcl, 0, 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);
-    ds->allocator = &default_allocator;
-    ds->surface = qemu_create_displaysurface(ds, w, h);
-
-    s->ds = ds;
-    s->graphic_mode = -1;
-    vga_update_display(s);
-
-    ppm_save(filename, ds->surface);
-
-    qemu_free_displaysurface(ds);
-    s->ds = saved_ds;
-}
-
-static void vga_screen_dump_graphic(VGAState *s, const char *filename)
+static DisplayChangeListener* vga_screen_dump_init(DisplayState *ds)
 {
-    int w, h;
+    DisplayChangeListener *dcl;
 
-    s->get_resolution(s, &w, &h);
-    vga_screen_dump_common(s, filename, w, h);
-}
-
-static void vga_screen_dump_text(VGAState *s, const char *filename)
-{
-    int w, h, cwidth, cheight;
-
-    vga_get_text_resolution(s, &w, &h, &cwidth, &cheight);
-    vga_screen_dump_common(s, filename, w * cwidth, h * cheight);
+    dcl = qemu_mallocz(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
@@ -2671,11 +2623,11 @@  static void vga_screen_dump(void *opaque, const char *filename)
 {
     VGAState *s = (VGAState *)opaque;
 
-    if (!(s->ar_index & 0x20))
-        vga_screen_dump_blank(s, filename);
-    else if (s->gr[6] & 1)
-        vga_screen_dump_graphic(s, filename);
-    else
-        vga_screen_dump_text(s, filename);
+    if (!screen_dump_dcl)
+        screen_dump_dcl = vga_screen_dump_init(s->ds);
+
+    screen_dump_filename = (char *)filename;
     vga_invalidate_display(s);
+    vga_hw_update();
 }
+