* RE: [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini)
@ 2009-01-16 11:13 laurent
2009-01-16 12:59 ` Stefano Stabellini
0 siblings, 1 reply; 5+ messages in thread
From: laurent @ 2009-01-16 11:13 UTC (permalink / raw)
To: qemu-devel
>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.
Signed-off-by: Laurent Vivier <Laurent@lvivier.info>
diff --git a/hw/vga.c b/hw/vga.c
index f2d98b0..769faea 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1838,6 +1838,7 @@ static void vga_update_display(void *opaque)
s->graphic_mode = graphic_mode;
full_update = 1;
}
+ s->rgb_to_pixel = rgb_to_pixel_dup_table[get_depth_index(s->ds)];
switch(graphic_mode) {
case GMODE_TEXT:
vga_draw_text(s, full_update);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini)
2009-01-16 11:13 [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini) laurent
@ 2009-01-16 12:59 ` Stefano Stabellini
2009-01-16 19:13 ` Aurelien Jarno
2009-01-16 19:46 ` Aurelien Jarno
0 siblings, 2 replies; 5+ messages in thread
From: Stefano Stabellini @ 2009-01-16 12:59 UTC (permalink / raw)
To: qemu-devel
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?
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);
^ 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
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
* Re: [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini)
2009-01-16 19:46 ` Aurelien Jarno
@ 2009-01-19 11:22 ` Stefano Stabellini
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Stabellini @ 2009-01-19 11:22 UTC (permalink / raw)
To: qemu-devel
Aurelien Jarno wrote:
> Now that Anthony have applied the remaining patches of your series, this
> patch applies and fix the problem. I have applied it.
Thank you.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-01-19 11:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-16 11:13 [Qemu-devel] [6336] DisplayState interface change (StefanoStabellini) laurent
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).