Re: [PATCH 2/2] dma-buf/dma-fence: add wrappers for driver and timeline name

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux