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

[Ben Widawsky]

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

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