ui/sdl2: Support multiple OpenGL-enabled windows

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

Posted by Antonio Caggiano 2 years, 8 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 2 years, 8 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 2 years, 8 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 2 years, 8 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