[Intel-gfx] [PATCH 1/3] drm/i915: Add ppgtt init/release trace points

[Daniele Ceraolo Spurio]

On 19 June 2014 15:22, Mateo Lozano, Oscar <oscar.mateo at intel.com> wrote:
>
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > Sent: Thursday, June 19, 2014 8:57 AM
> > To: Mateo Lozano, Oscar
> > Cc: intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915: Add ppgtt init/release trace
> > points
> >
> > On Wed, Jun 18, 2014 at 05:16:39PM +0100, oscar.mateo at intel.com wrote:
> > > From: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > >
> > > These tracepoints are useful for observing the creation and
> > > destruction of Full PPGTTs.
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_context.c |  5 +++++
> > >  drivers/gpu/drm/i915/i915_trace.h       | 38
> > +++++++++++++++++++++++++++++++++
> > >  2 files changed, 43 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > > b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index 5a62a19..bdfe3f5 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -136,6 +136,8 @@ static void ppgtt_release(struct kref *kref)
> > >     struct i915_hw_ppgtt *ppgtt =
> > >             container_of(kref, struct i915_hw_ppgtt, ref);
> > >
> > > +   trace_ppgtt_release(ppgtt);
> > > +
> > >     do_ppgtt_cleanup(ppgtt);
> > >     kfree(ppgtt);
> > >  }
> > > @@ -215,6 +217,9 @@ create_vm_for_ctx(struct drm_device *dev, struct
> > intel_context *ctx)
> > >     }
> > >
> > >     ppgtt->ctx = ctx;
> > > +
> > > +   trace_ppgtt_init(ppgtt);
> > > +
> > >     return ppgtt;
> > >  }
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_trace.h
> > > b/drivers/gpu/drm/i915/i915_trace.h
> > > index f5aa006..2d206d8 100644
> > > --- a/drivers/gpu/drm/i915/i915_trace.h
> > > +++ b/drivers/gpu/drm/i915/i915_trace.h
> > > @@ -587,6 +587,44 @@ TRACE_EVENT(intel_gpu_freq_change,
> > >         TP_printk("new_freq=%u", __entry->freq)  );
> > >
> > > +TRACE_EVENT(ppgtt_init,
> > > +
> > > +   TP_PROTO(struct i915_hw_ppgtt *ppgtt),
> > > +
> > > +   TP_ARGS(ppgtt),
> > > +
> > > +   TP_STRUCT__entry(
> > > +           __field(struct i915_hw_ppgtt*, trace_ppgtt)
> > > +           __field(unsigned int, ppgtt_op_code)
> > > +   ),
> > > +
> > > +   TP_fast_assign(
> > > +           __entry->trace_ppgtt = ppgtt;
> > > +           __entry->ppgtt_op_code = 0;
> > > +   ),
> > > +
> > > +   TP_printk("ppgtt op: %u", __entry->ppgtt_op_code)
> >
> > op is redundant and unuseful since it is already encoded into the tracepoint
> > itself. I'm not happy with the tracepoint name either, but it is close. Note,

Would something like "i915_create_ppgtt" be ok?

> > that we do like to pretend that our driver can coexist with itself. That means
> > we have to pass along the dev->minor here so that listeners can distinguish
> > events between imaginary devices. You need to say which client created the
> > ppgtt and include some method for identifying the ppgtt.

There are several things that we could use to identify the ppgtt. My
choice would be the VM pointer, which is unique for each ppgtt and is
also printed by the i915_vma_bind and i915_vma_unbind traces, but we
could also use the context id. The problem with using the context id
is that we won't be able to print it in the ppgtt_release trace.
Regarding the client, my idea is to print the task pid and/or the
process name; would that look good for you?

> > -Chris
>
> Hmmm... I was submitting this on behalf of our validation team but in retrospect that was a bad idea: it´s much better if they participate in the discussion. Daniele, care to join?
>
> -- Oscar

Thanks,
-- Daniele