[Intel-gfx] [PATCH v6] drm/i915: Add tracepoints to track a vm during its lifetime

[Chris Wilson]

On Mon, Nov 10, 2014 at 12:28:11PM +0000, Ceraolo Spurio, Daniele wrote:
> On 11/10/2014 11:54 AM, Chris Wilson wrote:
> >On Mon, Nov 10, 2014 at 11:40:40AM +0000, Ceraolo Spurio, Daniele wrote:
> >>On 11/8/2014 8:44 AM, Chris Wilson wrote:
> >>>On Fri, Nov 07, 2014 at 05:45:01PM +0000, daniele.ceraolospurio at intel.com wrote:
> >>>>+/**
> >>>>+ * DOC: execlist_submit_context tracepoint
> >>>>+ *
> >>>>+ * These tracepoint are used to track the contexts that are submitted to the
> >>>>+ * ring. An mm switch is automatically performed by the GPU during the context
> >>>>+ * switch. Given the fact that the mm switch is an important point in the
> >>>>+ * lifetime of a vm, the vm assigned to the context is also printed by the
> >>>>+ * tracepoint when full ppgtt is enabled.
> >>>>+ */
> >>>>+TRACE_EVENT(execlists_submit_context,
> >>>>+	TP_PROTO(struct intel_engine_cs *ring, u32 to_num, struct intel_context *to),
> >>>>+
> >>>>+	TP_ARGS(ring, to_num, to),
> >>>>+
> >>>>+	TP_STRUCT__entry(
> >>>>+			__field(u32, ring)
> >>>>+			__field(u32, to_num)
> >>>>+			__field(struct intel_context *, to)
> >>>>+			__field(struct i915_address_space *, vm)
> >>>>+			__field(u32, dev)
> >>>>+	),
> >>>>+
> >>>>+	TP_fast_assign(
> >>>>+			__entry->ring = ring->id;
> >>>>+			__entry->to_num = to_num;
> >>>>+			__entry->to = to;
> >>>>+			__entry->vm = to->ppgtt ? &to->ppgtt->base : NULL;
> >>>>+			__entry->dev = ring->dev->primary->index;
> >>>>+	),
> >>>>+
> >>>>+	TP_printk("dev=%u, ring=%u, ctx%d=%p, ctx_vm=%p",
> >>>>+		  __entry->dev, __entry->ring,
> >>>>+		  __entry->to_num, __entry->to, __entry->vm)
> >>>>+);
> >>>>+
> >>>>  #endif /* _I915_TRACE_H_ */
> >>>>
> >>>>  /* This part must be outside protection */
> >>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>index 6025ac7..e72759d 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>@@ -135,6 +135,7 @@
> >>>>  #include <drm/drmP.h>
> >>>>  #include <drm/i915_drm.h>
> >>>>  #include "i915_drv.h"
> >>>>+#include "i915_trace.h"
> >>>>
> >>>>  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
> >>>>  #define GEN8_LR_CONTEXT_OTHER_SIZE (2 * PAGE_SIZE)
> >>>>@@ -367,6 +368,7 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring,
> >>>>  	BUG_ON(!ctx_obj0);
> >>>>  	WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
> >>>>
> >>>>+	trace_execlists_submit_context(ring, 0, to0);
> >>>>  	execlists_ctx_write_tail(ctx_obj0, tail0);
> >>>
> >>>This is very tenuous. This is not part of the context lifetime but of
> >>>the request.
> >>>-Chris
> >>>
> >>
> >>The aim wasn't to track the context or the request but to track the
> >>switch_mm. considering that in execlist mode that is automatically
> >>done by the GPU when the ctx is moved on the ring, this looked like
> >>a good place to put the trace. What were you exactly concerned
> >>about?
> >
> >I have a different pattern for the lifetimes in my head. In terms of
> >usage tracking, when the context is submitted is to hardware is more or
> >less irrelevant to the lifetime per se - it is interesting, but only
> >in the context of tracking the execlists/scheduler.
> >
> >For the context, the important point is when a new request (e.g.
> >execbuffer) is created from that context, which will then keep that
> >context alive until the request is complete. That's my
> >switch_mm/switch_context equivalent. (I think I have refined my stance a
> >lot since working with the contexts and requests).
> >
> >The other perspective, is that I want the context tracepoints to be
> >actions on the contexts themselves, rather than actions on the hardware
> >which deserve to be in a different domain. (Obviously in the overlap,
> >there will be some arguing as to which domain it best fits in.)
> >I am trying to keep the tracepoints somewhat logically organised. :|
> >-Chris
> >
> 
> + intel-gfx, which I've inadvertently removed in my previous reply.
> 
> I now understand what you meant and I can see your point. However,
> since I'm still getting familiar with the handling of contexts in
> the driver (execlists etc), I'll have to dig a bit more to find the
> right places for tracepoints. Would it be ok for you if I were to
> drop the submit_context trace for now and just go on with the
> others? I'll get back to this part when I feel more comfortable with
> my understanding of the code.

Yes, all the others looked good to me. Please just drop the contentious
tracepoint inside execlists, and slap on my

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

(we will end up one in execlists, just probably as part of a different
set of tracepoints ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre