[PFC PATCH 0/3] drm/virtio: synchronous guest framebuffer update

[PFC PATCH 0/3] drm/virtio: synchronous guest framebuffer update

[Dongwon Kim]

"Resubmission"

This series is for fixing issues regarding scanout synchronization with
host (e.g. QEMU/KVM) that uses virtio-gpu. This was submitted a while ago
but didn't get enough feedback/reviews so I am trying it again. This is a
rebased version. And the previous version is at
https://lists.freedesktop.org/archives/dri-devel/2022-September/373782.html

And very first version that has some feedbacks can be found here:
https://www.spinics.net/lists/dri-devel/msg349641.html

Dongwon Kim (3):
  drm/virtio: .release ops for virtgpu fence release
  drm/virtio: new fence for every plane update
  drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

 drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 +++
 drivers/gpu/drm/virtio/virtgpu_fence.c |  8 +++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 80 +++++++++++++++-----------
 3 files changed, 63 insertions(+), 32 deletions(-)

-- 
2.20.1

[RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Dongwon Kim]

virtio_gpu_fence_release is added to free virtio-gpu-fence
upon release of dma_fence.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index f28357dbde35..ba659ac2a51d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
 		 (u64)atomic64_read(&fence->drv->last_fence_id));
 }
 
+static void virtio_gpu_fence_release(struct dma_fence *f)
+{
+	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
+
+	kfree(fence);
+}
+
 static const struct dma_fence_ops virtio_gpu_fence_ops = {
 	.get_driver_name     = virtio_gpu_get_driver_name,
 	.get_timeline_name   = virtio_gpu_get_timeline_name,
 	.signaled            = virtio_gpu_fence_signaled,
 	.fence_value_str     = virtio_gpu_fence_value_str,
 	.timeline_value_str  = virtio_gpu_timeline_value_str,
+	.release	     = virtio_gpu_fence_release,
 };
 
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
-- 
2.20.1

Re: [RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Dmitry Osipenko]

On 7/13/23 01:44, Dongwon Kim wrote:
> virtio_gpu_fence_release is added to free virtio-gpu-fence
> upon release of dma_fence.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_fence.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
> index f28357dbde35..ba659ac2a51d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
> @@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
>  		 (u64)atomic64_read(&fence->drv->last_fence_id));
>  }
>  
> +static void virtio_gpu_fence_release(struct dma_fence *f)
> +{
> +	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
> +
> +	kfree(fence);
> +}
> +
>  static const struct dma_fence_ops virtio_gpu_fence_ops = {
>  	.get_driver_name     = virtio_gpu_get_driver_name,
>  	.get_timeline_name   = virtio_gpu_get_timeline_name,
>  	.signaled            = virtio_gpu_fence_signaled,
>  	.fence_value_str     = virtio_gpu_fence_value_str,
>  	.timeline_value_str  = virtio_gpu_timeline_value_str,
> +	.release	     = virtio_gpu_fence_release,
>  };
>  
>  struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,

This change doesn't do anything practically useful, AFAICT.

-- 
Best regards,
Dmitry

Re: [RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Kim, Dongwon]

Hi,

On 8/14/2023 9:18 PM, Dmitry Osipenko wrote:
> On 7/13/23 01:44, Dongwon Kim wrote:
>> virtio_gpu_fence_release is added to free virtio-gpu-fence
>> upon release of dma_fence.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>> ---
>>   drivers/gpu/drm/virtio/virtgpu_fence.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
>> index f28357dbde35..ba659ac2a51d 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>> @@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
>>   		 (u64)atomic64_read(&fence->drv->last_fence_id));
>>   }
>>   
>> +static void virtio_gpu_fence_release(struct dma_fence *f)
>> +{
>> +	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>> +
>> +	kfree(fence);
>> +}
>> +
>>   static const struct dma_fence_ops virtio_gpu_fence_ops = {
>>   	.get_driver_name     = virtio_gpu_get_driver_name,
>>   	.get_timeline_name   = virtio_gpu_get_timeline_name,
>>   	.signaled            = virtio_gpu_fence_signaled,
>>   	.fence_value_str     = virtio_gpu_fence_value_str,
>>   	.timeline_value_str  = virtio_gpu_timeline_value_str,
>> +	.release	     = virtio_gpu_fence_release,
>>   };
>>   
>>   struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
> This change doesn't do anything practically useful, AFAICT.

The intention of this ".release" is to free virtio_gpu_fence when the 
last dma_fence_put is done for the associated dma fence.

>

Re: [RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Dmitry Osipenko]

On 8/16/23 21:10, Kim, Dongwon wrote:
> Hi,
> 
> On 8/14/2023 9:18 PM, Dmitry Osipenko wrote:
>> On 7/13/23 01:44, Dongwon Kim wrote:
>>> virtio_gpu_fence_release is added to free virtio-gpu-fence
>>> upon release of dma_fence.
>>>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>>> ---
>>>   drivers/gpu/drm/virtio/virtgpu_fence.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c
>>> b/drivers/gpu/drm/virtio/virtgpu_fence.c
>>> index f28357dbde35..ba659ac2a51d 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>>> @@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct
>>> dma_fence *f, char *str,
>>>            (u64)atomic64_read(&fence->drv->last_fence_id));
>>>   }
>>>   +static void virtio_gpu_fence_release(struct dma_fence *f)
>>> +{
>>> +    struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>>> +
>>> +    kfree(fence);
>>> +}
>>> +
>>>   static const struct dma_fence_ops virtio_gpu_fence_ops = {
>>>       .get_driver_name     = virtio_gpu_get_driver_name,
>>>       .get_timeline_name   = virtio_gpu_get_timeline_name,
>>>       .signaled            = virtio_gpu_fence_signaled,
>>>       .fence_value_str     = virtio_gpu_fence_value_str,
>>>       .timeline_value_str  = virtio_gpu_timeline_value_str,
>>> +    .release         = virtio_gpu_fence_release,
>>>   };
>>>     struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct
>>> virtio_gpu_device *vgdev,
>> This change doesn't do anything practically useful, AFAICT.
> 
> The intention of this ".release" is to free virtio_gpu_fence when the
> last dma_fence_put is done for the associated dma fence.

