[PATCH v1 0/2] GTK/SDL fixes for a black screen displayed by virtio-gpu

[PATCH v1 0/2] GTK/SDL fixes for a black screen displayed by virtio-gpu

[Dmitry Osipenko]

Hi,

This patchset fixes black screen displayed by Qemu using virtio-gpu.
There is a race condition bug with a timer that disables display output
after it has been enabled by virtio-gpu. The problem is reproducible
by running Qemu with a disabled GL vsync. Note vsync is disabled for
SDL display by default.

Dmitry Osipenko (2):
  ui/sdl2: Don't disable scanout when display is refreshed
  ui/gtk: Don't disable scanout when display is refreshed

 ui/gtk-egl.c     | 1 -
 ui/gtk-gl-area.c | 1 -
 ui/sdl2-gl.c     | 1 -
 3 files changed, 3 deletions(-)

-- 
2.46.0

[PATCH v1 1/2] ui/sdl2: Don't disable scanout when display is refreshed

[Dmitry Osipenko]

Display refreshment is invoked by a timer and it erroneously disables
the active scanout if it happens to be invoked after scanout has been
enabled. This offending scanout-disable race condition with a timer
can be easily hit when Qemu runs with a disabled vsync by using SDL or
GTK displays (with vblank_mode=0 for GTK). Refreshment of display's
content shouldn't disable the active display. Fix it by keeping the
scanout's state unchanged when display is redrawn.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 ui/sdl2-gl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index e01d9ab0c7bf..4975d923db38 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -51,7 +51,6 @@ static void sdl2_gl_render_surface(struct sdl2_console *scon)
     int ww, wh;
 
     SDL_GL_MakeCurrent(scon->real_window, scon->winctx);
-    sdl2_set_scanout_mode(scon, false);
 
     SDL_GetWindowSize(scon->real_window, &ww, &wh);
     surface_gl_setup_viewport(scon->gls, scon->surface, ww, wh);
-- 
2.46.0

Re: [PATCH v1 1/2] ui/sdl2: Don't disable scanout when display is refreshed

[Akihiko Odaki]

On 2024/10/07 8:43, Dmitry Osipenko wrote:
> Display refreshment is invoked by a timer and it erroneously disables
> the active scanout if it happens to be invoked after scanout has been
> enabled. This offending scanout-disable race condition with a timer
> can be easily hit when Qemu runs with a disabled vsync by using SDL or
> GTK displays (with vblank_mode=0 for GTK). Refreshment of display's
> content shouldn't disable the active display. Fix it by keeping the
> scanout's state unchanged when display is redrawn.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

> ---
>   ui/sdl2-gl.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
> index e01d9ab0c7bf..4975d923db38 100644
> --- a/ui/sdl2-gl.c
> +++ b/ui/sdl2-gl.c
> @@ -51,7 +51,6 @@ static void sdl2_gl_render_surface(struct sdl2_console *scon)
>       int ww, wh;
>   
>       SDL_GL_MakeCurrent(scon->real_window, scon->winctx);
> -    sdl2_set_scanout_mode(scon, false);
>   
>       SDL_GetWindowSize(scon->real_window, &ww, &wh);
>       surface_gl_setup_viewport(scon->gls, scon->surface, ww, wh);

Re: [PATCH v1 1/2] ui/sdl2: Don't disable scanout when display is refreshed

[Bernhard Beschow]

Am 7. Oktober 2024 18:15:11 UTC schrieb Akihiko Odaki <akihiko.odaki@daynix.com>:
>On 2024/10/07 8:43, Dmitry Osipenko wrote:
>> Display refreshment is invoked by a timer and it erroneously disables
>> the active scanout if it happens to be invoked after scanout has been
>> enabled. This offending scanout-disable race condition with a timer
>> can be easily hit when Qemu runs with a disabled vsync by using SDL or
>> GTK displays (with vblank_mode=0 for GTK). Refreshment of display's
>> content shouldn't disable the active display. Fix it by keeping the
>> scanout's state unchanged when display is redrawn.
>> 
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>
>Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Tested-by: Bernhard Beschow <shentey@gmail.com>

