Re: [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini)

[Aurelien Jarno]

On Fri, Jan 16, 2009 at 12:59:38PM +0000, Stefano Stabellini wrote:
> address@hidden wrote:
> 
> >> Revision: 6336
> >>          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6336
> >> Author:   aliguori
> >> Date:     2009-01-15 22:14:11 +0000 (Thu, 15 Jan 2009)
> >>
> >> Log Message:
> >> -----------
> >> DisplayState interface change (Stefano Stabellini)
> > 
> > This patch breaks qemu-system-ppc:
> > 
> > in update_palette256(), s->rgb_to_pixel() is used unitialized.
> > 
> > update_palette256() is called by vga_draw_graphic() whereas 
> > s->rgb_to_pixel() is initialized later in the function.
> > 
> > This patch correct the problem.
> > 
> 
> 
> 
> 
> 
> Thanks for pointing this out so quickly and for the fix!
> Another possible fix is to move update_palette256() after calling
> qemu_console_resize/qemu_create_displaysurface_from.
> 
> Even if I couldn't actually reproduce any bug triggered by a color depth 
> change on
> i386-softmmu, I think this second option is better because makes more
> obvious the fact that a change in DisplayState color depth affects both
> rgb_to_pixel and vga_draw_line functions.
> 
> Does it also fix your problem?

This patch does not apply. It looks like it is not done against the
current SVN. Doing the move by hand (see the patch below) fixes the
problem. I don't know which patch is the best one though.

diff --git a/hw/vga.c b/hw/vga.c
index f2d98b0..21af3ce 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1618,6 +1618,41 @@ static void vga_draw_graphic(VGAState *s, int 
full_update)
         s->double_scan = double_scan;
     }
 
+    depth = s->get_bpp(s);
+    if (s->line_offset != s->last_line_offset ||
+        disp_width != s->last_width ||
+        height != s->last_height ||
+        s->last_depth != depth) {
+        if (depth == 16 || depth == 32) {
+            if (is_graphic_console()) {
+                qemu_free_displaysurface(s->ds->surface);
+                s->ds->surface = qemu_create_displaysurface_from(disp_width, 
height, depth,
+                                                               s->line_offset,
+                                                               s->vram_ptr + 
(s->start_addr * 4));
+                dpy_resize(s->ds);
+            } else {
+                qemu_console_resize(s->console, disp_width, height);
+            }
+        } else {
+            qemu_console_resize(s->console, disp_width, height);
+        }
+        s->last_scr_width = disp_width;
+        s->last_scr_height = height;
+        s->last_width = disp_width;
+        s->last_height = height;
+        s->last_line_offset = s->line_offset;
+        s->last_depth = depth;
+        full_update = 1;
+    } else if (is_graphic_console() && is_buffer_shared(s->ds->surface) &&
+               (full_update || s->ds->surface->data != s->vram_ptr + 
(s->start_addr * 4))) {
+        s->ds->surface->data = s->vram_ptr + (s->start_addr * 4);
+        dpy_setdata(s->ds);
+    }
+
+    s->rgb_to_pixel =
+        rgb_to_pixel_dup_table[get_depth_index(s->ds)];
+
+
     if (shift_control == 0) {
         full_update |= update_palette16(s);
         if (s->sr[0x01] & 8) {
@@ -1669,40 +1704,6 @@ static void vga_draw_graphic(VGAState *s, int 
full_update)
     }
     vga_draw_line = vga_draw_line_table[v * NB_DEPTHS + 
get_depth_index(s->ds)];
 
-    depth = s->get_bpp(s);
-    if (s->line_offset != s->last_line_offset ||
-        disp_width != s->last_width ||
-        height != s->last_height ||
-        s->last_depth != depth) {
-        if (depth == 16 || depth == 32) {
-            if (is_graphic_console()) {
-                qemu_free_displaysurface(s->ds->surface);
-                s->ds->surface = qemu_create_displaysurface_from(disp_width, 
height, depth,
-                                                               s->line_offset,
-                                                               s->vram_ptr + 
(s->start_addr * 4));
-                dpy_resize(s->ds);
-            } else {
-                qemu_console_resize(s->console, disp_width, height);
-            }
-        } else {
-            qemu_console_resize(s->console, disp_width, height);
-        }
-        s->last_scr_width = disp_width;
-        s->last_scr_height = height;
-        s->last_width = disp_width;
-        s->last_height = height;
-        s->last_line_offset = s->line_offset;
-        s->last_depth = depth;
-        full_update = 1;
-    } else if (is_graphic_console() && is_buffer_shared(s->ds->surface) &&
-               (full_update || s->ds->surface->data != s->vram_ptr + 
(s->start_addr * 4))) {
-        s->ds->surface->data = s->vram_ptr + (s->start_addr * 4);
-        dpy_setdata(s->ds);
-    }
-
-    s->rgb_to_pixel =
-        rgb_to_pixel_dup_table[get_depth_index(s->ds)];
-
     if (!is_buffer_shared(s->ds->surface) && s->cursor_invalidate)
         s->cursor_invalidate(s);
 

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net