[RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release
Kim, Dongwon
dongwon.kim at intel.com
Thu Aug 17 05:25:29 UTC 2023
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 at redhat.com>
>>>> Cc: Vivek Kasireddy <vivek.kasireddy at intel.com>
>>>> Signed-off-by: Dongwon Kim <dongwon.kim at 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!
More information about the dri-devel
mailing list