[PATCH] virtio-gpu: do not replace surface when scanout is disabled

[PATCH] virtio-gpu: do not replace surface when scanout is disabled

[Dongwon Kim]

Surface is replaced with a place holder whenever the surface res
is unreferenced by the guest message. With this logic, there is
very frequent switching between guest display and the place holder
image, which is looking like a flickering display if the guest driver
is designed to unref the current scanout resource before sending out
a new scanout resource. So it is better to leave the current scanout
image until there is a new one flushed by the guest.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 hw/display/virtio-gpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 66cddd94d9..9d3e922c8f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -387,7 +387,6 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
         res->scanout_bitmask &= ~(1 << scanout_id);
     }
 
-    dpy_gfx_replace_surface(scanout->con, NULL);
     scanout->resource_id = 0;
     scanout->ds = NULL;
     scanout->width = 0;
-- 
2.34.1

Re: [PATCH] virtio-gpu: do not replace surface when scanout is disabled

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

Hi

On Wed, Jun 28, 2023 at 12:32 AM Dongwon Kim <dongwon.kim@intel.com> wrote:

> Surface is replaced with a place holder whenever the surface res
> is unreferenced by the guest message. With this logic, there is
> very frequent switching between guest display and the place holder
> image, which is looking like a flickering display if the guest driver
> is designed to unref the current scanout resource before sending out
> a new scanout resource. So it is better to leave the current scanout
> image until there is a new one flushed by the guest.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>

Why is the driver not setting a different scanout before destroying the
resource?

I think it's wrong to not replace the surface, as the associated scanout
resource may be destroyed or explicitly disabled for various purposes, and
we don't want to display garbage either.

---
>  hw/display/virtio-gpu.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 66cddd94d9..9d3e922c8f 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -387,7 +387,6 @@ static void virtio_gpu_disable_scanout(VirtIOGPU *g,
> int scanout_id)
>          res->scanout_bitmask &= ~(1 << scanout_id);
>      }
>
> -    dpy_gfx_replace_surface(scanout->con, NULL);
>      scanout->resource_id = 0;
>      scanout->ds = NULL;
>      scanout->width = 0;
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2645 bytes --]

Re: [PATCH] virtio-gpu: do not replace surface when scanout is disabled

[Kim, Dongwon]

On 7/4/2023 8:12 AM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jun 28, 2023 at 12:32 AM Dongwon Kim <dongwon.kim@intel.com> 
> wrote:
>
>     Surface is replaced with a place holder whenever the surface res
>     is unreferenced by the guest message. With this logic, there is
>     very frequent switching between guest display and the place holder
>     image, which is looking like a flickering display if the guest driver
>     is designed to unref the current scanout resource before sending out
>     a new scanout resource. So it is better to leave the current scanout
>     image until there is a new one flushed by the guest.
>
>     Cc: Gerd Hoffmann <kraxel@redhat.com>
>     Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>     Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>     Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>
>
> Why is the driver not setting a different scanout before destroying 
> the resource?
>
> I think it's wrong to not replace the surface, as the associated 
> scanout resource may be destroyed or explicitly disabled for various 
> purposes, and we don't want to display garbage either.

Yeah..I got your point. This is to address very specific to our use-case 
with windows guest that runs virtio-gpu like driver that does unref 
before the next framebuffer is set. And I agree that this sequence 
doesn't look right. Let me check if we could change the sequence in the 
guest driver.

>
>     ---
>      hw/display/virtio-gpu.c | 1 -
>      1 file changed, 1 deletion(-)
>
>     diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>     index 66cddd94d9..9d3e922c8f 100644
>     --- a/hw/display/virtio-gpu.c
>     +++ b/hw/display/virtio-gpu.c
>     @@ -387,7 +387,6 @@ static void
>     virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>              res->scanout_bitmask &= ~(1 << scanout_id);
>          }
>
>     -    dpy_gfx_replace_surface(scanout->con, NULL);
>          scanout->resource_id = 0;
>          scanout->ds = NULL;
>          scanout->width = 0;
>     -- 
>     2.34.1
>
>
>
>
> -- 
> Marc-André Lureau