What makes you think that fence won't be freed otherwise? Sounds like
haven't tried to check what dma_fence_release() code does, have you?

-- 
Best regards,
Dmitry

Re: [RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Kim, Dongwon]

Hi,

On 8/16/2023 10:05 PM, Dmitry Osipenko wrote:
> On 8/16/23 21:10, Kim, Dongwon wrote:
>> Hi,
>>
>> On 8/14/2023 9:18 PM, Dmitry Osipenko wrote:
>>> On 7/13/23 01:44, Dongwon Kim wrote:
>>>> virtio_gpu_fence_release is added to free virtio-gpu-fence
>>>> upon release of dma_fence.
>>>>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/virtio/virtgpu_fence.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> b/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> index f28357dbde35..ba659ac2a51d 100644
>>>> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> @@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct
>>>> dma_fence *f, char *str,
>>>>             (u64)atomic64_read(&fence->drv->last_fence_id));
>>>>    }
>>>>    +static void virtio_gpu_fence_release(struct dma_fence *f)
>>>> +{
>>>> +    struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>>>> +
>>>> +    kfree(fence);
>>>> +}
>>>> +
>>>>    static const struct dma_fence_ops virtio_gpu_fence_ops = {
>>>>        .get_driver_name     = virtio_gpu_get_driver_name,
>>>>        .get_timeline_name   = virtio_gpu_get_timeline_name,
>>>>        .signaled            = virtio_gpu_fence_signaled,
>>>>        .fence_value_str     = virtio_gpu_fence_value_str,
>>>>        .timeline_value_str  = virtio_gpu_timeline_value_str,
>>>> +    .release         = virtio_gpu_fence_release,
>>>>    };
>>>>      struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct
>>>> virtio_gpu_device *vgdev,
>>> This change doesn't do anything practically useful, AFAICT.
>> The intention of this ".release" is to free virtio_gpu_fence when the
>> last dma_fence_put is done for the associated dma fence.
> What makes you think that fence won't be freed otherwise? Sounds like
> haven't tried to check what dma_fence_release() code does, have you?

Yeah, I know it frees 'struct dma_fence *f' but what about 'struct virtio_gpu_fence *fence'? This is a device specific fence that contains struct dma_fence *f. But hold on... so when fence->ops->release is called then dma_fence_free won't be called here:

	if (fence->ops->release)
		fence->ops->release(fence);
	else
		dma_fence_free(fence);

In that case, I think virtio_gpu_fence_release should do "dma_fence_free(f)" before freeing virtio_gpu_fence? Am I right?
Like,

static void virtio_gpu_fence_release(struct dma_fence *f)
{
     struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);

     dma_fence_free(f);
     kfree(fence);
}

And can you please review the second and third patches in this series as well?
Thanks!

Re: [RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Dmitry Osipenko]

On 8/17/23 08:25, Kim, Dongwon wrote:
...
> Yeah, I know it frees 'struct dma_fence *f' but what about 'struct
> virtio_gpu_fence *fence'? This is a device specific fence that contains
> struct dma_fence *f. But hold on... so when fence->ops->release is
> called then dma_fence_free won't be called here:
> 
>     if (fence->ops->release)
>         fence->ops->release(fence);
>     else
>         dma_fence_free(fence);
> 
> In that case, I think virtio_gpu_fence_release should do
> "dma_fence_free(f)" before freeing virtio_gpu_fence? Am I right?
> Like,
> 
> static void virtio_gpu_fence_release(struct dma_fence *f)
> {
>     struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
> 
>     dma_fence_free(f);
>     kfree(fence);
> }

That is a double free and wrong of course. Both dma_fence *f and
virtio_gpu_fence *fence point at the same kmemory object. See
to_virtio_gpu_fence() and please research how container_of() works.

-- 
Best regards,
Dmitry

Re: [RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

[Kim, Dongwon]

On 8/16/2023 10:05 PM, Dmitry Osipenko wrote:
> On 8/16/23 21:10, Kim, Dongwon wrote:
>> Hi,
>>
>> On 8/14/2023 9:18 PM, Dmitry Osipenko wrote:
>>> On 7/13/23 01:44, Dongwon Kim wrote:
>>>> virtio_gpu_fence_release is added to free virtio-gpu-fence
>>>> upon release of dma_fence.
>>>>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/virtio/virtgpu_fence.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> b/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> index f28357dbde35..ba659ac2a51d 100644
>>>> --- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>>>> @@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct
>>>> dma_fence *f, char *str,
>>>>             (u64)atomic64_read(&fence->drv->last_fence_id));
>>>>    }
>>>>    +static void virtio_gpu_fence_release(struct dma_fence *f)
>>>> +{
>>>> +    struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>>>> +
>>>> +    kfree(fence);
>>>> +}
>>>> +
>>>>    static const struct dma_fence_ops virtio_gpu_fence_ops = {
>>>>        .get_driver_name     = virtio_gpu_get_driver_name,
>>>>        .get_timeline_name   = virtio_gpu_get_timeline_name,
>>>>        .signaled            = virtio_gpu_fence_signaled,
>>>>        .fence_value_str     = virtio_gpu_fence_value_str,
>>>>        .timeline_value_str  = virtio_gpu_timeline_value_str,
>>>> +    .release         = virtio_gpu_fence_release,
>>>>    };
>>>>      struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct
>>>> virtio_gpu_device *vgdev,
>>> This change doesn't do anything practically useful, AFAICT.
>> The intention of this ".release" is to free virtio_gpu_fence when the
>> last dma_fence_put is done for the associated dma fence.
> What makes you think that fence won't be freed otherwise? Sounds like
> haven't tried to check what dma_fence_release() code does, have you?