>
>> ---
>>   ui/sdl2-gl.c | 1 -
>>   1 file changed, 1 deletion(-)
>> 
>> diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
>> index e01d9ab0c7bf..4975d923db38 100644
>> --- a/ui/sdl2-gl.c
>> +++ b/ui/sdl2-gl.c
>> @@ -51,7 +51,6 @@ static void sdl2_gl_render_surface(struct sdl2_console *scon)
>>       int ww, wh;
>>         SDL_GL_MakeCurrent(scon->real_window, scon->winctx);
>> -    sdl2_set_scanout_mode(scon, false);
>>         SDL_GetWindowSize(scon->real_window, &ww, &wh);
>>       surface_gl_setup_viewport(scon->gls, scon->surface, ww, wh);
>
>

Re: [PATCH v1 1/2] ui/sdl2: Don't disable scanout when display is refreshed

[Dmitry Osipenko]

On 10/7/24 21:15, Akihiko Odaki wrote:
> On 2024/10/07 8:43, Dmitry Osipenko wrote:
>> Display refreshment is invoked by a timer and it erroneously disables
>> the active scanout if it happens to be invoked after scanout has been
>> enabled. This offending scanout-disable race condition with a timer
>> can be easily hit when Qemu runs with a disabled vsync by using SDL or
>> GTK displays (with vblank_mode=0 for GTK). Refreshment of display's
>> content shouldn't disable the active display. Fix it by keeping the
>> scanout's state unchanged when display is redrawn.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
>> ---
>>   ui/sdl2-gl.c | 1 -
>>   1 file changed, 1 deletion(-)

Is anyone willing to apply this patchset? Maybe Alex? Thanks in advance!

-- 
Best regards,
Dmitry

[PATCH v1 2/2] ui/gtk: Don't disable scanout when display is refreshed

[Dmitry Osipenko]

Display refreshment is invoked by a timer and it erroneously disables
the active scanout if it happens to be invoked after scanout has been
enabled. This offending scanout-disable race condition with a timer
can be easily hit when Qemu runs with a disabled vsync by using SDL or
GTK displays (with vblank_mode=0 for GTK). Refreshment of display's
content shouldn't disable the active display. Fix it by keeping the
scanout's state unchanged when display is redrawn.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 ui/gtk-egl.c     | 1 -
 ui/gtk-gl-area.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 9831c10e1bd5..6b85a51c4284 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -179,7 +179,6 @@ void gd_egl_refresh(DisplayChangeListener *dcl)
 
     if (vc->gfx.glupdates) {
         vc->gfx.glupdates = 0;
-        gtk_egl_set_scanout_mode(vc, false);
         gd_egl_draw(vc);
     }
 }
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index b628b354510d..b00817abc011 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -148,7 +148,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
 
     if (vc->gfx.glupdates) {
         vc->gfx.glupdates = 0;
-        gtk_gl_area_set_scanout_mode(vc, false);
         gtk_gl_area_queue_render(GTK_GL_AREA(vc->gfx.drawing_area));
     }
 }
-- 
2.46.0

Re: [PATCH v1 2/2] ui/gtk: Don't disable scanout when display is refreshed

[Akihiko Odaki]

On 2024/10/07 8:43, Dmitry Osipenko wrote:
> Display refreshment is invoked by a timer and it erroneously disables
> the active scanout if it happens to be invoked after scanout has been
> enabled. This offending scanout-disable race condition with a timer
> can be easily hit when Qemu runs with a disabled vsync by using SDL or
> GTK displays (with vblank_mode=0 for GTK). Refreshment of display's
> content shouldn't disable the active display. Fix it by keeping the
> scanout's state unchanged when display is redrawn.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

> ---
>   ui/gtk-egl.c     | 1 -
>   ui/gtk-gl-area.c | 1 -
>   2 files changed, 2 deletions(-)
> 
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
> index 9831c10e1bd5..6b85a51c4284 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -179,7 +179,6 @@ void gd_egl_refresh(DisplayChangeListener *dcl)
>   
>       if (vc->gfx.glupdates) {
>           vc->gfx.glupdates = 0;
> -        gtk_egl_set_scanout_mode(vc, false);
>           gd_egl_draw(vc);
>       }
>   }
> diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
> index b628b354510d..b00817abc011 100644
> --- a/ui/gtk-gl-area.c
> +++ b/ui/gtk-gl-area.c
> @@ -148,7 +148,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
>   
>       if (vc->gfx.glupdates) {
>           vc->gfx.glupdates = 0;
> -        gtk_gl_area_set_scanout_mode(vc, false);
>           gtk_gl_area_queue_render(GTK_GL_AREA(vc->gfx.drawing_area));
>       }
>   }

Re: [PATCH v1 0/2] GTK/SDL fixes for a black screen displayed by virtio-gpu

