[PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Instead of using the engine->id, use uabi_class:instance pairs in trace-
points including engine info.

This will be more readable, more future proof and more stable for
userspace consumption.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: svetlana.kukanova@intel.com
---
 drivers/gpu/drm/i915/i915_trace.h | 107 ++++++++++++++++++------------
 1 file changed, 65 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 5d4f78765083..3465cc1f9345 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -591,21 +591,26 @@ TRACE_EVENT(i915_gem_ring_sync_to,
 
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
-			     __field(u32, sync_from)
-			     __field(u32, sync_to)
+			     __field(u32, from_class)
+			     __field(u32, from_instance)
+			     __field(u32, to_class)
+			     __field(u32, to_instance)
 			     __field(u32, seqno)
 			     ),
 
 	    TP_fast_assign(
 			   __entry->dev = from->i915->drm.primary->index;
-			   __entry->sync_from = from->engine->id;
-			   __entry->sync_to = to->engine->id;
+			   __entry->from_class = from->engine->uabi_class;
+			   __entry->from_instance = from->engine->instance;
+			   __entry->to_class = to->engine->uabi_class;
+			   __entry->to_instance = to->engine->instance;
 			   __entry->seqno = from->global_seqno;
 			   ),
 
-	    TP_printk("dev=%u, sync-from=%u, sync-to=%u, seqno=%u",
+	    TP_printk("dev=%u, sync-from=%u:%u, sync-to=%u:%u, seqno=%u",
 		      __entry->dev,
-		      __entry->sync_from, __entry->sync_to,
+		      __entry->from_class, __entry->from_instance,
+		      __entry->to_class, __entry->to_instance,
 		      __entry->seqno)
 );
 
@@ -616,7 +621,8 @@ TRACE_EVENT(i915_request_queue,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, hw_id)
-			     __field(u32, ring)
+			     __field(u32, class)
+			     __field(u32, instance)
 			     __field(u32, ctx)
 			     __field(u32, seqno)
 			     __field(u32, flags)
@@ -625,15 +631,17 @@ TRACE_EVENT(i915_request_queue,
 	    TP_fast_assign(
 			   __entry->dev = rq->i915->drm.primary->index;
 			   __entry->hw_id = rq->gem_context->hw_id;
-			   __entry->ring = rq->engine->id;
+			   __entry->class = rq->engine->uabi_class;
+			   __entry->instance = rq->engine->instance;
 			   __entry->ctx = rq->fence.context;
 			   __entry->seqno = rq->fence.seqno;
 			   __entry->flags = flags;
 			   ),
 
-	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, flags=0x%x",
-		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
-		      __entry->seqno, __entry->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,
+		      __entry->flags)
 );
 
 DECLARE_EVENT_CLASS(i915_request,
@@ -643,7 +651,8 @@ DECLARE_EVENT_CLASS(i915_request,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, hw_id)
-			     __field(u32, ring)
+			     __field(u32, class)
+			     __field(u32, instance)
 			     __field(u32, ctx)
 			     __field(u32, seqno)
 			     __field(u32, global)
@@ -652,15 +661,17 @@ DECLARE_EVENT_CLASS(i915_request,
 	    TP_fast_assign(
 			   __entry->dev = rq->i915->drm.primary->index;
 			   __entry->hw_id = rq->gem_context->hw_id;
-			   __entry->ring = rq->engine->id;
+			   __entry->class = rq->engine->uabi_class;
+			   __entry->instance = rq->engine->instance;
 			   __entry->ctx = rq->fence.context;
 			   __entry->seqno = rq->fence.seqno;
 			   __entry->global = rq->global_seqno;
 			   ),
 
-	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, global=%u",
-		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
-		      __entry->seqno, __entry->global)
+	    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,
+		      __entry->global)
 );
 
 DEFINE_EVENT(i915_request, i915_request_add,
@@ -686,7 +697,8 @@ TRACE_EVENT(i915_request_in,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, hw_id)
-			     __field(u32, ring)
+			     __field(u32, class)
+			     __field(u32, instance)
 			     __field(u32, ctx)
 			     __field(u32, seqno)
 			     __field(u32, global_seqno)
@@ -697,7 +709,8 @@ TRACE_EVENT(i915_request_in,
 	    TP_fast_assign(
 			   __entry->dev = rq->i915->drm.primary->index;
 			   __entry->hw_id = rq->gem_context->hw_id;
-			   __entry->ring = rq->engine->id;
+			   __entry->class = rq->engine->uabi_class;
+			   __entry->instance = rq->engine->instance;
 			   __entry->ctx = rq->fence.context;
 			   __entry->seqno = rq->fence.seqno;
 			   __entry->global_seqno = rq->global_seqno;
@@ -705,10 +718,10 @@ TRACE_EVENT(i915_request_in,
 			   __entry->port = port;
 			   ),
 
-	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, prio=%u, global=%u, port=%u",
-		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
-		      __entry->seqno, __entry->prio, __entry->global_seqno,
-		      __entry->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,
+		      __entry->prio, __entry->global_seqno, __entry->port)
 );
 
 TRACE_EVENT(i915_request_out,
@@ -718,7 +731,8 @@ TRACE_EVENT(i915_request_out,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, hw_id)
-			     __field(u32, ring)
+			     __field(u32, class)
+			     __field(u32, instance)
 			     __field(u32, ctx)
 			     __field(u32, seqno)
 			     __field(u32, global_seqno)
@@ -728,16 +742,17 @@ TRACE_EVENT(i915_request_out,
 	    TP_fast_assign(
 			   __entry->dev = rq->i915->drm.primary->index;
 			   __entry->hw_id = rq->gem_context->hw_id;
-			   __entry->ring = rq->engine->id;
+			   __entry->class = rq->engine->uabi_class;
+			   __entry->instance = rq->engine->instance;
 			   __entry->ctx = rq->fence.context;
 			   __entry->seqno = rq->fence.seqno;
 			   __entry->global_seqno = rq->global_seqno;
 			   __entry->completed = i915_request_completed(rq);
 			   ),
 
-		    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, global=%u, completed?=%u",
-			      __entry->dev, __entry->hw_id, __entry->ring,
-			      __entry->ctx, __entry->seqno,
+		    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,
 			      __entry->global_seqno, __entry->completed)
 );
 
@@ -771,21 +786,23 @@ TRACE_EVENT(intel_engine_notify,
 
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
-			     __field(u32, ring)
+			     __field(u32, class)
+			     __field(u32, instance)
 			     __field(u32, seqno)
 			     __field(bool, waiters)
 			     ),
 
 	    TP_fast_assign(
 			   __entry->dev = engine->i915->drm.primary->index;
-			   __entry->ring = engine->id;
+			   __entry->class = engine->uabi_class;
+			   __entry->instance = engine->instance;
 			   __entry->seqno = intel_engine_get_seqno(engine);
 			   __entry->waiters = waiters;
 			   ),
 
-	    TP_printk("dev=%u, ring=%u, seqno=%u, waiters=%u",
-		      __entry->dev, __entry->ring, __entry->seqno,
-		      __entry->waiters)
+	    TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u",
+		      __entry->dev, __entry->class, __entry->instance,
+		      __entry->seqno, __entry->waiters)
 );
 
 DEFINE_EVENT(i915_request, i915_request_retire,
@@ -800,7 +817,8 @@ TRACE_EVENT(i915_request_wait_begin,
 	    TP_STRUCT__entry(
 			     __field(u32, dev)
 			     __field(u32, hw_id)
-			     __field(u32, ring)
+			     __field(u32, class)
+			     __field(u32, instance)
 			     __field(u32, ctx)
 			     __field(u32, seqno)
 			     __field(u32, global)
@@ -816,17 +834,19 @@ TRACE_EVENT(i915_request_wait_begin,
 	    TP_fast_assign(
 			   __entry->dev = rq->i915->drm.primary->index;
 			   __entry->hw_id = rq->gem_context->hw_id;
-			   __entry->ring = rq->engine->id;
+			   __entry->class = rq->engine->uabi_class;
+			   __entry->instance = rq->engine->instance;
 			   __entry->ctx = rq->fence.context;
 			   __entry->seqno = rq->fence.seqno;
 			   __entry->global = rq->global_seqno;
 			   __entry->flags = flags;
 			   ),
 
-	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, global=%u, blocking=%u, flags=0x%x",
-		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
-		      __entry->seqno, __entry->global,
-		      !!(__entry->flags & I915_WAIT_LOCKED), __entry->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,
+		      __entry->global, !!(__entry->flags & I915_WAIT_LOCKED),
+		      __entry->flags)
 );
 
 DEFINE_EVENT(i915_request, i915_request_wait_end,
@@ -966,21 +986,24 @@ TRACE_EVENT(switch_mm,
 	TP_ARGS(engine, to),
 
 	TP_STRUCT__entry(
-			__field(u32, ring)
+			__field(u32, class)
+			__field(u32, instance)
 			__field(struct i915_gem_context *, to)
 			__field(struct i915_address_space *, vm)
 			__field(u32, dev)
 	),
 
 	TP_fast_assign(
-			__entry->ring = engine->id;
+			__entry->class = engine->uabi_class;
+			__entry->instance = engine->instance;
 			__entry->to = to;
 			__entry->vm = to->ppgtt? &to->ppgtt->base : NULL;
 			__entry->dev = engine->i915->drm.primary->index;
 	),
 
-	TP_printk("dev=%u, ring=%u, ctx=%p, ctx_vm=%p",
-		  __entry->dev, __entry->ring, __entry->to, __entry->vm)
+	TP_printk("dev=%u, engine=%u:%u, ctx=%p, ctx_vm=%p",
+		  __entry->dev, __entry->class, __entry->instance, __entry->to,
+		  __entry->vm)
 );
 
 #endif /* _I915_TRACE_H_ */
-- 
2.17.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Tvrtko Ursulin]

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.

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)
 );