I see it now. For some reason, I assumed virtio_gpu_fence holds the 
pointer of dma_fence. This release ops is indeed not needed as you 
mentioned. Thanks

>

[RFC PATCH 2/3] drm/virtio: new fence for every plane update

[Dongwon Kim]

Having a fence linked to a virtio_gpu_framebuffer in plane update sequence
would cause conflict when several planes referencing the same framebuffer
especially when those planes are updated concurrently (e.g. Xorg screen
covering multi-displays configured for an extended mode). So it is better
for the fence to be created for every plane update event then link it to
the plane state since each plane update comes with a new plane state obj.

The plane state for virtio-gpu, "struct virtio_gpu_plane_state" is added for
this. This structure represents drm_plane_state and it contains the reference
to virtio_gpu_fence, which was previously in
"struct virtio_gpu_framebuffer".

"virtio_gpu_plane_duplicate_state" and "virtio_gpu_plane_destroy_state" were
added as well to manage virtio_gpu_plane_state.

Several drm helpers were slightly modified accordingly to use the fence in new
plane state structure. virtio_gpu_plane_cleanup_fb was completely removed as
none of code in the function are not required.

Also, the condition for adding fence, (plane->state->fb != new_state->fb) was
removed for the sychronous FB update even when the same FB is flushed again
consecutively.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 +++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 76 +++++++++++++++-----------
 2 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 4126c384286b..61fd37f95fbd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -191,6 +191,13 @@ struct virtio_gpu_framebuffer {
 #define to_virtio_gpu_framebuffer(x) \
 	container_of(x, struct virtio_gpu_framebuffer, base)
 
+struct virtio_gpu_plane_state {
+	struct drm_plane_state base;
+	struct virtio_gpu_fence *fence;
+};
+#define to_virtio_gpu_plane_state(x) \
+	container_of(x, struct virtio_gpu_plane_state, base)
+
 struct virtio_gpu_queue {
 	struct virtqueue *vq;
 	spinlock_t qlock;
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a2e045f3a000..a063f06ab6c5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -66,12 +66,36 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	return format;
 }
 
+static struct
+drm_plane_state *virtio_gpu_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct virtio_gpu_plane_state *new;
+
+	if (WARN_ON(!plane->state))
+		return NULL;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return NULL;
+
+	__drm_atomic_helper_plane_duplicate_state(plane, &new->base);
+
+	return &new->base;
+}
+
+static void virtio_gpu_plane_destroy_state(struct drm_plane *plane,
+					   struct drm_plane_state *state)
+{
+	__drm_atomic_helper_plane_destroy_state(state);
+	kfree(to_virtio_gpu_plane_state(state));
+}
+
 static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 	.disable_plane		= drm_atomic_helper_disable_plane,
 	.reset			= drm_atomic_helper_plane_reset,
-	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
-	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+	.atomic_duplicate_state = virtio_gpu_plane_duplicate_state,
+	.atomic_destroy_state	= virtio_gpu_plane_destroy_state,
 };
 
 static int virtio_gpu_plane_atomic_check(struct drm_plane *plane,
@@ -128,11 +152,13 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_framebuffer *vgfb;
+	struct virtio_gpu_plane_state *vgplane_st;
 	struct virtio_gpu_object *bo;
 
 	vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
+	vgplane_st = to_virtio_gpu_plane_state(plane->state);
 	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
-	if (vgfb->fence) {
+	if (vgplane_st->fence) {
 		struct virtio_gpu_object_array *objs;
 
 		objs = virtio_gpu_array_alloc(1);
@@ -141,13 +167,12 @@ static void virtio_gpu_resource_flush(struct drm_plane *plane,
 		virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
 		virtio_gpu_array_lock_resv(objs);
 		virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
-					      width, height, objs, vgfb->fence);
+					      width, height, objs,
+					      vgplane_st->fence);
 		virtio_gpu_notify(vgdev);
-
-		dma_fence_wait_timeout(&vgfb->fence->f, true,
+		dma_fence_wait_timeout(&vgplane_st->fence->f, true,
 				       msecs_to_jiffies(50));
-		dma_fence_put(&vgfb->fence->f);
-		vgfb->fence = NULL;
+		dma_fence_put(&vgplane_st->fence->f);
 	} else {
 		virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
 					      width, height, NULL, NULL);
@@ -237,41 +262,29 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_framebuffer *vgfb;
+	struct virtio_gpu_plane_state *vgplane_st;
 	struct virtio_gpu_object *bo;
 
 	if (!new_state->fb)
 		return 0;
 
 	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
+	vgplane_st = to_virtio_gpu_plane_state(new_state);
 	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
 	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
 		return 0;
 
-	if (bo->dumb && (plane->state->fb != new_state->fb)) {
-		vgfb->fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
+	if (bo->dumb) {
+		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
+						     vgdev->fence_drv.context,
 						     0);
-		if (!vgfb->fence)
+		if (!vgplane_st->fence)
 			return -ENOMEM;
 	}
 
 	return 0;
 }
 
-static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
-					struct drm_plane_state *state)
-{
-	struct virtio_gpu_framebuffer *vgfb;
-
-	if (!state->fb)
-		return;
-
-	vgfb = to_virtio_gpu_framebuffer(state->fb);
-	if (vgfb->fence) {
-		dma_fence_put(&vgfb->fence->f);
-		vgfb->fence = NULL;
-	}
-}
-
 static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 					   struct drm_atomic_state *state)
 {
@@ -281,6 +294,7 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_output *output = NULL;
 	struct virtio_gpu_framebuffer *vgfb;
+	struct virtio_gpu_plane_state *vgplane_st;
 	struct virtio_gpu_object *bo = NULL;
 	uint32_t handle;
 
@@ -293,6 +307,7 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 
 	if (plane->state->fb) {
 		vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
+		vgplane_st = to_virtio_gpu_plane_state(plane->state);
 		bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
 		handle = bo->hw_res_handle;
 	} else {
@@ -312,11 +327,10 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 			(vgdev, 0,
 			 plane->state->crtc_w,
 			 plane->state->crtc_h,
-			 0, 0, objs, vgfb->fence);
+			 0, 0, objs, vgplane_st->fence);
 		virtio_gpu_notify(vgdev);
-		dma_fence_wait(&vgfb->fence->f, true);
-		dma_fence_put(&vgfb->fence->f);
-		vgfb->fence = NULL;
+		dma_fence_wait(&vgplane_st->fence->f, true);
+		dma_fence_put(&vgplane_st->fence->f);
 	}
 
 	if (plane->state->fb != old_state->fb) {
@@ -351,14 +365,12 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
 
 static const struct drm_plane_helper_funcs virtio_gpu_primary_helper_funcs = {
 	.prepare_fb		= virtio_gpu_plane_prepare_fb,
-	.cleanup_fb		= virtio_gpu_plane_cleanup_fb,
 	.atomic_check		= virtio_gpu_plane_atomic_check,
 	.atomic_update		= virtio_gpu_primary_plane_update,
 };
 
 static const struct drm_plane_helper_funcs virtio_gpu_cursor_helper_funcs = {
 	.prepare_fb		= virtio_gpu_plane_prepare_fb,
-	.cleanup_fb		= virtio_gpu_plane_cleanup_fb,
 	.atomic_check		= virtio_gpu_plane_atomic_check,
 	.atomic_update		= virtio_gpu_cursor_plane_update,
 };
-- 
2.20.1

Re: [RFC PATCH 2/3] drm/virtio: new fence for every plane update

[Dmitry Osipenko]

...
> +static struct
> +drm_plane_state *virtio_gpu_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct virtio_gpu_plane_state *new;
> +
> +	if (WARN_ON(!plane->state))
> +		return NULL;

When plane->state can be NULL?

> +	new = kzalloc(sizeof(*new), GFP_KERNEL);
> +	if (!new)
> +		return NULL;
> +
> +	__drm_atomic_helper_plane_duplicate_state(plane, &new->base);
> +
> +	return &new->base;
> +}
> +
> +static void virtio_gpu_plane_destroy_state(struct drm_plane *plane,
> +					   struct drm_plane_state *state)
> +{
> +	__drm_atomic_helper_plane_destroy_state(state);
> +	kfree(to_virtio_gpu_plane_state(state));
> +}
> +
>  static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
>  	.update_plane		= drm_atomic_helper_update_plane,
>  	.disable_plane		= drm_atomic_helper_disable_plane,
>  	.reset			= drm_atomic_helper_plane_reset,
> -	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> -	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> +	.atomic_duplicate_state = virtio_gpu_plane_duplicate_state,
> +	.atomic_destroy_state	= virtio_gpu_plane_destroy_state,

