| Message ID | 20240918115513.2716-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dma-buf/dma-fence: remove unnecessary callbacks | expand |
On Wed, Sep 18, 2024 at 01:55:13PM +0200, Christian König wrote: > As discussed with Sima we want dma_fence objects to be able to outlive > their backend ops. Because of this timeline and driver name shouldn't > be queried any more after the fence has signaled. > > Add wrappers around the two queries and only return an empty string > if the fence was already signaled. There is still an obvious race > between signaling and querying the values, but that can only be > closed if we rework the locking as well. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-fence.c | 39 ++++++++++++++++++--- > drivers/dma-buf/sync_file.c | 8 ++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +- > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 4 +-- > drivers/gpu/drm/i915/i915_request.c | 2 +- > drivers/gpu/drm/i915/i915_sw_fence.c | 4 +-- > include/linux/dma-fence.h | 2 ++ > include/trace/events/dma_fence.h | 4 +-- > 8 files changed, 49 insertions(+), 16 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 0393a9bba3a8..d82f6c9ac018 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -538,8 +538,8 @@ void dma_fence_release(struct kref *kref) > if (WARN(!list_empty(&fence->cb_list) && > !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags), > "Fence %s:%s:%llx:%llx released with pending signals!\n", > - fence->ops->get_driver_name(fence), > - fence->ops->get_timeline_name(fence), > + dma_fence_driver_name(fence), > + dma_fence_timeline_name(fence), > fence->context, fence->seqno)) { > unsigned long flags; > > @@ -973,6 +973,37 @@ void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline) > } > EXPORT_SYMBOL(dma_fence_set_deadline); > > +/** > + * dma_fence_driver_name - return the driver name for a fence > + * @fence: the fence to query the driver name on > + * > + * Returns the driver name or empty string if the fence is already signaled. > + */ > +const char *dma_fence_driver_name(struct dma_fence *fence) > +{ I think a /* FIXME: blatantly racy, but better than nothig */ here and below would be good, just to make sure we don't forget. With that: Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch> > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return ""; > + > + return fence->ops->get_driver_name(fence); > +} > +EXPORT_SYMBOL(dma_fence_driver_name); > + > +/** > + * dma_fence_timeline_name - return the name of the fence context > + * @fence: the fence to query the context on > + * > + * Returns the name of the context this fence belongs to or empty string if the > + * fence is already signaled. > + */ > +const char *dma_fence_timeline_name(struct dma_fence *fence) > +{ > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > + return ""; > + > + return fence->ops->get_timeline_name(fence); > +} > +EXPORT_SYMBOL(dma_fence_timeline_name); > + > /** > * dma_fence_describe - Dump fence description into seq_file > * @fence: the fence to describe > @@ -983,8 +1014,8 @@ EXPORT_SYMBOL(dma_fence_set_deadline); > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) > { > seq_printf(seq, "%s %s seq %llu %ssignalled\n", > - fence->ops->get_driver_name(fence), > - fence->ops->get_timeline_name(fence), fence->seqno, > + dma_fence_driver_name(fence), > + dma_fence_timeline_name(fence), fence->seqno, > dma_fence_is_signaled(fence) ? "" : "un"); > } > EXPORT_SYMBOL(dma_fence_describe); > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index d9b1c1b2a72b..212df4b849fe 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -137,8 +137,8 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) > struct dma_fence *fence = sync_file->fence; > > snprintf(buf, len, "%s-%s%llu-%lld", > - fence->ops->get_driver_name(fence), > - fence->ops->get_timeline_name(fence), > + dma_fence_driver_name(fence), > + dma_fence_timeline_name(fence), > fence->context, > fence->seqno); > } > @@ -262,9 +262,9 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file, > static int sync_fill_fence_info(struct dma_fence *fence, > struct sync_fence_info *info) > { > - strscpy(info->obj_name, fence->ops->get_timeline_name(fence), > + strscpy(info->obj_name, dma_fence_timeline_name(fence), > sizeof(info->obj_name)); > - strscpy(info->driver_name, fence->ops->get_driver_name(fence), > + strscpy(info->driver_name, dma_fence_driver_name(fence), > sizeof(info->driver_name)); > > info->status = dma_fence_get_status(fence); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h > index 383fce40d4dd..224a40e03b36 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h > @@ -33,7 +33,7 @@ > #define TRACE_INCLUDE_FILE amdgpu_trace > > #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \ > - job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished) > + dma_fence_timeline_name(&job->base.s_fence->finished) > > TRACE_EVENT(amdgpu_device_rreg, > TP_PROTO(unsigned did, uint32_t reg, uint32_t value), > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c > index d1a382dfaa1d..ae3557ed6c1e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c > @@ -252,8 +252,8 @@ void intel_gt_watchdog_work(struct work_struct *work) > struct dma_fence *f = &rq->fence; > > pr_notice("Fence expiration time out i915-%s:%s:%llx!\n", > - f->ops->get_driver_name(f), > - f->ops->get_timeline_name(f), > + dma_fence_driver_name(f), > + dma_fence_timeline_name(f), > f->seqno); > i915_request_cancel(rq, -EINTR); > } > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 519e096c607c..aaec28fd4864 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -2184,7 +2184,7 @@ void i915_request_show(struct drm_printer *m, > const char *prefix, > int indent) > { > - const char *name = rq->fence.ops->get_timeline_name((struct dma_fence *)&rq->fence); > + const char *name = dma_fence_timeline_name((struct dma_fence *)&rq->fence); > char buf[80] = ""; > int x = 0; > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index 8a9aad523eec..b805ce8b8ab8 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -435,8 +435,8 @@ static void timer_i915_sw_fence_wake(struct timer_list *t) > return; > > pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n", > - cb->dma->ops->get_driver_name(cb->dma), > - cb->dma->ops->get_timeline_name(cb->dma), > + dma_fence_driver_name(cb->dma), > + dma_fence_timeline_name(cb->dma), > cb->dma->seqno, > i915_sw_fence_debug_hint(fence)); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index cf91cae6e30f..4b0634e42a36 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -264,6 +264,8 @@ void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, > > void dma_fence_release(struct kref *kref); > void dma_fence_free(struct dma_fence *fence); > +const char *dma_fence_driver_name(struct dma_fence *fence); > +const char *dma_fence_timeline_name(struct dma_fence *fence); > void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); > > /** > diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h > index a4de3df8500b..84c83074ee81 100644 > --- a/include/trace/events/dma_fence.h > +++ b/include/trace/events/dma_fence.h > @@ -16,8 +16,8 @@ DECLARE_EVENT_CLASS(dma_fence, > TP_ARGS(fence), > > TP_STRUCT__entry( > - __string(driver, fence->ops->get_driver_name(fence)) > - __string(timeline, fence->ops->get_timeline_name(fence)) > + __string(driver, dma_fence_driver_name(fence)) > + __string(timeline, dma_fence_timeline_name(fence)) > __field(unsigned int, context) > __field(unsigned int, seqno) > ), > -- > 2.34.1 >
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 0393a9bba3a8..d82f6c9ac018 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -538,8 +538,8 @@ void dma_fence_release(struct kref *kref) if (WARN(!list_empty(&fence->cb_list) && !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags), "Fence %s:%s:%llx:%llx released with pending signals!\n", - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), + dma_fence_driver_name(fence), + dma_fence_timeline_name(fence), fence->context, fence->seqno)) { unsigned long flags; @@ -973,6 +973,37 @@ void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline) } EXPORT_SYMBOL(dma_fence_set_deadline); +/** + * dma_fence_driver_name - return the driver name for a fence + * @fence: the fence to query the driver name on + * + * Returns the driver name or empty string if the fence is already signaled. + */ +const char *dma_fence_driver_name(struct dma_fence *fence) +{ + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + return ""; + + return fence->ops->get_driver_name(fence); +} +EXPORT_SYMBOL(dma_fence_driver_name); + +/** + * dma_fence_timeline_name - return the name of the fence context + * @fence: the fence to query the context on + * + * Returns the name of the context this fence belongs to or empty string if the + * fence is already signaled. + */ +const char *dma_fence_timeline_name(struct dma_fence *fence) +{ + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + return ""; + + return fence->ops->get_timeline_name(fence); +} +EXPORT_SYMBOL(dma_fence_timeline_name); + /** * dma_fence_describe - Dump fence description into seq_file * @fence: the fence to describe @@ -983,8 +1014,8 @@ EXPORT_SYMBOL(dma_fence_set_deadline); void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) { seq_printf(seq, "%s %s seq %llu %ssignalled\n", - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), fence->seqno, + dma_fence_driver_name(fence), + dma_fence_timeline_name(fence), fence->seqno, dma_fence_is_signaled(fence) ? "" : "un"); } EXPORT_SYMBOL(dma_fence_describe); diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index d9b1c1b2a72b..212df4b849fe 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -137,8 +137,8 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) struct dma_fence *fence = sync_file->fence; snprintf(buf, len, "%s-%s%llu-%lld", - fence->ops->get_driver_name(fence), - fence->ops->get_timeline_name(fence), + dma_fence_driver_name(fence), + dma_fence_timeline_name(fence), fence->context, fence->seqno); } @@ -262,9 +262,9 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file, static int sync_fill_fence_info(struct dma_fence *fence, struct sync_fence_info *info) { - strscpy(info->obj_name, fence->ops->get_timeline_name(fence), + strscpy(info->obj_name, dma_fence_timeline_name(fence), sizeof(info->obj_name)); - strscpy(info->driver_name, fence->ops->get_driver_name(fence), + strscpy(info->driver_name, dma_fence_driver_name(fence), sizeof(info->driver_name)); info->status = dma_fence_get_status(fence); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h index 383fce40d4dd..224a40e03b36 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h @@ -33,7 +33,7 @@ #define TRACE_INCLUDE_FILE amdgpu_trace #define AMDGPU_JOB_GET_TIMELINE_NAME(job) \ - job->base.s_fence->finished.ops->get_timeline_name(&job->base.s_fence->finished) + dma_fence_timeline_name(&job->base.s_fence->finished) TRACE_EVENT(amdgpu_device_rreg, TP_PROTO(unsigned did, uint32_t reg, uint32_t value), diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c index d1a382dfaa1d..ae3557ed6c1e 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c @@ -252,8 +252,8 @@ void intel_gt_watchdog_work(struct work_struct *work) struct dma_fence *f = &rq->fence; pr_notice("Fence expiration time out i915-%s:%s:%llx!\n", - f->ops->get_driver_name(f), - f->ops->get_timeline_name(f), + dma_fence_driver_name(f), + dma_fence_timeline_name(f), f->seqno); i915_request_cancel(rq, -EINTR); } diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 519e096c607c..aaec28fd4864 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -2184,7 +2184,7 @@ void i915_request_show(struct drm_printer *m, const char *prefix, int indent) { - const char *name = rq->fence.ops->get_timeline_name((struct dma_fence *)&rq->fence); + const char *name = dma_fence_timeline_name((struct dma_fence *)&rq->fence); char buf[80] = ""; int x = 0; diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index 8a9aad523eec..b805ce8b8ab8 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -435,8 +435,8 @@ static void timer_i915_sw_fence_wake(struct timer_list *t) return; pr_notice("Asynchronous wait on fence %s:%s:%llx timed out (hint:%ps)\n", - cb->dma->ops->get_driver_name(cb->dma), - cb->dma->ops->get_timeline_name(cb->dma), + dma_fence_driver_name(cb->dma), + dma_fence_timeline_name(cb->dma), cb->dma->seqno, i915_sw_fence_debug_hint(fence)); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index cf91cae6e30f..4b0634e42a36 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -264,6 +264,8 @@ void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, void dma_fence_release(struct kref *kref); void dma_fence_free(struct dma_fence *fence); +const char *dma_fence_driver_name(struct dma_fence *fence); +const char *dma_fence_timeline_name(struct dma_fence *fence); void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq); /** diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h index a4de3df8500b..84c83074ee81 100644 --- a/include/trace/events/dma_fence.h +++ b/include/trace/events/dma_fence.h @@ -16,8 +16,8 @@ DECLARE_EVENT_CLASS(dma_fence, TP_ARGS(fence), TP_STRUCT__entry( - __string(driver, fence->ops->get_driver_name(fence)) - __string(timeline, fence->ops->get_timeline_name(fence)) + __string(driver, dma_fence_driver_name(fence)) + __string(timeline, dma_fence_timeline_name(fence)) __field(unsigned int, context) __field(unsigned int, seqno) ),
As discussed with Sima we want dma_fence objects to be able to outlive their backend ops. Because of this timeline and driver name shouldn't be queried any more after the fence has signaled. Add wrappers around the two queries and only return an empty string if the fence was already signaled. There is still an obvious race between signaling and querying the values, but that can only be closed if we rework the locking as well. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-fence.c | 39 ++++++++++++++++++--- drivers/dma-buf/sync_file.c | 8 ++--- drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +- drivers/gpu/drm/i915/gt/intel_gt_requests.c | 4 +-- drivers/gpu/drm/i915/i915_request.c | 2 +- drivers/gpu/drm/i915/i915_sw_fence.c | 4 +-- include/linux/dma-fence.h | 2 ++ include/trace/events/dma_fence.h | 4 +-- 8 files changed, 49 insertions(+), 16 deletions(-)