-- 
2.17.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-05-24 16:04:46)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Instead of using the engine->id, use uabi_class:instance pairs in trace-
> points including engine info.

Should we not pack dev,hw_id,class,instance into u16s?

> This will be more readable, more future proof and more stable for
> userspace consumption.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: svetlana.kukanova@intel.com
> ---
> @@ -616,7 +621,8 @@ TRACE_EVENT(i915_request_queue,
>             TP_STRUCT__entry(
>                              __field(u32, dev)
>                              __field(u32, hw_id)
> -                            __field(u32, ring)
> +                            __field(u32, class)
> +                            __field(u32, instance)
>                              __field(u32, ctx)

ctx needs u64 :(

I've no objection to switching to our hopefully futureproof uabi
nomenclature.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Tvrtko Ursulin]

On 24/05/2018 16:29, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-05-24 16:04:46)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Instead of using the engine->id, use uabi_class:instance pairs in trace-
>> points including engine info.
> 
> Should we not pack dev,hw_id,class,instance into u16s?

Can do.

>> This will be more readable, more future proof and more stable for
>> userspace consumption.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: svetlana.kukanova@intel.com
>> ---
>> @@ -616,7 +621,8 @@ TRACE_EVENT(i915_request_queue,
>>              TP_STRUCT__entry(
>>                               __field(u32, dev)
>>                               __field(u32, hw_id)
>> -                            __field(u32, ring)
>> +                            __field(u32, class)
>> +                            __field(u32, instance)
>>                               __field(u32, ctx)
> 
> ctx needs u64 :(
> I've no objection to switching to our hopefully futureproof uabi
> nomenclature.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks, I'll grow the series with all of the above.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Lionel Landwerlin]

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)
>   );


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Lionel Landwerlin]

On 24/05/18 16:04, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Instead of using the engine->id, use uabi_class:instance pairs in trace-
> points including engine info.
>
> This will be more readable, more future proof and more stable for
> userspace consumption.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: svetlana.kukanova@intel.com
Don't you want engine->uabi_id instead of engine->instance ?
> ---
>   drivers/gpu/drm/i915/i915_trace.h | 107 ++++++++++++++++++------------
>   1 file changed, 65 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 5d4f78765083..3465cc1f9345 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -591,21 +591,26 @@ TRACE_EVENT(i915_gem_ring_sync_to,
>   
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
> -			     __field(u32, sync_from)
> -			     __field(u32, sync_to)
> +			     __field(u32, from_class)
> +			     __field(u32, from_instance)
> +			     __field(u32, to_class)
> +			     __field(u32, to_instance)
>   			     __field(u32, seqno)
>   			     ),
>   
>   	    TP_fast_assign(
>   			   __entry->dev = from->i915->drm.primary->index;
> -			   __entry->sync_from = from->engine->id;
> -			   __entry->sync_to = to->engine->id;
> +			   __entry->from_class = from->engine->uabi_class;
> +			   __entry->from_instance = from->engine->instance;
> +			   __entry->to_class = to->engine->uabi_class;
> +			   __entry->to_instance = to->engine->instance;
>   			   __entry->seqno = from->global_seqno;
>   			   ),
>   
> -	    TP_printk("dev=%u, sync-from=%u, sync-to=%u, seqno=%u",
> +	    TP_printk("dev=%u, sync-from=%u:%u, sync-to=%u:%u, seqno=%u",
>   		      __entry->dev,
> -		      __entry->sync_from, __entry->sync_to,
> +		      __entry->from_class, __entry->from_instance,
> +		      __entry->to_class, __entry->to_instance,
>   		      __entry->seqno)
>   );
>   
> @@ -616,7 +621,8 @@ TRACE_EVENT(i915_request_queue,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, hw_id)
> -			     __field(u32, ring)
> +			     __field(u32, class)
> +			     __field(u32, instance)
>   			     __field(u32, ctx)
>   			     __field(u32, seqno)
>   			     __field(u32, flags)
> @@ -625,15 +631,17 @@ TRACE_EVENT(i915_request_queue,
>   	    TP_fast_assign(
>   			   __entry->dev = rq->i915->drm.primary->index;
>   			   __entry->hw_id = rq->gem_context->hw_id;
> -			   __entry->ring = rq->engine->id;
> +			   __entry->class = rq->engine->uabi_class;
> +			   __entry->instance = rq->engine->instance;
>   			   __entry->ctx = rq->fence.context;
>   			   __entry->seqno = rq->fence.seqno;
>   			   __entry->flags = flags;
>   			   ),
>   
> -	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, flags=0x%x",
> -		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
> -		      __entry->seqno, __entry->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,
> +		      __entry->flags)
>   );
>   
>   DECLARE_EVENT_CLASS(i915_request,
> @@ -643,7 +651,8 @@ DECLARE_EVENT_CLASS(i915_request,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, hw_id)
> -			     __field(u32, ring)
> +			     __field(u32, class)
> +			     __field(u32, instance)
>   			     __field(u32, ctx)
>   			     __field(u32, seqno)
>   			     __field(u32, global)
> @@ -652,15 +661,17 @@ DECLARE_EVENT_CLASS(i915_request,
>   	    TP_fast_assign(
>   			   __entry->dev = rq->i915->drm.primary->index;
>   			   __entry->hw_id = rq->gem_context->hw_id;
> -			   __entry->ring = rq->engine->id;
> +			   __entry->class = rq->engine->uabi_class;
> +			   __entry->instance = rq->engine->instance;
>   			   __entry->ctx = rq->fence.context;
>   			   __entry->seqno = rq->fence.seqno;
>   			   __entry->global = rq->global_seqno;
>   			   ),
>   
> -	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, global=%u",
> -		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
> -		      __entry->seqno, __entry->global)
> +	    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,
> +		      __entry->global)
>   );
>   
>   DEFINE_EVENT(i915_request, i915_request_add,
> @@ -686,7 +697,8 @@ TRACE_EVENT(i915_request_in,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, hw_id)
> -			     __field(u32, ring)
> +			     __field(u32, class)
> +			     __field(u32, instance)
>   			     __field(u32, ctx)
>   			     __field(u32, seqno)
>   			     __field(u32, global_seqno)
> @@ -697,7 +709,8 @@ TRACE_EVENT(i915_request_in,
>   	    TP_fast_assign(
>   			   __entry->dev = rq->i915->drm.primary->index;
>   			   __entry->hw_id = rq->gem_context->hw_id;
> -			   __entry->ring = rq->engine->id;
> +			   __entry->class = rq->engine->uabi_class;
> +			   __entry->instance = rq->engine->instance;
>   			   __entry->ctx = rq->fence.context;
>   			   __entry->seqno = rq->fence.seqno;
>   			   __entry->global_seqno = rq->global_seqno;
> @@ -705,10 +718,10 @@ TRACE_EVENT(i915_request_in,
>   			   __entry->port = port;
>   			   ),
>   
> -	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, prio=%u, global=%u, port=%u",
> -		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
> -		      __entry->seqno, __entry->prio, __entry->global_seqno,
> -		      __entry->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,
> +		      __entry->prio, __entry->global_seqno, __entry->port)
>   );
>   
>   TRACE_EVENT(i915_request_out,
> @@ -718,7 +731,8 @@ TRACE_EVENT(i915_request_out,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, hw_id)
> -			     __field(u32, ring)
> +			     __field(u32, class)
> +			     __field(u32, instance)
>   			     __field(u32, ctx)
>   			     __field(u32, seqno)
>   			     __field(u32, global_seqno)
> @@ -728,16 +742,17 @@ TRACE_EVENT(i915_request_out,
>   	    TP_fast_assign(
>   			   __entry->dev = rq->i915->drm.primary->index;
>   			   __entry->hw_id = rq->gem_context->hw_id;
> -			   __entry->ring = rq->engine->id;
> +			   __entry->class = rq->engine->uabi_class;
> +			   __entry->instance = rq->engine->instance;
>   			   __entry->ctx = rq->fence.context;
>   			   __entry->seqno = rq->fence.seqno;
>   			   __entry->global_seqno = rq->global_seqno;
>   			   __entry->completed = i915_request_completed(rq);
>   			   ),
>   
> -		    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, global=%u, completed?=%u",
> -			      __entry->dev, __entry->hw_id, __entry->ring,
> -			      __entry->ctx, __entry->seqno,
> +		    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,
>   			      __entry->global_seqno, __entry->completed)
>   );
>   
> @@ -771,21 +786,23 @@ TRACE_EVENT(intel_engine_notify,
>   
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
> -			     __field(u32, ring)
> +			     __field(u32, class)
> +			     __field(u32, instance)
>   			     __field(u32, seqno)
>   			     __field(bool, waiters)
>   			     ),
>   
>   	    TP_fast_assign(
>   			   __entry->dev = engine->i915->drm.primary->index;
> -			   __entry->ring = engine->id;
> +			   __entry->class = engine->uabi_class;
> +			   __entry->instance = engine->instance;
>   			   __entry->seqno = intel_engine_get_seqno(engine);
>   			   __entry->waiters = waiters;
>   			   ),
>   
> -	    TP_printk("dev=%u, ring=%u, seqno=%u, waiters=%u",
> -		      __entry->dev, __entry->ring, __entry->seqno,
> -		      __entry->waiters)
> +	    TP_printk("dev=%u, engine=%u:%u, seqno=%u, waiters=%u",
> +		      __entry->dev, __entry->class, __entry->instance,
> +		      __entry->seqno, __entry->waiters)
>   );
>   
>   DEFINE_EVENT(i915_request, i915_request_retire,
> @@ -800,7 +817,8 @@ TRACE_EVENT(i915_request_wait_begin,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, hw_id)
> -			     __field(u32, ring)
> +			     __field(u32, class)
> +			     __field(u32, instance)
>   			     __field(u32, ctx)
>   			     __field(u32, seqno)
>   			     __field(u32, global)
> @@ -816,17 +834,19 @@ TRACE_EVENT(i915_request_wait_begin,
>   	    TP_fast_assign(
>   			   __entry->dev = rq->i915->drm.primary->index;
>   			   __entry->hw_id = rq->gem_context->hw_id;
> -			   __entry->ring = rq->engine->id;
> +			   __entry->class = rq->engine->uabi_class;
> +			   __entry->instance = rq->engine->instance;
>   			   __entry->ctx = rq->fence.context;
>   			   __entry->seqno = rq->fence.seqno;
>   			   __entry->global = rq->global_seqno;
>   			   __entry->flags = flags;
>   			   ),
>   
> -	    TP_printk("dev=%u, hw_id=%u, ring=%u, ctx=%u, seqno=%u, global=%u, blocking=%u, flags=0x%x",
> -		      __entry->dev, __entry->hw_id, __entry->ring, __entry->ctx,
> -		      __entry->seqno, __entry->global,
> -		      !!(__entry->flags & I915_WAIT_LOCKED), __entry->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,
> +		      __entry->global, !!(__entry->flags & I915_WAIT_LOCKED),
> +		      __entry->flags)
>   );
>   
>   DEFINE_EVENT(i915_request, i915_request_wait_end,
> @@ -966,21 +986,24 @@ TRACE_EVENT(switch_mm,
>   	TP_ARGS(engine, to),
>   
>   	TP_STRUCT__entry(
> -			__field(u32, ring)
> +			__field(u32, class)
> +			__field(u32, instance)
>   			__field(struct i915_gem_context *, to)
>   			__field(struct i915_address_space *, vm)
>   			__field(u32, dev)
>   	),
>   
>   	TP_fast_assign(
> -			__entry->ring = engine->id;
> +			__entry->class = engine->uabi_class;
> +			__entry->instance = engine->instance;
>   			__entry->to = to;
>   			__entry->vm = to->ppgtt? &to->ppgtt->base : NULL;
>   			__entry->dev = engine->i915->drm.primary->index;
>   	),
>   
> -	TP_printk("dev=%u, ring=%u, ctx=%p, ctx_vm=%p",
> -		  __entry->dev, __entry->ring, __entry->to, __entry->vm)
> +	TP_printk("dev=%u, engine=%u:%u, ctx=%p, ctx_vm=%p",
> +		  __entry->dev, __entry->class, __entry->instance, __entry->to,
> +		  __entry->vm)
>   );
>   
>   #endif /* _I915_TRACE_H_ */


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/trace: Describe engines as class:instance pairs

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/trace: Describe engines as class:instance pairs
URL   : https://patchwork.freedesktop.org/series/43709/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
29e13710369f drm/i915/trace: Describe engines as class:instance pairs
4f5293dac612 drm/i915/trace: Remove engine out of the context sandwich
-:31: ERROR:CODE_INDENT: code indent should use tabs where possible
#31: FILE: drivers/gpu/drm/i915/i915_trace.h:643:
+^I              __entry->hw_id, __entry->ctx, __entry->seqno,$

total: 1 errors, 0 warnings, 0 checks, 60 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Tvrtko Ursulin]

On 24/05/2018 16:53, Lionel Landwerlin wrote:
> On 24/05/18 16:04, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Instead of using the engine->id, use uabi_class:instance pairs in trace-
>> points including engine info.
>>
>> This will be more readable, more future proof and more stable for
>> userspace consumption.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: svetlana.kukanova@intel.com
> Don't you want engine->uabi_id instead of engine->instance ?

No, class:instance is the new engine identifier - why do you think we 
would need legacy engine->uabi_id?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Lionel Landwerlin]