Similar to the other email, please see how container_of() works. There
is no need change .atomic_destroy_state

...
> @@ -237,41 +262,29 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
>  	struct drm_device *dev = plane->dev;
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>  	struct virtio_gpu_framebuffer *vgfb;
> +	struct virtio_gpu_plane_state *vgplane_st;
>  	struct virtio_gpu_object *bo;
>  
>  	if (!new_state->fb)
>  		return 0;
>  
>  	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
> +	vgplane_st = to_virtio_gpu_plane_state(new_state);
>  	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
>  	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
>  		return 0;
>  
> -	if (bo->dumb && (plane->state->fb != new_state->fb)) {
> -		vgfb->fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
> +	if (bo->dumb) {

Why "&& (plane->state->fb != new_state->fb)" disappeared?

> +		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
> +						     vgdev->fence_drv.context,
>  						     0);
> -		if (!vgfb->fence)
> +		if (!vgplane_st->fence)
>  			return -ENOMEM;
>  	}
>  
>  	return 0;
>  }
>  
> -static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
> -					struct drm_plane_state *state)
> -{
> -	struct virtio_gpu_framebuffer *vgfb;
> -
> -	if (!state->fb)
> -		return;
> -
> -	vgfb = to_virtio_gpu_framebuffer(state->fb);
> -	if (vgfb->fence) {
> -		dma_fence_put(&vgfb->fence->f);
> -		vgfb->fence = NULL;
> -	}
> -}
How come that virtio_gpu_plane_cleanup_fb() isn't needed anymore? You
created fence in prepare_fb(), you must release it in cleanup_fb() if
fence still presents.

-- 
Best regards,
Dmitry

Re: [RFC PATCH 2/3] drm/virtio: new fence for every plane update

[Kim, Dongwon]

On 8/17/2023 7:21 PM, Dmitry Osipenko wrote:
> ...
>> +static struct
>> +drm_plane_state *virtio_gpu_plane_duplicate_state(struct drm_plane *plane)
>> +{
>> +	struct virtio_gpu_plane_state *new;
>> +
>> +	if (WARN_ON(!plane->state))
>> +		return NULL;
> When plane->state can be NULL?

Honestly this error check is from another drm driver. I am not sure if there is *any* case where this will hit. But wouldn't it safe to make sure this is not 
NULL here?

>> +	new = kzalloc(sizeof(*new), GFP_KERNEL);
>> +	if (!new)
>> +		return NULL;
>> +
>> +	__drm_atomic_helper_plane_duplicate_state(plane, &new->base);
>> +
>> +	return &new->base;
>> +}
>> +
>> +static void virtio_gpu_plane_destroy_state(struct drm_plane *plane,
>> +					   struct drm_plane_state *state)
>> +{
>> +	__drm_atomic_helper_plane_destroy_state(state);
>> +	kfree(to_virtio_gpu_plane_state(state));
>> +}
>> +
>>   static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
>>   	.update_plane		= drm_atomic_helper_update_plane,
>>   	.disable_plane		= drm_atomic_helper_disable_plane,
>>   	.reset			= drm_atomic_helper_plane_reset,
>> -	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> -	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
>> +	.atomic_duplicate_state = virtio_gpu_plane_duplicate_state,
>> +	.atomic_destroy_state	= virtio_gpu_plane_destroy_state,
> Similar to the other email, please see how container_of() works. There
> is no need change .atomic_destroy_state

Thanks again for pointing this out. I will fix this.

>
> ...
>> @@ -237,41 +262,29 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
>>   	struct drm_device *dev = plane->dev;
>>   	struct virtio_gpu_device *vgdev = dev->dev_private;
>>   	struct virtio_gpu_framebuffer *vgfb;
>> +	struct virtio_gpu_plane_state *vgplane_st;
>>   	struct virtio_gpu_object *bo;
>>   
>>   	if (!new_state->fb)
>>   		return 0;
>>   
>>   	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
>> +	vgplane_st = to_virtio_gpu_plane_state(new_state);
>>   	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
>>   	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
>>   		return 0;
>>   
>> -	if (bo->dumb && (plane->state->fb != new_state->fb)) {
>> -		vgfb->fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context,
>> +	if (bo->dumb) {
> Why "&& (plane->state->fb != new_state->fb)" disappeared?

Because same FB could be flushed again and fence is needed for synchronous operation in such case.

>
>> +		vgplane_st->fence = virtio_gpu_fence_alloc(vgdev,
>> +						     vgdev->fence_drv.context,
>>   						     0);
>> -		if (!vgfb->fence)
>> +		if (!vgplane_st->fence)
>>   			return -ENOMEM;
>>   	}
>>   
>>   	return 0;
>>   }
>>   
>> -static void virtio_gpu_plane_cleanup_fb(struct drm_plane *plane,
>> -					struct drm_plane_state *state)
>> -{
>> -	struct virtio_gpu_framebuffer *vgfb;
>> -
>> -	if (!state->fb)
>> -		return;
>> -
>> -	vgfb = to_virtio_gpu_framebuffer(state->fb);
>> -	if (vgfb->fence) {
>> -		dma_fence_put(&vgfb->fence->f);
>> -		vgfb->fence = NULL;
>> -	}
>> -}
> How come that virtio_gpu_plane_cleanup_fb() isn't needed anymore? You
> created fence in prepare_fb(), you must release it in cleanup_fb() if
> fence still presents.

This fence is put during virt-gpu dequeuing process (the response is received)
and eventually released after resource flush is done.

       virtio_gpu_notify(vgdev);
       dma_fence_wait_timeout(&vgplane_st->fence->f, true,
                              msecs_to_jiffies(50));
       dma_fence_put(&vgplane_st->fence->f);

But even so, I guess what you said makes sense ("if fence still presents").
I will check this again.

>

[RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Dongwon Kim]

This helper is needed for framebuffer synchronization. Old framebuffer data
is often displayed on the guest display without this helper.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a063f06ab6c5..e197299489ce 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -26,6 +26,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_gem_atomic_helper.h>
 
 #include "virtgpu_drv.h"
 
@@ -271,6 +272,9 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
 	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
 	vgplane_st = to_virtio_gpu_plane_state(new_state);
 	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
+
+	drm_gem_plane_helper_prepare_fb(plane, new_state);
+
 	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
 		return 0;
 
-- 
2.20.1

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Dmitry Osipenko]

On 7/13/23 01:44, Dongwon Kim wrote:
> This helper is needed for framebuffer synchronization. Old framebuffer data
> is often displayed on the guest display without this helper.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index a063f06ab6c5..e197299489ce 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -26,6 +26,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_gem_atomic_helper.h>
>  
>  #include "virtgpu_drv.h"
>  
> @@ -271,6 +272,9 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
>  	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
>  	vgplane_st = to_virtio_gpu_plane_state(new_state);
>  	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
> +
> +	drm_gem_plane_helper_prepare_fb(plane, new_state);

The implicit display BO sync should happen on a host side, unless you're
rendering with Venus and then displaying with virgl. Doing it on guest
side should be a major performance hit. Please provide a complete
description of your setup: what VMM you use, config options, what tests
you're running.

-- 
Best regards,
Dmitry

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Kim, Dongwon]

On 8/17/2023 7:33 PM, Dmitry Osipenko wrote:
> On 7/13/23 01:44, Dongwon Kim wrote:
>> This helper is needed for framebuffer synchronization. Old framebuffer data
>> is often displayed on the guest display without this helper.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>> ---
>>   drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> index a063f06ab6c5..e197299489ce 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> @@ -26,6 +26,7 @@
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_damage_helper.h>
>>   #include <drm/drm_fourcc.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>>   
>>   #include "virtgpu_drv.h"
>>   
>> @@ -271,6 +272,9 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
>>   	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
>>   	vgplane_st = to_virtio_gpu_plane_state(new_state);
>>   	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
>> +
>> +	drm_gem_plane_helper_prepare_fb(plane, new_state);
> The implicit display BO sync should happen on a host side, unless you're
> rendering with Venus and then displaying with virgl. Doing it on guest
> side should be a major performance hit. Please provide a complete
> description of your setup: what VMM you use, config options, what tests
> you're running.

We use virtio-gpu as a kms device while using i915 as the render device in our setup.
And we use QEMU as VMM. Virtio-gpu driver flushes the scanout to QEMU as a blob resource
(reference to the buffer). QEMU then creates a dmabuf using udmabuf for the blob
then renders it as a texture using OGL. The test I ran is simple. Just starting terminal
app and typing things to see if there is any frame regression. I believe this helper is
required since the BO on the guest is basically dmabuf that is being shared between i915
and virtio-gpu driver. I didn't think about the performance impact. If the impact is
too much and that is not acceptable, is there any other suggestions or some tests I can try?

Thanks!

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Dmitry Osipenko]

