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

2023-08-17 Thread 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 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
   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(>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

2023-08-17 Thread 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

2023-08-16 Thread 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 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
   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(>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

2023-08-16 Thread 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 
>>> Cc: Vivek Kasireddy 
>>> Signed-off-by: Dongwon Kim 
>>> ---
>>>   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(>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

2023-08-16 Thread 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 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
  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(>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

2023-08-14 Thread 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 
> Cc: Vivek Kasireddy 
> Signed-off-by: Dongwon Kim 
> ---
>  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(>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

2023-07-12 Thread Dongwon Kim 
virtio_gpu_fence_release is added to free virtio-gpu-fence
upon release of dma_fence.

Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 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(>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 1/3] drm/virtio: .release ops for virtgpu fence release

2022-09-26 Thread Dongwon Kim 
virtio_gpu_fence_release is added to free virtio-gpu-fence
upon release of dma_fence.

Cc: Gurchetan Singh 
Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 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(>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