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@xxxxxxx>
> ---
> 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@xxxxxxxx>
> + 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
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