On 8/20/23 23:58, Kim, Dongwon wrote:
> On 8/17/2023 7:33 PM, Dmitry Osipenko wrote:
>> On 7/13/23 01:44, Dongwon Kim wrote:
>>> This helper is needed for framebuffer synchronization. Old
>>> framebuffer data
>>> is often displayed on the guest display without this helper.
>>>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>>> ---
>>>   drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
>>> b/drivers/gpu/drm/virtio/virtgpu_plane.c
>>> index a063f06ab6c5..e197299489ce 100644
>>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
>>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
>>> @@ -26,6 +26,7 @@
>>>   #include <drm/drm_atomic_helper.h>
>>>   #include <drm/drm_damage_helper.h>
>>>   #include <drm/drm_fourcc.h>
>>> +#include <drm/drm_gem_atomic_helper.h>
>>>     #include "virtgpu_drv.h"
>>>   @@ -271,6 +272,9 @@ static int virtio_gpu_plane_prepare_fb(struct
>>> drm_plane *plane,
>>>       vgfb = to_virtio_gpu_framebuffer(new_state->fb);
>>>       vgplane_st = to_virtio_gpu_plane_state(new_state);
>>>       bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
>>> +
>>> +    drm_gem_plane_helper_prepare_fb(plane, new_state);
>> The implicit display BO sync should happen on a host side, unless you're
>> rendering with Venus and then displaying with virgl. Doing it on guest
>> side should be a major performance hit. Please provide a complete
>> description of your setup: what VMM you use, config options, what tests
>> you're running.
> 
> We use virtio-gpu as a kms device while using i915 as the render device
> in our setup.
> And we use QEMU as VMM. Virtio-gpu driver flushes the scanout to QEMU as
> a blob resource
> (reference to the buffer). QEMU then creates a dmabuf using udmabuf for
> the blob
> then renders it as a texture using OGL. The test I ran is simple. Just
> starting terminal
> app and typing things to see if there is any frame regression. I believe
> this helper is
> required since the BO on the guest is basically dmabuf that is being
> shared between i915
> and virtio-gpu driver. I didn't think about the performance impact. If
> the impact is
> too much and that is not acceptable, is there any other suggestions or
> some tests I can try?

You can do fence-wait in the guest userspace/Mesa after blitting/drawing
to the udmabuf.

You may run popular vk/gl gfx benchmarks using gl/sdl outputs to see the
fps impact.

Virglrender today supports native contexts. The method you're using for
GPU priming was proven to be slow in comparison to multi-gpu native
contexts. There is ongoing work for supporting fence passing from guest
to host [1] that allows to do fence-syncing on host. You'll find links
to the WIP virtio-intel native context in [1] as well. You won't find
GPU priming support using native context in [1], patches hasn't been
published yet.

[1]
https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1138

Note that in general it's not acceptable to upstream patches that serve
downstream only. Yours display sync issue is irrelevant to the upstream
stack unless you're going to upstream all the VMM and guest userspace
patches, and in such case you should always publish all the patches and
provide links.

So, you need to check the performance impact and publish all the patches
to the relevant upstream projects.

-- 
Best regards,
Dmitry

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Kim, Dongwon]

On 8/23/2023 8:52 PM, Dmitry Osipenko wrote:
> On 8/20/23 23:58, Kim, Dongwon wrote:
>> On 8/17/2023 7:33 PM, Dmitry Osipenko wrote:
>>> On 7/13/23 01:44, Dongwon Kim wrote:
>>>> This helper is needed for framebuffer synchronization. Old
>>>> framebuffer data
>>>> is often displayed on the guest display without this helper.
>>>>
>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
>>>> b/drivers/gpu/drm/virtio/virtgpu_plane.c
>>>> index a063f06ab6c5..e197299489ce 100644
>>>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
>>>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
>>>> @@ -26,6 +26,7 @@
>>>>    #include <drm/drm_atomic_helper.h>
>>>>    #include <drm/drm_damage_helper.h>
>>>>    #include <drm/drm_fourcc.h>
>>>> +#include <drm/drm_gem_atomic_helper.h>
>>>>      #include "virtgpu_drv.h"
>>>>    @@ -271,6 +272,9 @@ static int virtio_gpu_plane_prepare_fb(struct
>>>> drm_plane *plane,
>>>>        vgfb = to_virtio_gpu_framebuffer(new_state->fb);
>>>>        vgplane_st = to_virtio_gpu_plane_state(new_state);
>>>>        bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
>>>> +
>>>> +    drm_gem_plane_helper_prepare_fb(plane, new_state);
>>> The implicit display BO sync should happen on a host side, unless you're
>>> rendering with Venus and then displaying with virgl. Doing it on guest
>>> side should be a major performance hit. Please provide a complete
>>> description of your setup: what VMM you use, config options, what tests
>>> you're running.
>> We use virtio-gpu as a kms device while using i915 as the render device
>> in our setup.
>> And we use QEMU as VMM. Virtio-gpu driver flushes the scanout to QEMU as
>> a blob resource
>> (reference to the buffer). QEMU then creates a dmabuf using udmabuf for
>> the blob
>> then renders it as a texture using OGL. The test I ran is simple. Just
>> starting terminal
>> app and typing things to see if there is any frame regression. I believe
>> this helper is
>> required since the BO on the guest is basically dmabuf that is being
>> shared between i915
>> and virtio-gpu driver. I didn't think about the performance impact. If
>> the impact is
>> too much and that is not acceptable, is there any other suggestions or
>> some tests I can try?
> You can do fence-wait in the guest userspace/Mesa after blitting/drawing
> to the udmabuf.

There is already synchronization between QEMU and virtio-gpu driver on the guest. Upon resource flush, virtio-gpu waits for the response for the message from the QEMU and QEMU sends out the response once rendering is done. The problem we are seeing is not that the rendering part is reusing the buffer before it's displayed by the QEMU. Problem we are facing is more like some frame is often not finished when "resource-flush" is issued. So unless there is a way for QEMU to wait for this fence (guest drm), I think we should have some synchronization point in the guest side.

I saw other DRM drivers, omap, tegra, vc4 and so on are doing the similar so I guess this is a generic solution for such cases. But I do understand your concern as the primary use case of virtio-gpu driver is for virgl. So this extra wait would cost some performance drop. But I have a couple of points here as well.

1. Wouldn't this extra wait caused by drm_gem_plane_helper_prepare_fb be minimal as the actual
rendering is done in the host?

2. Can we just make this helper called only if virgl is not used as 3D driver?

>
> You may run popular vk/gl gfx benchmarks using gl/sdl outputs to see the
> fps impact.

ok

>
> Virglrender today supports native contexts. The method you're using for
> GPU priming was proven to be slow in comparison to multi-gpu native
> contexts. There is ongoing work for supporting fence passing from guest
> to host [1] that allows to do fence-syncing on host. You'll find links
> to the WIP virtio-intel native context in [1] as well. You won't find
> GPU priming support using native context in [1], patches hasn't been
> published yet.
>
> [1]
> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1138
>
> Note that in general it's not acceptable to upstream patches that serve
> downstream only. Yours display sync issue is irrelevant to the upstream
> stack unless you're going to upstream all the VMM and guest userspace
> patches, and in such case you should always publish all the patches and
> provide links.
>
> So, you need to check the performance impact and publish all the patches
> to the relevant upstream projects.

QEMU has all patches regarding this (blob scanout support) but guest Mesa
patch for KMSRO is still outstanding.

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592

I understand this specific patch would need more discussion/justification but
what about other two, are you generally ok with those in the same series?

Thanks!!

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Dmitry Osipenko]

