| Message ID | 20230712224424.30158-2-dongwon.kim@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/virtio: synchronous guest framebuffer update | expand |
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.
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. >
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?
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!
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.
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 >
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,
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(+)