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 <[email protected]> > --- > 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 > [email protected] > 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 [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