On 8/24/23 20:58, Kim, Dongwon wrote:
...
>> You can do fence-wait in the guest userspace/Mesa after blitting/drawing
>> to the udmabuf.
> 
> There is already synchronization between QEMU and virtio-gpu driver on
> the guest. Upon resource flush, virtio-gpu waits for the response for
> the message from the QEMU and QEMU sends out the response once rendering
> is done. The problem we are seeing is not that the rendering part is
> reusing the buffer before it's displayed by the QEMU. Problem we are
> facing is more like some frame is often not finished when
> "resource-flush" is issued. So unless there is a way for QEMU to wait
> for this fence (guest drm), I think we should have some synchronization
> point in the guest side.
> 
> I saw other DRM drivers, omap, tegra, vc4 and so on are doing the
> similar so I guess this is a generic solution for such cases. But I do
> understand your concern as the primary use case of virtio-gpu driver is
> for virgl. So this extra wait would cost some performance drop. But I
> have a couple of points here as well.
> 
> 1. Wouldn't this extra wait caused by drm_gem_plane_helper_prepare_fb be
> minimal as the actual
> rendering is done in the host?
> 
> 2. Can we just make this helper called only if virgl is not used as 3D
> driver?

The problem you described above shouldn't be resolved by your patch. You
need to wait for FB to be released by the host's display and not to
before GPU finished rendering on guest. I.e. you're swapping display
buffers and your dGPU starts rendering to the buffer that is in active
use by host's display, correct?

Maybe you need to do glFinish() on host after swapping buffers? But that
will block host for a long time.

For now I don't have solution.

>> Virglrender today supports native contexts. The method you're using for
>> GPU priming was proven to be slow in comparison to multi-gpu native
>> contexts. There is ongoing work for supporting fence passing from guest
>> to host [1] that allows to do fence-syncing on host. You'll find links
>> to the WIP virtio-intel native context in [1] as well. You won't find
>> GPU priming support using native context in [1], patches hasn't been
>> published yet.
>>
>> [1]
>> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1138
>>
>> Note that in general it's not acceptable to upstream patches that serve
>> downstream only. Yours display sync issue is irrelevant to the upstream
>> stack unless you're going to upstream all the VMM and guest userspace
>> patches, and in such case you should always publish all the patches and
>> provide links.
>>
>> So, you need to check the performance impact and publish all the patches
>> to the relevant upstream projects.
> 
> QEMU has all patches regarding this (blob scanout support) but guest Mesa
> patch for KMSRO is still outstanding.
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592
> 
> I understand this specific patch would need more
> discussion/justification but
> what about other two, are you generally ok with those in the same series?

The first patch should be dropped. The second one could be useful,
you'll need to provide step-by-step instruction for how to reproduce the
multi-display issue, please write it in the cover-letter for the next
patch version.

-- 
Best regards,
Dmitry

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Kim, Dongwon]

Hi Dmitry,

On 8/31/2023 3:51 PM, Dmitry Osipenko wrote:
> On 8/24/23 20:58, Kim, Dongwon wrote:
> ...
>>> You can do fence-wait in the guest userspace/Mesa after blitting/drawing
>>> to the udmabuf.
>> There is already synchronization between QEMU and virtio-gpu driver on
>> the guest. Upon resource flush, virtio-gpu waits for the response for
>> the message from the QEMU and QEMU sends out the response once rendering
>> is done. The problem we are seeing is not that the rendering part is
>> reusing the buffer before it's displayed by the QEMU. Problem we are
>> facing is more like some frame is often not finished when
>> "resource-flush" is issued. So unless there is a way for QEMU to wait
>> for this fence (guest drm), I think we should have some synchronization
>> point in the guest side.
>>
>> I saw other DRM drivers, omap, tegra, vc4 and so on are doing the
>> similar so I guess this is a generic solution for such cases. But I do
>> understand your concern as the primary use case of virtio-gpu driver is
>> for virgl. So this extra wait would cost some performance drop. But I
>> have a couple of points here as well.
>>
>> 1. Wouldn't this extra wait caused by drm_gem_plane_helper_prepare_fb be
>> minimal as the actual
>> rendering is done in the host?
>>
>> 2. Can we just make this helper called only if virgl is not used as 3D
>> driver?
> The problem you described above shouldn't be resolved by your patch. You
> need to wait for FB to be released by the host's display and not to
> before GPU finished rendering on guest. I.e. you're swapping display
> buffers and your dGPU starts rendering to the buffer that is in active
> use by host's display, correct?

