Re: [PATCH] ui/console: fix qemu_console_resize() regression

[Marc-André Lureau]

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?)