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

Ben Widawsky
2014-03-19T00:05:49Z

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))

> +
> + 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)?

> else
> stats->inactive += obj->base.size;
> - } else {
> - if (!list_empty(&obj->global_list))
> - stats->unbound += obj->base.size;
> +
> + return 0;
> }
>
> + if (!list_empty(&obj->global_list))
> + stats->unbound += obj->base.size;
> +
> return 0;
> }
>
> @@ -411,6 +429,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
> struct task_struct *task;
>
> memset(&stats, 0, sizeof(stats));
> + stats.file_priv = file->driver_priv;
> idr_for_each(&file->object_idr, per_file_stats, &stats);
> /*
> * Although we have a valid reference on file->pid, that does
> @@ -420,12 +439,13 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
> */
> rcu_read_lock();
> task = pid_task(file->pid, PIDTYPE_PID);
> - seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu inactive, %zu unbound)\n",
> + seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu unbound)\n",
> task ? task->comm : "<unknown>",
> stats.count,
> stats.total,
> stats.active,
> stats.inactive,
> + stats.global,
> stats.unbound);
> rcu_read_unlock();
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2a319ba..b76c6de 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -721,6 +721,8 @@ struct i915_hw_ppgtt {
> dma_addr_t *gen8_pt_dma_addr[4];
> };
>
> + struct i915_hw_context *ctx;
> +
> int (*enable)(struct i915_hw_ppgtt *ppgtt);
> int (*switch_mm)(struct i915_hw_ppgtt *ppgtt,
> struct intel_ring_buffer *ring,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ce41cff..1a94b07 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -215,6 +215,7 @@ create_vm_for_ctx(struct drm_device *dev, struct i915_hw_context *ctx)
> return ERR_PTR(ret);
> }
>
> + ppgtt->ctx = ctx;
> return ppgtt;
> }
>
> --
> 1.8.5.3
>

--
Ben Widawsky, Intel Open Source Technology Center