I don't believe the guest will start rendering on the same FB while host is
consuming it because the virtio-gpu driver on the guest won't release the FB for the next
frame before it gets the virtio resp for the resource flush command and the host (QEMU)
will hold the response until the rendering is finished.

And having this helper clearly fixes the issue we encountered (some old frame is
sometimes shown..). But you might be right. The way I understood the original problem
might not be 100% correct as it's based on my assumption on how things were fixed
with addition of the helper. Then can you help me to approach to the real problem?
If it's really fixed by the helper, then what would the original issue be?

>
> Maybe you need to do glFinish() on host after swapping buffers? But that
> will block host for a long time.

I can try glFinish everytime after it draws a frame on the host. But..
If that is the issue, then wouldn't I expect some skipped frames, not old frames?

> For now I don't have solution.
>
>>> Virglrender today supports native contexts. The method you're using for
>>> GPU priming was proven to be slow in comparison to multi-gpu native
>>> contexts. There is ongoing work for supporting fence passing from guest
>>> to host [1] that allows to do fence-syncing on host. You'll find links
>>> to the WIP virtio-intel native context in [1] as well. You won't find
>>> GPU priming support using native context in [1], patches hasn't been
>>> published yet.
>>>
>>> [1]
>>> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1138
>>>
>>> Note that in general it's not acceptable to upstream patches that serve
>>> downstream only. Yours display sync issue is irrelevant to the upstream
>>> stack unless you're going to upstream all the VMM and guest userspace
>>> patches, and in such case you should always publish all the patches and
>>> provide links.
>>>
>>> So, you need to check the performance impact and publish all the patches
>>> to the relevant upstream projects.
>> QEMU has all patches regarding this (blob scanout support) but guest Mesa
>> patch for KMSRO is still outstanding.
>>
>> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/9592
>>
>> I understand this specific patch would need more
>> discussion/justification but
>> what about other two, are you generally ok with those in the same series?
> The first patch should be dropped. The second one could be useful,

Yes, the first one will be gone. I will upload a revised version of the second one
only with proper explanation.

> you'll need to provide step-by-step instruction for how to reproduce the
> multi-display issue, please write it in the cover-letter for the next
> patch version.

Thanks. Always appreciate your feedback!!

Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Dmitry Osipenko]

On 9/6/23 00:08, Kim, Dongwon wrote:
> 
> I don't believe the guest will start rendering on the same FB while host is
> consuming it because the virtio-gpu driver on the guest won't release
> the FB for the next
> frame before it gets the virtio resp for the resource flush command and
> the host (QEMU)
> will hold the response until the rendering is finished.

The virgl_cmd_set_scanout() shouldn't hold the response if you're using
SDL because frame swapping won't be vsynced. It may hold the response
implicitly if you're using GTK for the Qemu's display. Are you using SDL?

-- 
Best regards,
Dmitry

RE: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Kim, Dongwon]

Hi Dmitry,

Resource flush is what is waiting for the fence to be signaled. 

(in current code before my patches are applied)
static void virtio_gpu_resource_flush(struct drm_plane *plane,
                                      uint32_t x, uint32_t y,
                                      uint32_t width, uint32_t height)
......
                virtio_gpu_notify(vgdev);

                dma_fence_wait_timeout(&vgfb->fence->f, true,
                                       msecs_to_jiffies(50));
.......

We use gtk on the host.

Thanks!

-----Original Message-----
From: Dmitry Osipenko <dmitry.osipenko@collabora.com> 
Sent: Wednesday, October 4, 2023 4:32 PM
To: Kim, Dongwon <dongwon.kim@intel.com>; dri-devel@lists.freedesktop.org
Cc: Kasireddy, Vivek <vivek.kasireddy@intel.com>; kraxel@redhat.com
Subject: Re: [RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

On 9/6/23 00:08, Kim, Dongwon wrote:
> 
> I don't believe the guest will start rendering on the same FB while 
> host is consuming it because the virtio-gpu driver on the guest won't 
> release the FB for the next frame before it gets the virtio resp for 
> the resource flush command and the host (QEMU) will hold the response 
> until the rendering is finished.

The virgl_cmd_set_scanout() shouldn't hold the response if you're using SDL because frame swapping won't be vsynced. It may hold the response implicitly if you're using GTK for the Qemu's display. Are you using SDL?

--
Best regards,
Dmitry

[PFC PATCH 0/3] drm/virtio: synchronous guest framebuffer update

[Dongwon Kim]

This series is for fixing some issues regarding scanout synchronization with
host (e.g. QEMU/KVM) that uses virtio-gpu. This series replaces the previously
submitted one, "[PATCH v2 0/2] drm/virtio: fence handling in case of multi
scanouts".

Dongwon Kim (3):
  drm/virtio: .release ops for virtgpu fence release
  drm/virtio: new fence for every plane update
  drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

 drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 +++
 drivers/gpu/drm/virtio/virtgpu_fence.c |  8 +++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 85 +++++++++++++++-----------
 3 files changed, 65 insertions(+), 35 deletions(-)

-- 
2.20.1

[RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

[Dongwon Kim]

This helper is needed for framebuffer synchronization. Old framebuffer data
is often displayed on the guest display without this helper.

Cc: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 51b14ee4ece9..968afd0029fa 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -26,6 +26,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_gem_atomic_helper.h>
 
 #include "virtgpu_drv.h"
 
@@ -270,6 +271,9 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane *plane,
 	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
 	vgplane_st = to_virtio_gpu_plane_state(new_state);
 	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
+
+	drm_gem_plane_helper_prepare_fb(plane, new_state);
+
 	if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
 		return 0;
 
-- 
2.20.1