From: Marc-André Lureau <marcandre.lureau@redhat.com>
The display may be corrupted when changing screen colour depth in
qemu-system-ppc/MacOS since 7.0.
Do not short-cut qemu_console_resize() if the surface is backed by vga
vram. When the scanout isn't set, or it is already allocated, or opengl,
and the size is fitting, we still avoid the reallocation & replace path.
Fixes: commit cb8962c1 ("ui: do not create a surface when resizing a GL scanout")
Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
ui/console.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/ui/console.c b/ui/console.c
index e139f7115e1f..765892f84f1c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2575,11 +2575,13 @@ static void vc_chr_open(Chardev *chr,
void qemu_console_resize(QemuConsole *s, int width, int height)
{
- DisplaySurface *surface;
+ DisplaySurface *surface = qemu_console_surface(s);
assert(s->console_type == GRAPHIC_CONSOLE);
- if (qemu_console_get_width(s, -1) == width &&
+ if ((s->scanout.kind != SCANOUT_SURFACE ||
+ (surface && surface->flags & QEMU_ALLOCATED_FLAG)) &&
+ qemu_console_get_width(s, -1) == width &&
qemu_console_get_height(s, -1) == height) {
return;
}
--
2.37.0.rc0
On 25/07/2022 12:58, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > The display may be corrupted when changing screen colour depth in > qemu-system-ppc/MacOS since 7.0. Is it worth being more specific here? Whilst MacOS with its NDRV driver exhibits the issue, it's really only because MacOS has separate selections for depth and resolution which allows one to be set without updating the other. I did a quick play with the Forth reproducer, and even with current git master the issue goes away if you also change the width/height at the same time as the depth. > Do not short-cut qemu_console_resize() if the surface is backed by vga > vram. When the scanout isn't set, or it is already allocated, or opengl, > and the size is fitting, we still avoid the reallocation & replace path. > > Fixes: commit cb8962c1 ("ui: do not create a surface when resizing a GL scanout") > > Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > ui/console.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/ui/console.c b/ui/console.c > index e139f7115e1f..765892f84f1c 100644 > --- a/ui/console.c > +++ b/ui/console.c > @@ -2575,11 +2575,13 @@ static void vc_chr_open(Chardev *chr, > > void qemu_console_resize(QemuConsole *s, int width, int height) > { > - DisplaySurface *surface; > + DisplaySurface *surface = qemu_console_surface(s); > > assert(s->console_type == GRAPHIC_CONSOLE); > > - if (qemu_console_get_width(s, -1) == width && > + if ((s->scanout.kind != SCANOUT_SURFACE || > + (surface && surface->flags & QEMU_ALLOCATED_FLAG)) && > + qemu_console_get_width(s, -1) == width && > qemu_console_get_height(s, -1) == height) { > return; > } The criteria listed for the short-cut in the commit message are quite handy, so is it worth adding a comment along the same lines as a reminder? Or is this logic touched so rarely that it isn't worthwhile? Regardless of the above, thanks for coming up with the patch and I can confirm that it fixes both the Forth reproducer and the changing of the Monitor colour depth in MacOS itself: Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
On 25/07/2022 17:35, Mark Cave-Ayland wrote: > On 25/07/2022 12:58, marcandre.lureau@redhat.com wrote: > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> The display may be corrupted when changing screen colour depth in >> qemu-system-ppc/MacOS since 7.0. > > Is it worth being more specific here? Whilst MacOS with its NDRV driver exhibits the > issue, it's really only because MacOS has separate selections for depth and > resolution which allows one to be set without updating the other. I did a quick play > with the Forth reproducer, and even with current git master the issue goes away if > you also change the width/height at the same time as the depth. > >> Do not short-cut qemu_console_resize() if the surface is backed by vga >> vram. When the scanout isn't set, or it is already allocated, or opengl, >> and the size is fitting, we still avoid the reallocation & replace path. >> >> Fixes: commit cb8962c1 ("ui: do not create a surface when resizing a GL scanout") >> >> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> ui/console.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/ui/console.c b/ui/console.c >> index e139f7115e1f..765892f84f1c 100644 >> --- a/ui/console.c >> +++ b/ui/console.c >> @@ -2575,11 +2575,13 @@ static void vc_chr_open(Chardev *chr, >> void qemu_console_resize(QemuConsole *s, int width, int height) >> { >> - DisplaySurface *surface; >> + DisplaySurface *surface = qemu_console_surface(s); >> assert(s->console_type == GRAPHIC_CONSOLE); >> - if (qemu_console_get_width(s, -1) == width && >> + if ((s->scanout.kind != SCANOUT_SURFACE || >> + (surface && surface->flags & QEMU_ALLOCATED_FLAG)) && >> + qemu_console_get_width(s, -1) == width && >> qemu_console_get_height(s, -1) == height) { >> return; >> } > > The criteria listed for the short-cut in the commit message are quite handy, so is it > worth adding a comment along the same lines as a reminder? Or is this logic touched > so rarely that it isn't worthwhile? > > Regardless of the above, thanks for coming up with the patch and I can confirm that > it fixes both the Forth reproducer and the changing of the Monitor colour depth in > MacOS itself: > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Hi Marc-André, Are you planning to submit this as a fix for 7.1? I think it would certainly qualify as a bug fix, and would likely affect users other than just qemu-system-ppc/MacOS. ATB, Mark.
Hi On Thu, Aug 4, 2022 at 12:11 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 25/07/2022 17:35, Mark Cave-Ayland wrote: > > > On 25/07/2022 12:58, marcandre.lureau@redhat.com wrote: > > > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > >> The display may be corrupted when changing screen colour depth in > >> qemu-system-ppc/MacOS since 7.0. > > > > Is it worth being more specific here? Whilst MacOS with its NDRV driver exhibits the > > issue, it's really only because MacOS has separate selections for depth and > > resolution which allows one to be set without updating the other. I did a quick play > > with the Forth reproducer, and even with current git master the issue goes away if > > you also change the width/height at the same time as the depth. > > > >> Do not short-cut qemu_console_resize() if the surface is backed by vga > >> vram. When the scanout isn't set, or it is already allocated, or opengl, > >> and the size is fitting, we still avoid the reallocation & replace path. > >> > >> Fixes: commit cb8962c1 ("ui: do not create a surface when resizing a GL scanout") > >> > >> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> --- > >> ui/console.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/ui/console.c b/ui/console.c > >> index e139f7115e1f..765892f84f1c 100644 > >> --- a/ui/console.c > >> +++ b/ui/console.c > >> @@ -2575,11 +2575,13 @@ static void vc_chr_open(Chardev *chr, > >> void qemu_console_resize(QemuConsole *s, int width, int height) > >> { > >> - DisplaySurface *surface; > >> + DisplaySurface *surface = qemu_console_surface(s); > >> assert(s->console_type == GRAPHIC_CONSOLE); > >> - if (qemu_console_get_width(s, -1) == width && > >> + if ((s->scanout.kind != SCANOUT_SURFACE || > >> + (surface && surface->flags & QEMU_ALLOCATED_FLAG)) && > >> + qemu_console_get_width(s, -1) == width && > >> qemu_console_get_height(s, -1) == height) { > >> return; > >> } > > > > The criteria listed for the short-cut in the commit message are quite handy, so is it > > worth adding a comment along the same lines as a reminder? Or is this logic touched > > so rarely that it isn't worthwhile? I don't know how often it will change, but it seems a bit fragile to me. I can add the commit comment along. > > > > Regardless of the above, thanks for coming up with the patch and I can confirm that > > it fixes both the Forth reproducer and the changing of the Monitor colour depth in > > MacOS itself: > > > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Hi Marc-André, > > Are you planning to submit this as a fix for 7.1? I think it would certainly qualify > as a bug fix, and would likely affect users other than just qemu-system-ppc/MacOS. Gerd, could you review the patch and let me send a MR ? (or do you have other UI patches queued already and take it?)
> > >> diff --git a/ui/console.c b/ui/console.c > > >> index e139f7115e1f..765892f84f1c 100644 > > >> --- a/ui/console.c > > >> +++ b/ui/console.c > > >> @@ -2575,11 +2575,13 @@ static void vc_chr_open(Chardev *chr, > > >> void qemu_console_resize(QemuConsole *s, int width, int height) > > >> { > > >> - DisplaySurface *surface; > > >> + DisplaySurface *surface = qemu_console_surface(s); > > >> assert(s->console_type == GRAPHIC_CONSOLE); > > >> - if (qemu_console_get_width(s, -1) == width && > > >> + if ((s->scanout.kind != SCANOUT_SURFACE || > > >> + (surface && surface->flags & QEMU_ALLOCATED_FLAG)) && > > >> + qemu_console_get_width(s, -1) == width && > > >> qemu_console_get_height(s, -1) == height) { > > >> return; > > >> } > Gerd, could you review the patch and let me send a MR ? (or do you > have other UI patches queued already and take it?) Patch looks good to me. Acked-by: Gerd Hoffmann <kraxel@redhat.com> [ just back from summer vacation, no pending queue atm, just started walking through my email backlog though ... ] take care, Gerd
© 2016 - 2024 Red Hat, Inc.