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

[Ceraolo Spurio, Daniele]

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.

Thanks,
Daniele