| Message ID | 1417526478-27010-1-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -4 364/364 360/364
ILK +1-6 365/366 360/366
SNB 450/450 450/450
IVB 498/498 498/498
BYT 289/289 289/289
HSW 564/564 564/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt_gem_concurrent_blit_gpuX-bcs-early-read PASS(2, M25) NO_RESULT(1, M25)
*PNV igt_gem_concurrent_blit_prw-bcs-overwrite-source-interruptible PASS(2, M25) NO_RESULT(1, M25)
*PNV igt_gem_userptr_blits_coherency-sync PASS(2, M25) NO_RESULT(1, M25)
*PNV igt_kms_sysfs_edid_timing PASS(3, M25M7) FAIL(1, M25)
*ILK igt_kms_flip_rcs-flip-vs-panning-interruptible PASS(3, M26) DMESG_WARN(1, M26)
*ILK igt_kms_render_direct-render PASS(3, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_bcs-flip-vs-modeset-interruptible DMESG_WARN(1, M26)PASS(6, M26) DMESG_WARN(1, M26)
*ILK igt_kms_flip_blocking-absolute-wf_vblank-interruptible NSPT(1, M26)PASS(2, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_busy-flip-interruptible DMESG_WARN(1, M26)PASS(5, M26) DMESG_WARN(1, M26)
*ILK igt_kms_flip_rcs-flip-vs-dpms PASS(4, M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_wf_vblank-ts-check DMESG_WARN(2, M26)PASS(11, M26M37) PASS(1, M26)
Note: You need to pay more attention to line start with '*'
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) +{ + 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'); - } - } - } + 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);
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. Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 84 ++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 35 deletions(-)