On 24/05/18 17:07, Tvrtko Ursulin wrote:
>
> On 24/05/2018 16:53, Lionel Landwerlin wrote:
>> On 24/05/18 16:04, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Instead of using the engine->id, use uabi_class:instance pairs in 
>>> trace-
>>> points including engine info.
>>>
>>> This will be more readable, more future proof and more stable for
>>> userspace consumption.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: svetlana.kukanova@intel.com
>> Don't you want engine->uabi_id instead of engine->instance ?
>
> No, class:instance is the new engine identifier - why do you think we 
> would need legacy engine->uabi_id?

Maybe I forgot about your engine listing series...
I would expect the tracepoint to match the engines listed through that uapi.

-
Lionel

>
> Regards,
>
> Tvrtko
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/trace: Describe engines as class:instance pairs

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/trace: Describe engines as class:instance pairs
URL   : https://patchwork.freedesktop.org/series/43709/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4233 -> Patchwork_9106 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9106 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9106, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43709/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9106:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9106 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-bsw-n3050:       PASS -> DMESG-WARN (fdo#106207)

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         PASS -> FAIL (fdo#102575)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-hsw-4200u:       FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#106207 https://bugs.freedesktop.org/show_bug.cgi?id=106207


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4233 -> Patchwork_9106

  CI_DRM_4233: 0b7643db57abff1223f77fb8cbb8dd8a00ca9938 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4498: f9ecb79ad8b02278cfdb5b82495df47061c04f8f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9106: 4f5293dac6122cdcb6456a96f29f8c69c73e0346 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4f5293dac612 drm/i915/trace: Remove engine out of the context sandwich
29e13710369f drm/i915/trace: Describe engines as class:instance pairs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9106/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/trace: Describe engines as class:instance pairs

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/trace: Describe engines as class:instance pairs
URL   : https://patchwork.freedesktop.org/series/43709/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4233_full -> Patchwork_9106_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9106_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9106_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43709/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9106_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9106_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking:
      shard-glk:          PASS -> FAIL (fdo#105703)

    igt@kms_flip_tiling@flip-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          FAIL (fdo#105703) -> PASS

    igt@kms_flip@plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4233 -> Patchwork_9106

  CI_DRM_4233: 0b7643db57abff1223f77fb8cbb8dd8a00ca9938 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4498: f9ecb79ad8b02278cfdb5b82495df47061c04f8f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9106: 4f5293dac6122cdcb6456a96f29f8c69c73e0346 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9106/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Tvrtko Ursulin]

On 24/05/2018 17:13, Lionel Landwerlin wrote:
> On 24/05/18 17:07, Tvrtko Ursulin wrote:
>>
>> On 24/05/2018 16:53, Lionel Landwerlin wrote:
>>> On 24/05/18 16:04, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Instead of using the engine->id, use uabi_class:instance pairs in 
>>>> trace-
>>>> points including engine info.
>>>>
>>>> This will be more readable, more future proof and more stable for
>>>> userspace consumption.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: svetlana.kukanova@intel.com
>>> Don't you want engine->uabi_id instead of engine->instance ?
>>
>> No, class:instance is the new engine identifier - why do you think we 
>> would need legacy engine->uabi_id?
> 
> Maybe I forgot about your engine listing series...
> I would expect the tracepoint to match the engines listed through that 
> uapi.

Yeah I don't have engine->uabi_id exported in engine discovery. I could 
add it, but given how we don't plan to extend it (the legacy engine 
selection), I think it is not needed. If there will be popular demand 
though can do it.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/trace: Describe engines as class:instance pairs

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-05-25 08:11:41)
> 
> On 24/05/2018 17:13, Lionel Landwerlin wrote:
> > On 24/05/18 17:07, Tvrtko Ursulin wrote:
> >>
> >> On 24/05/2018 16:53, Lionel Landwerlin wrote:
> >>> On 24/05/18 16:04, Tvrtko Ursulin wrote:
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>> Instead of using the engine->id, use uabi_class:instance pairs in 
> >>>> trace-
> >>>> points including engine info.
> >>>>
> >>>> This will be more readable, more future proof and more stable for
> >>>> userspace consumption.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>>> Cc: svetlana.kukanova@intel.com
> >>> Don't you want engine->uabi_id instead of engine->instance ?
> >>
> >> No, class:instance is the new engine identifier - why do you think we 
> >> would need legacy engine->uabi_id?
> > 
> > Maybe I forgot about your engine listing series...
> > I would expect the tracepoint to match the engines listed through that 
> > uapi.
> 
> Yeah I don't have engine->uabi_id exported in engine discovery. I could 
> add it, but given how we don't plan to extend it (the legacy engine 
> selection), I think it is not needed. If there will be popular demand 
> though can do it.

Also that we plan to conflate the I915_EXEC_RING selector so that
engine->uabi_id would no longer be a unique mapping, will add to the
confusion.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/trace: Remove engine out of the context sandwich

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx