| Message ID | 20240213155112.156537-2-pierre-eric.pelloux-prayer@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence, drm, amdgpu new trace events | expand |
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 */
On Wed, 14 Feb 2024 13:00:16 +0100 Christian König <christian.koenig@amd.com> wrote: > > +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. FYI, TRACE_EVENT() is actually defined as: #define TRACE_EVENT(name, proto, args, tstruct, assign, print) \ DECLARE_EVENT_CLASS(name, \ PARAMS(proto), \ PARAMS(args), \ PARAMS(tstruct), \ PARAMS(assign), \ PARAMS(print)); \ DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args)); So basically, you could really just declare one TRACE_EVENT() and add DEFINE_EVENT()s on top of it ;) I never recommended that because I thought it would be confusing. -- Steve
Le 14/02/2024 à 16:10, Steven Rostedt a écrit : > On Wed, 14 Feb 2024 13:00:16 +0100 > Christian König <christian.koenig@amd.com> wrote: > >>> +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. > > FYI, TRACE_EVENT() is actually defined as: > > #define TRACE_EVENT(name, proto, args, tstruct, assign, print) \ > DECLARE_EVENT_CLASS(name, \ > PARAMS(proto), \ > PARAMS(args), \ > PARAMS(tstruct), \ > PARAMS(assign), \ > PARAMS(print)); \ > DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args)); > > So basically, you could really just declare one TRACE_EVENT() and add > DEFINE_EVENT()s on top of it ;) > > I never recommended that because I thought it would be confusing. Thanks Steve and Christian for your feedback. I'm integrating your suggestions in my branch and will re-send the series after more testing. Pierre-Eric > > -- Steve
On Tue, Feb 13, 2024 at 04:50:26PM +0100, Pierre-Eric Pelloux-Prayer wrote: > This new event can be used to trace where a given dma_fence is added > as a dependency of some other work. How? What I'd expected here is that you add a dependency chain from one fence to another, but this only has one fence. How do you figure out what's the next dma_fence that will stall on this dependency? Like in the gpu scheduler we do know what will be the fence that userspace gets back, so we can make that connection. And same for the atomic code (although you don't wire that up at all). I'm very confused on how this works and rather worried it's a brittle amdgpu-only solution ... -Sima > 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) > + __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, > + > + TP_PROTO(struct dma_fence *fence, const char *reason), > + > + TP_ARGS(fence, reason) > +); > + > #endif /* _TRACE_DMA_FENCE_H */ > > /* This part must be outside protection */ > -- > 2.40.1 >
Am 16.02.24 um 17:32 schrieb Daniel Vetter: > On Tue, Feb 13, 2024 at 04:50:26PM +0100, Pierre-Eric Pelloux-Prayer wrote: >> This new event can be used to trace where a given dma_fence is added >> as a dependency of some other work. > How? > > What I'd expected here is that you add a dependency chain from one fence > to another, but this only has one fence. That's what I though initially as well, but at the point we add the dependency fences to the scheduler job we don't have the scheduler fence initialized yet. We could change this so that we only trace all the fences after we have initialized the scheduler fence, but then we loose the information where the dependency comes from. > How do you figure out what's the > next dma_fence that will stall on this dependency? I'm not fully sure on that either. Pierre? Christian. > Like in the gpu > scheduler we do know what will be the fence that userspace gets back, so > we can make that connection. And same for the atomic code (although you > don't wire that up at all). > > I'm very confused on how this works and rather worried it's a brittle > amdgpu-only solution ... > -Sima > >> 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) >> + __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, >> + >> + TP_PROTO(struct dma_fence *fence, const char *reason), >> + >> + TP_ARGS(fence, reason) >> +); >> + >> #endif /* _TRACE_DMA_FENCE_H */ >> >> /* This part must be outside protection */ >> -- >> 2.40.1 >>
On Fri, Feb 16, 2024 at 05:51:59PM +0100, Christian König wrote: > Am 16.02.24 um 17:32 schrieb Daniel Vetter: > > On Tue, Feb 13, 2024 at 04:50:26PM +0100, Pierre-Eric Pelloux-Prayer wrote: > > > This new event can be used to trace where a given dma_fence is added > > > as a dependency of some other work. > > How? > > > > What I'd expected here is that you add a dependency chain from one fence > > to another, but this only has one fence. > > That's what I though initially as well, but at the point we add the > dependency fences to the scheduler job we don't have the scheduler fence > initialized yet. > > We could change this so that we only trace all the fences after we have > initialized the scheduler fence, but then we loose the information where the > dependency comes from. Hm right, I thought we'd dump the hashed pointe value into the fence events too, then you could make the connection. But we don't, so this is a bit annoying ... And walking the entire scheduler dependency chain at trace_dma_fence_emit time (or something similar) maybe? -Sima > > How do you figure out what's the > > next dma_fence that will stall on this dependency? > > I'm not fully sure on that either. Pierre? > > Christian. > > > > Like in the gpu > > scheduler we do know what will be the fence that userspace gets back, so > > we can make that connection. And same for the atomic code (although you > > don't wire that up at all). > > > > I'm very confused on how this works and rather worried it's a brittle > > amdgpu-only solution ... > > -Sima > > > > > 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) > > > + __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, > > > + > > > + TP_PROTO(struct dma_fence *fence, const char *reason), > > > + > > > + TP_ARGS(fence, reason) > > > +); > > > + > > > #endif /* _TRACE_DMA_FENCE_H */ > > > /* This part must be outside protection */ > > > -- > > > 2.40.1 > > > >
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) + __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, + + TP_PROTO(struct dma_fence *fence, const char *reason), + + TP_ARGS(fence, reason) +); + #endif /* _TRACE_DMA_FENCE_H */ /* This part must be outside protection */
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(+)