| Message ID | 20180524150447.16927-2-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Tvrtko Ursulin (2018-05-24 16:04:47) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > In the string tracepoint representation we ended up with the engine > sandwiched between context hardware id and context fence id. > > Move the two pieces of context data together and consolidate for > redability using the format of ctx=hw_id:fence_context_id. Grr, bring backs my memory of ctx being the most important! In order of segregation, I think of it as device+context as being the outer container (a user isn't meant to escape their context). The timelines and engines they use are all contained within their context. But what we call ctx here isn't really context, but timeline; how about if we switch to the fence=%llx:%d representation we've mostly settled on for the debug messages? -Chris
On 24/05/2018 16:33, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-05-24 16:04:47) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> In the string tracepoint representation we ended up with the engine >> sandwiched between context hardware id and context fence id. >> >> Move the two pieces of context data together and consolidate for >> redability using the format of ctx=hw_id:fence_context_id. > > Grr, bring backs my memory of ctx being the most important! I knew it! :)) > In order of segregation, I think of it as device+context as being the > outer container (a user isn't meant to escape their context). The > timelines and engines they use are all contained within their context. If I move ctx before engine, then seqno is left to hang after the engine. And honestly I forgot all my arguments in this topic. By the virtue of exhaustion I am prepared to give in. :)) > But what we call ctx here isn't really context, but timeline; how about > if we switch to the fence=%llx:%d representation we've mostly settled on > for the debug messages? For the ctx and seqno pair? But here we have the additional issue of hw_id. I think context is better than timeline at this level. Or you mean keep explicit hw_id and join ctx and seqno into fence=%llx:%d? Regards, Tvrtko
On 24/05/18 16:04, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > In the string tracepoint representation we ended up with the engine > sandwiched between context hardware id and context fence id. > > Move the two pieces of context data together and consolidate for > redability using the format of ctx=hw_id:fence_context_id. Arg! Will need to update the tracepoint parser in igt :( > > Binary records are left as is, that is both fields remaing under the > existing name and ordering. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_trace.h | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h > index 3465cc1f9345..ab67b1661de4 100644 > --- a/drivers/gpu/drm/i915/i915_trace.h > +++ b/drivers/gpu/drm/i915/i915_trace.h > @@ -638,9 +638,9 @@ TRACE_EVENT(i915_request_queue, > __entry->flags = flags; > ), > > - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, flags=0x%x", > - __entry->dev, __entry->hw_id, __entry->class, > - __entry->instance, __entry->ctx, __entry->seqno, > + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, flags=0x%x", > + __entry->dev, __entry->class, __entry->instance, > + __entry->hw_id, __entry->ctx, __entry->seqno, > __entry->flags) > ); > > @@ -668,9 +668,9 @@ DECLARE_EVENT_CLASS(i915_request, > __entry->global = rq->global_seqno; > ), > > - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, global=%u", > - __entry->dev, __entry->hw_id, __entry->class, > - __entry->instance, __entry->ctx, __entry->seqno, > + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, global=%u", > + __entry->dev, __entry->class, __entry->instance, > + __entry->hw_id, __entry->ctx, __entry->seqno, > __entry->global) > ); > > @@ -718,9 +718,9 @@ TRACE_EVENT(i915_request_in, > __entry->port = port; > ), > > - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, prio=%u, global=%u, port=%u", > - __entry->dev, __entry->hw_id, __entry->class, > - __entry->instance, __entry->ctx, __entry->seqno, > + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, prio=%u, global=%u, port=%u", > + __entry->dev, __entry->class, __entry->instance, > + __entry->hw_id, __entry->ctx, __entry->seqno, > __entry->prio, __entry->global_seqno, __entry->port) > ); > > @@ -750,9 +750,9 @@ TRACE_EVENT(i915_request_out, > __entry->completed = i915_request_completed(rq); > ), > > - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, global=%u, completed?=%u", > - __entry->dev, __entry->hw_id, __entry->class, > - __entry->instance, __entry->ctx, __entry->seqno, > + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, global=%u, completed?=%u", > + __entry->dev, __entry->class, __entry->instance, > + __entry->hw_id, __entry->ctx, __entry->seqno, > __entry->global_seqno, __entry->completed) > ); > > @@ -842,9 +842,9 @@ TRACE_EVENT(i915_request_wait_begin, > __entry->flags = flags; > ), > > - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, global=%u, blocking=%u, flags=0x%x", > - __entry->dev, __entry->hw_id, __entry->class, > - __entry->instance, __entry->ctx, __entry->seqno, > + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, global=%u, blocking=%u, flags=0x%x", > + __entry->dev, __entry->class, __entry->instance, > + __entry->hw_id, __entry->ctx, __entry->seqno, > __entry->global, !!(__entry->flags & I915_WAIT_LOCKED), > __entry->flags) > );
On 24/05/2018 16:48, Lionel Landwerlin wrote: > On 24/05/18 16:04, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> In the string tracepoint representation we ended up with the engine >> sandwiched between context hardware id and context fence id. >> >> Move the two pieces of context data together and consolidate for >> redability using the format of ctx=hw_id:fence_context_id. > > Arg! Will need to update the tracepoint parser in igt :( I'll leave them separate then, was mostly aiming to remove engine out of the sandwich. Regards, Tvrtko >> >> Binary records are left as is, that is both fields remaing under the >> existing name and ordering. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_trace.h | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_trace.h >> b/drivers/gpu/drm/i915/i915_trace.h >> index 3465cc1f9345..ab67b1661de4 100644 >> --- a/drivers/gpu/drm/i915/i915_trace.h >> +++ b/drivers/gpu/drm/i915/i915_trace.h >> @@ -638,9 +638,9 @@ TRACE_EVENT(i915_request_queue, >> __entry->flags = flags; >> ), >> - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, >> flags=0x%x", >> - __entry->dev, __entry->hw_id, __entry->class, >> - __entry->instance, __entry->ctx, __entry->seqno, >> + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, >> flags=0x%x", >> + __entry->dev, __entry->class, __entry->instance, >> + __entry->hw_id, __entry->ctx, __entry->seqno, >> __entry->flags) >> ); >> @@ -668,9 +668,9 @@ DECLARE_EVENT_CLASS(i915_request, >> __entry->global = rq->global_seqno; >> ), >> - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, >> global=%u", >> - __entry->dev, __entry->hw_id, __entry->class, >> - __entry->instance, __entry->ctx, __entry->seqno, >> + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, >> global=%u", >> + __entry->dev, __entry->class, __entry->instance, >> + __entry->hw_id, __entry->ctx, __entry->seqno, >> __entry->global) >> ); >> @@ -718,9 +718,9 @@ TRACE_EVENT(i915_request_in, >> __entry->port = port; >> ), >> - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, >> prio=%u, global=%u, port=%u", >> - __entry->dev, __entry->hw_id, __entry->class, >> - __entry->instance, __entry->ctx, __entry->seqno, >> + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, >> prio=%u, global=%u, port=%u", >> + __entry->dev, __entry->class, __entry->instance, >> + __entry->hw_id, __entry->ctx, __entry->seqno, >> __entry->prio, __entry->global_seqno, __entry->port) >> ); >> @@ -750,9 +750,9 @@ TRACE_EVENT(i915_request_out, >> __entry->completed = i915_request_completed(rq); >> ), >> - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, >> seqno=%u, global=%u, completed?=%u", >> - __entry->dev, __entry->hw_id, __entry->class, >> - __entry->instance, __entry->ctx, __entry->seqno, >> + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, >> global=%u, completed?=%u", >> + __entry->dev, __entry->class, __entry->instance, >> + __entry->hw_id, __entry->ctx, __entry->seqno, >> __entry->global_seqno, __entry->completed) >> ); >> @@ -842,9 +842,9 @@ TRACE_EVENT(i915_request_wait_begin, >> __entry->flags = flags; >> ), >> - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, >> global=%u, blocking=%u, flags=0x%x", >> - __entry->dev, __entry->hw_id, __entry->class, >> - __entry->instance, __entry->ctx, __entry->seqno, >> + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, >> global=%u, blocking=%u, flags=0x%x", >> + __entry->dev, __entry->class, __entry->instance, >> + __entry->hw_id, __entry->ctx, __entry->seqno, >> __entry->global, !!(__entry->flags & I915_WAIT_LOCKED), >> __entry->flags) >> ); > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Tvrtko Ursulin (2018-05-24 16:48:45) > > On 24/05/2018 16:33, Chris Wilson wrote: > > But what we call ctx here isn't really context, but timeline; how about > > if we switch to the fence=%llx:%d representation we've mostly settled on > > for the debug messages? > > For the ctx and seqno pair? But here we have the additional issue of > hw_id. I think context is better than timeline at this level. > > Or you mean keep explicit hw_id and join ctx and seqno into fence=%llx:%d? Right. I think what we call ctx here is very confusing, as it's just the fence.context (i.e timeline id) and not any of the ids we assign to the context (neither hw_id or uabi_id), so I don't think ctx refers to i915_gem_context/intel_context at all and so would rather stop using 'ctx'. Pardon the rambling, -Chris
On 24/05/2018 17:00, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-05-24 16:48:45) >> >> On 24/05/2018 16:33, Chris Wilson wrote: >>> But what we call ctx here isn't really context, but timeline; how about >>> if we switch to the fence=%llx:%d representation we've mostly settled on >>> for the debug messages? >> >> For the ctx and seqno pair? But here we have the additional issue of >> hw_id. I think context is better than timeline at this level. >> >> Or you mean keep explicit hw_id and join ctx and seqno into fence=%llx:%d? > > Right. I think what we call ctx here is very confusing, as it's just the > fence.context (i.e timeline id) and not any of the ids we assign to the > context (neither hw_id or uabi_id), so I don't think ctx refers to > i915_gem_context/intel_context at all and so would rather stop using > 'ctx'. I couldn't make myself re-order ctx and engine, since I did not find the solution for the resulting ctx and seqno split. And I did not like the fence=%llu:%u for tracepoints. Just can't think of requests as fences. Wrt hw_id and dev moving to u16, both fields theoretically can be wider so I did not do that either. What's left is class:instance, group two context id's together, and expand ctx to u64. Hopefully not controversial at this time, or as a first step, or something. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-05-25 09:30:29) > > On 24/05/2018 17:00, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-05-24 16:48:45) > >> > >> On 24/05/2018 16:33, Chris Wilson wrote: > >>> But what we call ctx here isn't really context, but timeline; how about > >>> if we switch to the fence=%llx:%d representation we've mostly settled on > >>> for the debug messages? > >> > >> For the ctx and seqno pair? But here we have the additional issue of > >> hw_id. I think context is better than timeline at this level. > >> > >> Or you mean keep explicit hw_id and join ctx and seqno into fence=%llx:%d? > > > > Right. I think what we call ctx here is very confusing, as it's just the > > fence.context (i.e timeline id) and not any of the ids we assign to the > > context (neither hw_id or uabi_id), so I don't think ctx refers to > > i915_gem_context/intel_context at all and so would rather stop using > > 'ctx'. > > I couldn't make myself re-order ctx and engine, since I did not find the > solution for the resulting ctx and seqno split. And I did not like the > fence=%llu:%u for tracepoints. Just can't think of requests as fences. But we are referring to the fence id of the request (rq->fence.context, rq->fence.seqno), not its global seqno. > Wrt hw_id and dev moving to u16, both fields theoretically can be wider > so I did not do that either. dev cannot. We are limited to 16 DRM devices on the system, so minor is always 0-15. Immaterial if we need padding between members in the struct, we might as well use up the padding. -Chris
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 3465cc1f9345..ab67b1661de4 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -638,9 +638,9 @@ TRACE_EVENT(i915_request_queue, __entry->flags = flags; ), - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, flags=0x%x", - __entry->dev, __entry->hw_id, __entry->class, - __entry->instance, __entry->ctx, __entry->seqno, + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, flags=0x%x", + __entry->dev, __entry->class, __entry->instance, + __entry->hw_id, __entry->ctx, __entry->seqno, __entry->flags) ); @@ -668,9 +668,9 @@ DECLARE_EVENT_CLASS(i915_request, __entry->global = rq->global_seqno; ), - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, global=%u", - __entry->dev, __entry->hw_id, __entry->class, - __entry->instance, __entry->ctx, __entry->seqno, + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, global=%u", + __entry->dev, __entry->class, __entry->instance, + __entry->hw_id, __entry->ctx, __entry->seqno, __entry->global) ); @@ -718,9 +718,9 @@ TRACE_EVENT(i915_request_in, __entry->port = port; ), - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, prio=%u, global=%u, port=%u", - __entry->dev, __entry->hw_id, __entry->class, - __entry->instance, __entry->ctx, __entry->seqno, + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, prio=%u, global=%u, port=%u", + __entry->dev, __entry->class, __entry->instance, + __entry->hw_id, __entry->ctx, __entry->seqno, __entry->prio, __entry->global_seqno, __entry->port) ); @@ -750,9 +750,9 @@ TRACE_EVENT(i915_request_out, __entry->completed = i915_request_completed(rq); ), - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, global=%u, completed?=%u", - __entry->dev, __entry->hw_id, __entry->class, - __entry->instance, __entry->ctx, __entry->seqno, + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, global=%u, completed?=%u", + __entry->dev, __entry->class, __entry->instance, + __entry->hw_id, __entry->ctx, __entry->seqno, __entry->global_seqno, __entry->completed) ); @@ -842,9 +842,9 @@ TRACE_EVENT(i915_request_wait_begin, __entry->flags = flags; ), - TP_printk("dev=%u, hw_id=%u, engine=%u:%u, ctx=%u, seqno=%u, global=%u, blocking=%u, flags=0x%x", - __entry->dev, __entry->hw_id, __entry->class, - __entry->instance, __entry->ctx, __entry->seqno, + TP_printk("dev=%u, engine=%u:%u, ctx=%u:%u, seqno=%u, global=%u, blocking=%u, flags=0x%x", + __entry->dev, __entry->class, __entry->instance, + __entry->hw_id, __entry->ctx, __entry->seqno, __entry->global, !!(__entry->flags & I915_WAIT_LOCKED), __entry->flags) );