Re: [PATCH] drm/i915: Don't pin LRC in GGTT when dumping in debugfs

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 02, 2014 at 01:21:18PM +0000, Thomas Daniel wrote:
> LRC object does not need to be mapped into the GGTT when dumping. A side-effect
> of this patch is that a compiler warning goes away (not checking return value
> of i915_gem_obj_ggtt_pin).
> 
> v2: Broke out individual context dumping into a new function as the indentation
> was getting a bit crazy.  Added notification of contexts with no gem object for
> debugging purposes.  Removed unnecessary pin_pages and unpin_pages, replaced
> with explicit get_pages for the context object as there may be no backing store
> allocated at this time (Comment for get_pages says "Ensure that the associated
> pages are gathered from the backing storage and pinned into our object").
> Improved error checking - get_pages and get_page are checked for failure.

The comment that get_pages pins the backing storage is outdated btw -
at every point where the shrinker might kick in (any memory allocation
really) you might loose the backing storage again. Hence why we have the
pin/unpin_pages functions. But like Chris said this isn't actually
required here. Care for a patch to update that comment?

> 
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   84 ++++++++++++++++++++---------------
>  1 file changed, 49 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7ea3843..e1de646 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1773,6 +1773,50 @@ static int i915_context_status(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> +static void i915_dump_lrc_obj(struct seq_file *m,
> +		struct intel_engine_cs *ring,
> +		struct drm_i915_gem_object *ctx_obj)

Please align parameter continuation lines to the opening ( since that's
the usual style we use in i915. Not doing that tends to look pretty
jarring. checkpatch --strict will look for these (and a few other things I
tend to ignore, so please use with caution).

> +{
> +	struct page *page;
> +	uint32_t *reg_state;
> +	int j;
> +	unsigned long ggtt_offset = 0;
> +
> +	if (ctx_obj == NULL) {
> +		seq_printf(m, "Context on %s with no gem object\n",
> +				ring->name);
> +		return;
> +	}
> +
> +	seq_printf(m, "CONTEXT: %s %u\n", ring->name,
> +			intel_execlists_ctx_id(ctx_obj));
> +
> +	if (!i915_gem_obj_ggtt_bound(ctx_obj))
> +		seq_puts(m, "\tNot bound in GGTT\n");
> +	else
> +		ggtt_offset = i915_gem_obj_ggtt_offset(ctx_obj);
> +
> +	if (i915_gem_object_get_pages(ctx_obj)) {
> +		seq_puts(m, "\tFailed to get pages for context object\n");
> +		return;
> +	}
> +
> +	page = i915_gem_object_get_page(ctx_obj, 1);
> +	if (!WARN_ON(page == NULL)) {
> +		reg_state = kmap_atomic(page);
> +
> +		for (j = 0; j < 0x600 / sizeof(u32) / 4; j += 4) {
> +			seq_printf(m, "\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n",
> +					ggtt_offset + 4096 + (j * 4),
> +					reg_state[j], reg_state[j + 1],
> +					reg_state[j + 2], reg_state[j + 3]);
> +		}
> +		kunmap_atomic(reg_state);
> +	}
> +
> +	seq_putc(m, '\n');
> +}
> +
>  static int i915_dump_lrc(struct seq_file *m, void *unused)
>  {
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
> @@ -1791,41 +1835,11 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>  	if (ret)
>  		return ret;
>  
> -	list_for_each_entry(ctx, &dev_priv->context_list, link) {
> -		for_each_ring(ring, dev_priv, i) {
> -			struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
> -
> -			if (ring->default_context == ctx)
> -				continue;
> -
> -			if (ctx_obj) {
> -				struct page *page;
> -				uint32_t *reg_state;
> -				int j;
> -
> -				i915_gem_obj_ggtt_pin(ctx_obj,
> -						GEN8_LR_CONTEXT_ALIGN, 0);
> -
> -				page = i915_gem_object_get_page(ctx_obj, 1);
> -				reg_state = kmap_atomic(page);
> -
> -				seq_printf(m, "CONTEXT: %s %u\n", ring->name,
> -						intel_execlists_ctx_id(ctx_obj));
> -
> -				for (j = 0; j < 0x600 / sizeof(u32) / 4; j += 4) {
> -					seq_printf(m, "\t[0x%08lx] 0x%08x 0x%08x 0x%08x 0x%08x\n",
> -					i915_gem_obj_ggtt_offset(ctx_obj) + 4096 + (j * 4),
> -					reg_state[j], reg_state[j + 1],
> -					reg_state[j + 2], reg_state[j + 3]);
> -				}
> -				kunmap_atomic(reg_state);
> -
> -				i915_gem_object_ggtt_unpin(ctx_obj);
> -
> -				seq_putc(m, '\n');
> -			}
> -		}
> -	}

I've added back some of the braces here for readability of this nested
loops - I got a bit confused ;-)

Queued for -next, thanks for the patch.
-Daniel

> +	list_for_each_entry(ctx, &dev_priv->context_list, link)
> +		for_each_ring(ring, dev_priv, i)
> +			if (ring->default_context != ctx)
> +				i915_dump_lrc_obj(m, ring,
> +						ctx->engine[i].state);
>  
>  	mutex_unlock(&dev->struct_mutex);
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx