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

[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 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: 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_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

[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: 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_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 9b98470593b0..20a418f64533 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -190,6 +190,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 4c09e313bebc..fd5e170dcb22 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

[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

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

>

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]

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/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/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 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

[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