ui/sdl2: Support multiple OpenGL-enabled windows

[PATCH] ui/sdl2: Support multiple OpenGL-enabled windows

Posted by Antonio Caggiano 11 months ago

Multiple graphics devices can be defined with an associated OpenGL
enabled SDL console, hence make sure to not destroy their shaders and
windows.

Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
---
 ui/sdl2-gl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index bbfa70eac3..795fb1afc9 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -89,7 +89,7 @@ void sdl2_gl_switch(DisplayChangeListener *dcl,
 
     scon->surface = new_surface;
 
-    if (is_placeholder(new_surface) && qemu_console_get_index(dcl->con)) {
+    if (is_placeholder(new_surface) && !scon->opengl) {
         qemu_gl_fini_shader(scon->gls);
         scon->gls = NULL;
         sdl2_window_destroy(scon);
-- 
2.40.0

Re: [PATCH] ui/sdl2: Support multiple OpenGL-enabled windows

Posted by Marc-André Lureau 11 months ago

Hi Antonio

On Wed, Jun 7, 2023 at 1:13 PM Antonio Caggiano <quic_acaggian@quicinc.com>
wrote:

> Multiple graphics devices can be defined with an associated OpenGL
> enabled SDL console, hence make sure to not destroy their shaders and
> windows.
>
>
Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
> ---
>  ui/sdl2-gl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
> index bbfa70eac3..795fb1afc9 100644
> --- a/ui/sdl2-gl.c
> +++ b/ui/sdl2-gl.c
> @@ -89,7 +89,7 @@ void sdl2_gl_switch(DisplayChangeListener *dcl,
>
>      scon->surface = new_surface;
>
> -    if (is_placeholder(new_surface) && qemu_console_get_index(dcl->con)) {
> +    if (is_placeholder(new_surface) && !scon->opengl) {
>          qemu_gl_fini_shader(scon->gls);
>          scon->gls = NULL;
>          sdl2_window_destroy(scon);
>

This was introduced in commit c821a58ee7003c2a0567dddaee33c2a5ae71c404 by
Akihiko.

Why should the window visibility behaviour be different whether it uses
opengl or not ?

If you are fixing a GL/shader crash, maybe it needs to be done differently.

thanks

Re: [PATCH] ui/sdl2: Support multiple OpenGL-enabled windows

Posted by Akihiko Odaki 11 months ago

On 2023/06/07 19:29, Marc-André Lureau wrote:
> Hi Antonio
> 
> On Wed, Jun 7, 2023 at 1:13 PM Antonio Caggiano 
> <quic_acaggian@quicinc.com <mailto:quic_acaggian@quicinc.com>> wrote:
> 
>     Multiple graphics devices can be defined with an associated OpenGL
>     enabled SDL console, hence make sure to not destroy their shaders and
>     windows.

I guess you meant multiple graphics devices can be associated to an 
OpenGL-enabled console and a switch event from a device destroys the 
shared state, but I don't see anything that associates multiple devices 
to a single console.

> 
>     Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com
>     <mailto:quic_acaggian@quicinc.com>>
>     ---
>       ui/sdl2-gl.c | 2 +-
>       1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
>     index bbfa70eac3..795fb1afc9 100644
>     --- a/ui/sdl2-gl.c
>     +++ b/ui/sdl2-gl.c
>     @@ -89,7 +89,7 @@ void sdl2_gl_switch(DisplayChangeListener *dcl,
> 
>           scon->surface = new_surface;
> 
>     -    if (is_placeholder(new_surface) &&
>     qemu_console_get_index(dcl->con)) {
>     +    if (is_placeholder(new_surface) && !scon->opengl) {
>               qemu_gl_fini_shader(scon->gls);
>               scon->gls = NULL;
>               sdl2_window_destroy(scon);
> 
> 
> This was introduced in commit c821a58ee7003c2a0567dddaee33c2a5ae71c404 
> by Akihiko.
> 
> Why should the window visibility behaviour be different whether it uses 
> opengl or not ?
> 
> If you are fixing a GL/shader crash, maybe it needs to be done differently.
> 
> thanks
> 
It does not make sense to check scon->opengl here; it should be always 
true when this function is called.

The condition qemu_console_get_index(dcl->con) should not be removed 
either. This keeps the first console persistent and makes sure the user 
can always interact with QEMU with the GUI SDL2 provides.

Regards,
Akihiko Odaki

Re: [PATCH] ui/sdl2: Support multiple OpenGL-enabled windows

Posted by Antonio Caggiano 11 months ago

Hi Marc-André and Akihiko,

On 07/06/2023 13:24, Akihiko Odaki wrote:
> On 2023/06/07 19:29, Marc-André Lureau wrote:
>> Hi Antonio
>>
>> On Wed, Jun 7, 2023 at 1:13 PM Antonio Caggiano 
>> <quic_acaggian@quicinc.com <mailto:quic_acaggian@quicinc.com>> wrote:
>>
>>     Multiple graphics devices can be defined with an associated OpenGL
>>     enabled SDL console, hence make sure to not destroy their shaders and
>>     windows.
> 
> I guess you meant multiple graphics devices can be associated to an 
> OpenGL-enabled console and a switch event from a device destroys the 
> shared state, but I don't see anything that associates multiple devices 
> to a single console.

The idea is to be able to run qemu with OpenGL-enabled SDL windows and 
multiple GPUs [0], e.g.:
-device virtio-vga-gl
-device virtio-vga

[0] 
https://user-images.githubusercontent.com/6058008/244386705-54654833-903e-478a-85f5-d951b7c448b4.mov

> 
>>
>>     Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com
>>     <mailto:quic_acaggian@quicinc.com>>
>>     ---
>>       ui/sdl2-gl.c | 2 +-
>>       1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>     diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
>>     index bbfa70eac3..795fb1afc9 100644
>>     --- a/ui/sdl2-gl.c
>>     +++ b/ui/sdl2-gl.c
>>     @@ -89,7 +89,7 @@ void sdl2_gl_switch(DisplayChangeListener *dcl,
>>
>>           scon->surface = new_surface;
>>
>>     -    if (is_placeholder(new_surface) &&
>>     qemu_console_get_index(dcl->con)) {
>>     +    if (is_placeholder(new_surface) && !scon->opengl) {
>>               qemu_gl_fini_shader(scon->gls);
>>               scon->gls = NULL;
>>               sdl2_window_destroy(scon);
>>
>>
>> This was introduced in commit c821a58ee7003c2a0567dddaee33c2a5ae71c404 
>> by Akihiko.
>>
>> Why should the window visibility behaviour be different whether it 
>> uses opengl or not ?
>>
>> If you are fixing a GL/shader crash, maybe it needs to be done 
>> differently.
>>
>> thanks
>>
> It does not make sense to check scon->opengl here; it should be always 
> true when this function is called.
> 
> The condition qemu_console_get_index(dcl->con) should not be removed 
> either. This keeps the first console persistent and makes sure the user 
> can always interact with QEMU with the GUI SDL2 provides.

The problem I encounter when adding a second GPUs is that its related 
SDL console gets its window and shaders destroyed, which are definitely 
something I need for rendering it. :D

Do you think where is a better way to avoid that?

> 
> Regards,
> Akihiko Odaki

Cheers,
Antonio Caggiano