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

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

[marcandre.lureau]

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

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

[Mark Cave-Ayland]

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.

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

[Mark Cave-Ayland]

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.

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

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

[Gerd Hoffmann]

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