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?

Now that Anthony have applied the remaining patches of your series, this
patch applies and fix the problem. I have applied it.
 
> diff --git a/hw/vga.c b/hw/vga.c
> index f376ca6..87125d9 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -1618,6 +1618,40 @@ 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->ds, disp_width, height);
> +            }
> +        } else {
> +            qemu_console_resize(s->ds, 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 +1703,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->ds, disp_width, height);
> -            }
> -        } else {
> -            qemu_console_resize(s->ds, 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