[PULL 2/5] ui/gtk-egl: Check EGLSurface before doing scanout

marcandre.lureau@redhat.com posted 5 patches 1 year ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Akihiko Odaki <akihiko.odaki@daynix.com>
There is a newer version of this series
[PULL 2/5] ui/gtk-egl: Check EGLSurface before doing scanout
Posted by marcandre.lureau@redhat.com 1 year ago
From: Antonio Caggiano <quic_acaggian@quicinc.com>

The first time gd_egl_scanout_texture() is called, there's a possibility
that the GTK drawing area might not be realized yet, in which case its
associated GdkWindow is NULL. This means gd_egl_init() was also skipped
and the EGLContext and EGLSurface stored in the VirtualGfxConsole are
not valid yet.

Continuing with the scanout in this conditions would result in hitting
an assert in libepoxy: "Couldn't find current GLX or EGL context".

A possible workaround is to just ignore the scanout request, giving the
the GTK drawing area some time to finish its realization. At that point,
the gd_egl_init() will succeed and the EGLContext and EGLSurface stored
in the VirtualGfxConsole will be valid.

Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20231016123215.2699269-1-quic_acaggian@quicinc.com>
---
 ui/gtk-egl.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index a1060fd80f..3e8d1c1d02 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -243,12 +243,19 @@ void gd_egl_scanout_texture(DisplayChangeListener *dcl,
     vc->gfx.h = h;
     vc->gfx.y0_top = backing_y_0_top;
 
-    eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
-                   vc->gfx.esurface, vc->gfx.ectx);
+    if (!vc->gfx.esurface) {
+        gd_egl_init(vc);
+        if (!vc->gfx.esurface) {
+            return;
+        }
+
+        eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
+                       vc->gfx.esurface, vc->gfx.ectx);
 
-    gtk_egl_set_scanout_mode(vc, true);
-    egl_fb_setup_for_tex(&vc->gfx.guest_fb, backing_width, backing_height,
-                         backing_id, false);
+        gtk_egl_set_scanout_mode(vc, true);
+        egl_fb_setup_for_tex(&vc->gfx.guest_fb, backing_width, backing_height,
+                             backing_id, false);
+    }
 }
 
 void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
-- 
2.41.0


Re: [PULL 2/5] ui/gtk-egl: Check EGLSurface before doing scanout
Posted by Volker Rümelin 1 year ago
> From: Antonio Caggiano <quic_acaggian@quicinc.com>
>
> The first time gd_egl_scanout_texture() is called, there's a possibility
> that the GTK drawing area might not be realized yet, in which case its
> associated GdkWindow is NULL. This means gd_egl_init() was also skipped
> and the EGLContext and EGLSurface stored in the VirtualGfxConsole are
> not valid yet.
>
> Continuing with the scanout in this conditions would result in hitting
> an assert in libepoxy: "Couldn't find current GLX or EGL context".
>
> A possible workaround is to just ignore the scanout request, giving the
> the GTK drawing area some time to finish its realization. At that point,
> the gd_egl_init() will succeed and the EGLContext and EGLSurface stored
> in the VirtualGfxConsole will be valid.
>
> Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <20231016123215.2699269-1-quic_acaggian@quicinc.com>
> ---
>  ui/gtk-egl.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

Hi Antonio,

this patch breaks the QEMU guest display on my system. QEMU was started
with ./qemu-system-x86_64 -machine q35 -device
virtio-vga-gl,xres=1280,yres=768 -display gtk,zoom-to-fit=off,gl=on. I
can see the OVMF boot screen and then GRUB. After Linux was started,
plymouth normally shows the OVMF boot logo and a rotating spinner. With
your patch the guest screen is black and shows 'Display output is not
active.'. It seems the guest works without issues. I can use ssh to log
in and I can't find any obvious errors in the guest log files. The host
uses a GNOME desktop under X11.

If I revert this patch everything works as expected.

With best regards,
Volker

Cc: Michael Tokarev
This will affect Stable-7.2.7 and Stable-8.1.3.

> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> index a1060fd80f..3e8d1c1d02 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -243,12 +243,19 @@ void gd_egl_scanout_texture(DisplayChangeListener *dcl,
>      vc->gfx.h = h;
>      vc->gfx.y0_top = backing_y_0_top;
>  
> -    eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> -                   vc->gfx.esurface, vc->gfx.ectx);
> +    if (!vc->gfx.esurface) {
> +        gd_egl_init(vc);
> +        if (!vc->gfx.esurface) {
> +            return;
> +        }
> +
> +        eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> +                       vc->gfx.esurface, vc->gfx.ectx);
>  
> -    gtk_egl_set_scanout_mode(vc, true);
> -    egl_fb_setup_for_tex(&vc->gfx.guest_fb, backing_width, backing_height,
> -                         backing_id, false);
> +        gtk_egl_set_scanout_mode(vc, true);
> +        egl_fb_setup_for_tex(&vc->gfx.guest_fb, backing_width, backing_height,
> +                             backing_id, false);
> +    }
>  }
>  
>  void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,


Re: [PULL 2/5] ui/gtk-egl: Check EGLSurface before doing scanout
Posted by Volker Rümelin 1 year ago
>> From: Antonio Caggiano <quic_acaggian@quicinc.com>
>>
>> The first time gd_egl_scanout_texture() is called, there's a possibility
>> that the GTK drawing area might not be realized yet, in which case its
>> associated GdkWindow is NULL. This means gd_egl_init() was also skipped
>> and the EGLContext and EGLSurface stored in the VirtualGfxConsole are
>> not valid yet.
>>
>> Continuing with the scanout in this conditions would result in hitting
>> an assert in libepoxy: "Couldn't find current GLX or EGL context".
>>
>> A possible workaround is to just ignore the scanout request, giving the
>> the GTK drawing area some time to finish its realization. At that point,
>> the gd_egl_init() will succeed and the EGLContext and EGLSurface stored
>> in the VirtualGfxConsole will be valid.
>>
>> Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Message-Id: <20231016123215.2699269-1-quic_acaggian@quicinc.com>
>> ---
>>  ui/gtk-egl.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
> Hi Antonio,
>
> this patch breaks the QEMU guest display on my system. QEMU was started
> with ./qemu-system-x86_64 -machine q35 -device
> virtio-vga-gl,xres=1280,yres=768 -display gtk,zoom-to-fit=off,gl=on. I
> can see the OVMF boot screen and then GRUB. After Linux was started,
> plymouth normally shows the OVMF boot logo and a rotating spinner. With
> your patch the guest screen is black and shows 'Display output is not
> active.'. It seems the guest works without issues. I can use ssh to log
> in and I can't find any obvious errors in the guest log files. The host
> uses a GNOME desktop under X11.
>
> If I revert this patch everything works as expected.
>
> With best regards,
> Volker
>
> Cc: Michael Tokarev
> This will affect Stable-7.2.7 and Stable-8.1.3.
>
>> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
>> index a1060fd80f..3e8d1c1d02 100644
>> --- a/ui/gtk-egl.c
>> +++ b/ui/gtk-egl.c
>> @@ -243,12 +243,19 @@ void gd_egl_scanout_texture(DisplayChangeListener *dcl,
>>      vc->gfx.h = h;
>>      vc->gfx.y0_top = backing_y_0_top;
>>  
>> -    eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
>> -                   vc->gfx.esurface, vc->gfx.ectx);
>> +    if (!vc->gfx.esurface) {
>> +        gd_egl_init(vc);
>> +        if (!vc->gfx.esurface) {
>> +            return;
>> +        }
>> +
>> +        eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
>> +                       vc->gfx.esurface, vc->gfx.ectx);
>>  
>> -    gtk_egl_set_scanout_mode(vc, true);
>> -    egl_fb_setup_for_tex(&vc->gfx.guest_fb, backing_width, backing_height,
>> -                         backing_id, false);
>> +        gtk_egl_set_scanout_mode(vc, true);
>> +        egl_fb_setup_for_tex(&vc->gfx.guest_fb, backing_width, backing_height,
>> +                             backing_id, false);
>> +    }
>>  }
>>  
>>  void gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,

I can see the mistake. I'll write a patch to fix it.

With best regards,
Volker