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

Chris Wilson
2014-03-19T06:55:24Z

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@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@chris-wilson.co.uk>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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