[Michael Tokarev]

07.10.2024 02:43, Dmitry Osipenko wrote:
> Hi,
> 
> This patchset fixes black screen displayed by Qemu using virtio-gpu.
> There is a race condition bug with a timer that disables display output
> after it has been enabled by virtio-gpu. The problem is reproducible
> by running Qemu with a disabled GL vsync. Note vsync is disabled for
> SDL display by default.
> 
> Dmitry Osipenko (2):
>    ui/sdl2: Don't disable scanout when display is refreshed
>    ui/gtk: Don't disable scanout when display is refreshed

Is it a -stable material?  Myself I haven't seen this prob so far, so
it might be not worth the effort.  Also, any idea when the prob has been
introduced (or since when it has become real)?

Thanks,

/mjt

Re: [PATCH v1 0/2] GTK/SDL fixes for a black screen displayed by virtio-gpu

[Dmitry Osipenko]

On 10/7/24 22:26, Michael Tokarev wrote:
> 07.10.2024 02:43, Dmitry Osipenko wrote:
>> Hi,
>>
>> This patchset fixes black screen displayed by Qemu using virtio-gpu.
>> There is a race condition bug with a timer that disables display output
>> after it has been enabled by virtio-gpu. The problem is reproducible
>> by running Qemu with a disabled GL vsync. Note vsync is disabled for
>> SDL display by default.
>>
>> Dmitry Osipenko (2):
>>    ui/sdl2: Don't disable scanout when display is refreshed
>>    ui/gtk: Don't disable scanout when display is refreshed
> 
> Is it a -stable material?  Myself I haven't seen this prob so far, so
> it might be not worth the effort.  Also, any idea when the prob has been
> introduced (or since when it has become real)?

The problem is reproducible with a stable Qemu.  With SDL display it may
require many retries to repro, but with GTK display + vblank_mode=0 it
happens all the time.  Don't know when exactly it was introduced, but
will become much easier to hit it with the upcoming changes to Qemu.
Judging by the git blame, problem always existed, perhaps nobody payed
attention to it in the past.  Up to maintainers to decide whether it's
worth having these patches in stable, to me it's not worthwhile because
normally nobody would run Qemu with vblank_mode=0.  And you won't notice
the problem if you're running desktop session in the guest because it
will re-enable scanout on startup.

-- 
Best regards,
Dmitry

Re: [PATCH v1 0/2] GTK/SDL fixes for a black screen displayed by virtio-gpu

[Michael Tokarev]

07.10.2024 23:19, Dmitry Osipenko wrote:
>> Is it a -stable material?  Myself I haven't seen this prob so far, so
>> it might be not worth the effort.  Also, any idea when the prob has been
>> introduced (or since when it has become real)?
> 
> The problem is reproducible with a stable Qemu.  With SDL display it may

Which stable qemu do you mean? We've 4 currently active/supported stable
series - 7.2.x, 8.2.x, 9.0.x, 9.1.x :)

> require many retries to repro, but with GTK display + vblank_mode=0 it
> happens all the time.  Don't know when exactly it was introduced, but
> will become much easier to hit it with the upcoming changes to Qemu.

what is vblank_mode=0 anyway, how to turn it on/off ?

Do you mean setting this variable in environment when launching qemu?

Thanks,

/mjt

Re: [PATCH v1 0/2] GTK/SDL fixes for a black screen displayed by virtio-gpu

[Dmitry Osipenko]

On 10/7/24 23:48, Michael Tokarev wrote:
> 07.10.2024 23:19, Dmitry Osipenko wrote:
>>> Is it a -stable material?  Myself I haven't seen this prob so far, so
>>> it might be not worth the effort.  Also, any idea when the prob has been
>>> introduced (or since when it has become real)?
>>
>> The problem is reproducible with a stable Qemu.  With SDL display it may
> 
> Which stable qemu do you mean? We've 4 currently active/supported stable
> series - 7.2.x, 8.2.x, 9.0.x, 9.1.x :)

I check only the v8, v7 shouldn't differ from v8 in regards to the
display code, AFAICT.

>> require many retries to repro, but with GTK display + vblank_mode=0 it
>> happens all the time.  Don't know when exactly it was introduced, but
>> will become much easier to hit it with the upcoming changes to Qemu.
> 
> what is vblank_mode=0 anyway, how to turn it on/off ?
> 
> Do you mean setting this variable in environment when launching qemu?

Yes, you'll need to set this env var

-- 
Best regards,
Dmitry