* Re: [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini)
2009-01-16 12:59 ` Stefano Stabellini
@ 2009-01-16 19:13 ` Aurelien Jarno
2009-01-16 19:46 ` Aurelien Jarno
1 sibling, 0 replies; 5+ messages in thread
From: Aurelien Jarno @ 2009-01-16 19:13 UTC (permalink / raw)
To: qemu-devel
On Fri, Jan 16, 2009 at 12:59:38PM +0000, Stefano Stabellini wrote:
> laurent@lvivier.info 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
`. `' aurel32@debian.org | aurelien@aurel32.net
`- people.debian.org/~aurel32 | www.aurel32.net
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini)
2009-01-16 12:59 ` Stefano Stabellini
2009-01-16 19:13 ` Aurelien Jarno
@ 2009-01-16 19:46 ` Aurelien Jarno
2009-01-19 11:22 ` Stefano Stabellini
1 sibling, 1 reply; 5+ messages in thread
From: Aurelien Jarno @ 2009-01-16 19:46 UTC (permalink / raw)
To: qemu-devel
On Fri, Jan 16, 2009 at 12:59:38PM +0000, Stefano Stabellini wrote:
> laurent@lvivier.info 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
`. `' aurel32@debian.org | aurelien@aurel32.net
`- people.debian.org/~aurel32 | www.aurel32.net
^ permalink raw reply [flat|nested] 5+ messages in thread