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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
Cc: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
Signed-off-by: Dongwon Kim <dongwon.kim@xxxxxxxxx>
---
   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!




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux