| Message ID | 20211025163404.2774-1-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/trace: Hide backend specific fields behind Kconfig | expand |
On 10/25/2021 09:34, Matthew Brost wrote: > Hide the guc_id and tail fields, for request trace points, behind > CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option. Trace points > are ABI (maybe?) so don't change them without kernel developers Kconfig > options. The i915 sw arch team have previously hard blocked requests for changes to trace points from user land tool developers on the grounds that trace points are not ABI and are free to change at whim as and when the i915 internal implementation changes. They are purely for use of developers to debug the i915 driver as the i915 driver currently stands at any given instant. So I don't see how it can be argued that we must not update any trace points to allow for debugging of i915 scheduling issues on current platforms. And having to enable extra config options just to keep existing higher level trace points usable seems broken. John. > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/i915_trace.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index 9795f456cccf..4f5238d02b51 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -787,6 +787,7 @@ TRACE_EVENT(i915_request_queue, > __entry->ctx, __entry->seqno, __entry->flags) > ); > > +#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) > DECLARE_EVENT_CLASS(i915_request, > TP_PROTO(struct i915_request *rq), > TP_ARGS(rq), > @@ -816,6 +817,32 @@ DECLARE_EVENT_CLASS(i915_request, > __entry->guc_id, __entry->ctx, __entry->seqno, > __entry->tail) > ); > +#else > +DECLARE_EVENT_CLASS(i915_request, > + TP_PROTO(struct i915_request *rq), > + TP_ARGS(rq), > + > + TP_STRUCT__entry( > + __field(u32, dev) > + __field(u64, ctx) > + __field(u16, class) > + __field(u16, instance) > + __field(u32, seqno) > + ), > + > + TP_fast_assign( > + __entry->dev = rq->engine->i915->drm.primary->index; > + __entry->class = rq->engine->uabi_class; > + __entry->instance = rq->engine->uabi_instance; > + __entry->ctx = rq->fence.context; > + __entry->seqno = rq->fence.seqno; > + ), > + > + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u", > + __entry->dev, __entry->class, __entry->instance, > + __entry->ctx, __entry->seqno) > +); > +#endif > > DEFINE_EVENT(i915_request, i915_request_add, > TP_PROTO(struct i915_request *rq),
Quoting John Harrison (2021-10-26 00:06:54) > On 10/25/2021 09:34, Matthew Brost wrote: > > Hide the guc_id and tail fields, for request trace points, behind > > CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option. Trace points > > are ABI (maybe?) so don't change them without kernel developers Kconfig > > options. > The i915 sw arch team have previously hard blocked requests for changes > to trace points from user land tool developers on the grounds that trace > points are not ABI and are free to change at whim as and when the i915 > internal implementation changes. They are purely for use of developers > to debug the i915 driver as the i915 driver currently stands at any > given instant. Correct. That is indicated by the LOW_LEVEL_TRACEPOINTS. All the discussions about stable usage really revolve around the low level backend specific scheduling tracepoints to analyze hardware utilization. And those even become infeasible to expose when GuC scheduling is enabled as the information really goes to GuC log. Luckily we have added the mechanism to get the actual utilization through OA via gpuvis tool, so we don't have to guesstimate it from the KMD scheduling tracepoints (which are for KMD debugging). > So I don't see how it can be argued that we must not update any trace > points to allow for debugging of i915 scheduling issues on current > platforms. And having to enable extra config options just to keep > existing higher level trace points usable seems broken. We can update them (even outside LOW_LEVEL_TRACEPOINTS) but there should not be any backend specific data added outside the LOW_LEVEL_TRACEPOINTS, just to prevent anyone from starting to use them in some visualization/analysis tooling. If you have the energy to drive the general LKML/Linux Plumbers level discussion about tracepoint stability limbo into a conclusion, I'll be more than happy to see it resolved :) Regards, Joonas > > John. > > > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > > --- > > drivers/gpu/drm/i915/i915_trace.h | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > > index 9795f456cccf..4f5238d02b51 100644 > > --- a/drivers/gpu/drm/i915/i915_trace.h > > +++ b/drivers/gpu/drm/i915/i915_trace.h > > @@ -787,6 +787,7 @@ TRACE_EVENT(i915_request_queue, > > __entry->ctx, __entry->seqno, __entry->flags) > > ); > > > > +#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) > > DECLARE_EVENT_CLASS(i915_request, > > TP_PROTO(struct i915_request *rq), > > TP_ARGS(rq), > > @@ -816,6 +817,32 @@ DECLARE_EVENT_CLASS(i915_request, > > __entry->guc_id, __entry->ctx, __entry->seqno, > > __entry->tail) > > ); > > +#else > > +DECLARE_EVENT_CLASS(i915_request, > > + TP_PROTO(struct i915_request *rq), > > + TP_ARGS(rq), > > + > > + TP_STRUCT__entry( > > + __field(u32, dev) > > + __field(u64, ctx) > > + __field(u16, class) > > + __field(u16, instance) > > + __field(u32, seqno) > > + ), > > + > > + TP_fast_assign( > > + __entry->dev = rq->engine->i915->drm.primary->index; > > + __entry->class = rq->engine->uabi_class; > > + __entry->instance = rq->engine->uabi_instance; > > + __entry->ctx = rq->fence.context; > > + __entry->seqno = rq->fence.seqno; > > + ), > > + > > + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u", > > + __entry->dev, __entry->class, __entry->instance, > > + __entry->ctx, __entry->seqno) > > +); > > +#endif > > > > DEFINE_EVENT(i915_request, i915_request_add, > > TP_PROTO(struct i915_request *rq), >
Quoting Matthew Brost (2021-10-25 19:34:04) > Hide the guc_id and tail fields, for request trace points, behind > CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option. Trace points > are ABI (maybe?) so don't change them without kernel developers Kconfig > options. I've pushed the simple fix to eliminate the 'guc_id' field. In my opinion a separate tracepoint with more information would be a better choice here compared to mutating an existing one. The idea with LOW_LEVEL_TRACEPOINTS is to make sure there are two sets of tracepoints: one that is quasi stable and the other that is unstable. Mutating the other set when the unstable set is enabled kind of breaks that clean split. Regards, Joonas > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/i915_trace.h | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index 9795f456cccf..4f5238d02b51 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -787,6 +787,7 @@ TRACE_EVENT(i915_request_queue, > __entry->ctx, __entry->seqno, __entry->flags) > ); > > +#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) > DECLARE_EVENT_CLASS(i915_request, > TP_PROTO(struct i915_request *rq), > TP_ARGS(rq), > @@ -816,6 +817,32 @@ DECLARE_EVENT_CLASS(i915_request, > __entry->guc_id, __entry->ctx, __entry->seqno, > __entry->tail) > ); > +#else > +DECLARE_EVENT_CLASS(i915_request, > + TP_PROTO(struct i915_request *rq), > + TP_ARGS(rq), > + > + TP_STRUCT__entry( > + __field(u32, dev) > + __field(u64, ctx) > + __field(u16, class) > + __field(u16, instance) > + __field(u32, seqno) > + ), > + > + TP_fast_assign( > + __entry->dev = rq->engine->i915->drm.primary->index; > + __entry->class = rq->engine->uabi_class; > + __entry->instance = rq->engine->uabi_instance; > + __entry->ctx = rq->fence.context; > + __entry->seqno = rq->fence.seqno; > + ), > + > + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u", > + __entry->dev, __entry->class, __entry->instance, > + __entry->ctx, __entry->seqno) > +); > +#endif > > DEFINE_EVENT(i915_request, i915_request_add, > TP_PROTO(struct i915_request *rq), > -- > 2.32.0 >
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 9795f456cccf..4f5238d02b51 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -787,6 +787,7 @@ TRACE_EVENT(i915_request_queue, __entry->ctx, __entry->seqno, __entry->flags) ); +#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS) DECLARE_EVENT_CLASS(i915_request, TP_PROTO(struct i915_request *rq), TP_ARGS(rq), @@ -816,6 +817,32 @@ DECLARE_EVENT_CLASS(i915_request, __entry->guc_id, __entry->ctx, __entry->seqno, __entry->tail) ); +#else +DECLARE_EVENT_CLASS(i915_request, + TP_PROTO(struct i915_request *rq), + TP_ARGS(rq), + + TP_STRUCT__entry( + __field(u32, dev) + __field(u64, ctx) + __field(u16, class) + __field(u16, instance) + __field(u32, seqno) + ), + + TP_fast_assign( + __entry->dev = rq->engine->i915->drm.primary->index; + __entry->class = rq->engine->uabi_class; + __entry->instance = rq->engine->uabi_instance; + __entry->ctx = rq->fence.context; + __entry->seqno = rq->fence.seqno; + ), + + TP_printk("dev=%u, engine=%u:%u, ctx=%llu, seqno=%u", + __entry->dev, __entry->class, __entry->instance, + __entry->ctx, __entry->seqno) +); +#endif DEFINE_EVENT(i915_request, i915_request_add, TP_PROTO(struct i915_request *rq),
Hide the guc_id and tail fields, for request trace points, behind CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option. Trace points are ABI (maybe?) so don't change them without kernel developers Kconfig options. Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/i915_trace.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)