Re: [PATCH v2 1/6] tracing, dma-buf: add a trace_dma_fence_sync_to event

["Christian König" <christian.koenig@amd.com>]

Am 13.02.24 um 16:50 schrieb Pierre-Eric Pelloux-Prayer:
> This new event can be used to trace where a given dma_fence is added
> as a dependency of some other work.
>
> I plan to use it in amdgpu.
>
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   drivers/dma-buf/dma-fence.c      |  1 +
>   include/trace/events/dma_fence.h | 34 ++++++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index e0fd99e61a2d..671a499a5ccd 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -23,6 +23,7 @@
>   EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit);
>   EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal);
>   EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled);
> +EXPORT_TRACEPOINT_SYMBOL(dma_fence_sync_to);
>   
>   static DEFINE_SPINLOCK(dma_fence_stub_lock);
>   static struct dma_fence dma_fence_stub;
> diff --git a/include/trace/events/dma_fence.h b/include/trace/events/dma_fence.h
> index 3963e79ca7b4..9b3875f7aa79 100644
> --- a/include/trace/events/dma_fence.h
> +++ b/include/trace/events/dma_fence.h
> @@ -83,6 +83,40 @@ DEFINE_EVENT(dma_fence, dma_fence_wait_end,
>   	TP_ARGS(fence)
>   );
>   
> +DECLARE_EVENT_CLASS(dma_fence_from,
> +
> +	TP_PROTO(struct dma_fence *fence, const char *reason),
> +
> +	TP_ARGS(fence, reason),
> +
> +	TP_STRUCT__entry(
> +		__string(driver, fence->ops->get_driver_name(fence))
> +		__string(timeline, fence->ops->get_timeline_name(fence))
> +		__field(unsigned int, context)
> +		__field(unsigned int, seqno)

Those are 64bit numbers, only recording the lower 32bits isn't enough.

> +		__string(reason, reason)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(driver, fence->ops->get_driver_name(fence));
> +		__assign_str(timeline, fence->ops->get_timeline_name(fence));
> +		__entry->context = fence->context;
> +		__entry->seqno = fence->seqno;
> +		__assign_str(reason, reason);
> +	),
> +
> +	TP_printk("driver=%s timeline=%s context=%u seqno=%u reason=%s",
> +		  __get_str(driver), __get_str(timeline), __entry->context,
> +		  __entry->seqno, __get_str(reason))
> +);
> +
> +DEFINE_EVENT(dma_fence_from, dma_fence_sync_to,

For a single event you should probably use TRACE_EVENT() instead of 
declaring a class. A class is only used if you have multiple events with 
the same parameters.

Then the name dma_fence_sync_to is not that descriptive. Maybe 
dma_fence_used_as_dependency() is better.

Then we should probably wire this up in the DRM scheduler as well. See 
functions drm_sched_job_add_dependency(), 
drm_sched_job_add_resv_dependencies() and 
drm_sched_job_add_syncobj_dependency().

Should be trivial to add the new trace point there as well.

Thanks,
Christian.

> +
> +	TP_PROTO(struct dma_fence *fence, const char *reason),
> +
> +	TP_ARGS(fence, reason)
> +);
> +
>   #endif /*  _TRACE_DMA_FENCE_H */
>   
>   /* This part must be outside protection */