Re: [PATCH 1/3] drm/i915: Add ppgtt init/release trace points

["Mateo Lozano, Oscar" <oscar.mateo@intel.com>]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, June 19, 2014 8:57 AM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx@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@intel.com wrote:
> > From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >
> > These tracepoints are useful for observing the creation and
> > destruction of Full PPGTTs.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo@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,
> 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.
> -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