[Intel-gfx] [PATCH 3/4] drm/i915: Per-process stats work better when evaluated per-process

[Chris Wilson]

On Tue, Mar 18, 2014 at 05:05:45PM -0700, Ben Widawsky wrote:
> On Thu, Mar 13, 2014 at 11:57:00AM -0300, Rodrigo Vivi wrote:
> > From: Chris Wilson <chris at chris-wilson.co.uk>
> > 
> > The idea of printing objects used by each process is to judge how each
> > process is using them. This means that we need to evaluate whether the
> > object is bound for that particular process, rather than just whether it
> > is bound into the global GTT.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ben Widawsky <benjamin.widawsky at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c     | 34 ++++++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/i915_drv.h         |  2 ++
> >  drivers/gpu/drm/i915/i915_gem_context.c |  1 +
> >  3 files changed, 30 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index a90d31c..ed3965f 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -299,28 +299,46 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
> >  } while (0)
> >  
> >  struct file_stats {
> > +	struct drm_i915_file_private *file_priv;
> >  	int count;
> > -	size_t total, active, inactive, unbound;
> > +	size_t total, global, active, inactive, unbound;
> >  };
> >  
> >  static int per_file_stats(int id, void *ptr, void *data)
> >  {
> >  	struct drm_i915_gem_object *obj = ptr;
> >  	struct file_stats *stats = data;
> > +	struct i915_vma *vma;
> >  
> >  	stats->count++;
> >  	stats->total += obj->base.size;
> >  
> > -	if (i915_gem_obj_ggtt_bound(obj)) {
> > -		if (!list_empty(&obj->ring_list))
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > +		struct i915_hw_ppgtt *ppgtt;
> > +
> > +		if (!drm_mm_node_allocated(&vma->node))
> > +			continue;
> > +
> > +		ppgtt = container_of(vma->vm, struct i915_hw_ppgtt, base);
> > +		if (ppgtt->ctx == NULL) {
> > +			stats->global += obj->base.size;
> > +			continue;
> > +		}
> 
> I'm not really clear how this is supposed to work for global. Can you
> make me happy and change it to:
> 
> if (i915_is_ggtt(vma->vm))

That better reflects ggtt. How about
if (i915_is_ggtt(vma->vm)) { global++; continue; }
if (ppgtt->ctx == NULL) { default++; continue; }
 
> > +
> > +		if (ppgtt->ctx->file_priv != stats->file_priv)
> > +			continue;
> > +
> > +		if (obj->ring) /* XXX per-vma statistic */
> >  			stats->active += obj->base.size;
> 
> Doesn't active get counted too many times if multiple VMAs exist for the
> same active object (not a new problem to this patch)?

Yes. It's a bridge I'm willing to cross in the future!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre