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@xxxxxxxxx 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@xxxxxxxxxxxxxxxxxx> (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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx