| Message ID | 1454095171-22475-6-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 29, 2016 at 07:19:30PM +0000, Dave Gordon wrote: > In LRC mode, the HWSP is part of the default context object, and > therefore does not exist independently. Worse, it doesn't contribute > to the refcount on the default context object either. > > Currently, the default context is deallocated in intel_lr_context_free(), > but the HWSP kmapping is not torn down until the subsequent call to > intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj > continues to point to the (now non-existent) default context object, > and the kernel mapping likewise points to a page which is now free. > > The solution is to dispose of the HWSP kmapping and pointer before the > object itself is freed, so this patch moves the call to the teardown > code from intel_lr_context_free() to intel_logical_ring_cleanup(). > > This code was introduced in > > 48d8238 drm/i915/bdw: Generic logical ring init and cleanup > > i.e. it has been there ever since LRC mode was first implemented. > > v3: > Rebased. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index ff38e57..947ef15 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2003,7 +2003,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) > i915_cmd_parser_fini_ring(ring); > i915_gem_batch_pool_fini(&ring->batch_pool); > > - lrc_teardown_hardware_status_page(ring); > + /* Status page should have gone already */ > + WARN_ON(ring->status_page.page_addr); > + WARN_ON(ring->status_page.obj); > > ring->disable_lite_restore_wa = false; > ring->ctx_desc_template = 0; > @@ -2447,6 +2449,11 @@ void intel_lr_context_free(struct intel_context *ctx) > continue; > > if (ctx == ctx->i915->kernel_context) { > + /* > + * The HWSP is part of the kernel context > + * object in LRC mode, so tear it down now. > + */ > + lrc_teardown_hardware_status_page(ringbuf->ring); No. This is tieing the engine specific detail into the already present layering violation in the context that I insist be removed. All you have to do is to correctly pin/unpin the default context during engine setup/teardown to fix the code and elimiate all the silly checks against the kernel_context that execlists does. -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index ff38e57..947ef15 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2003,7 +2003,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) i915_cmd_parser_fini_ring(ring); i915_gem_batch_pool_fini(&ring->batch_pool); - lrc_teardown_hardware_status_page(ring); + /* Status page should have gone already */ + WARN_ON(ring->status_page.page_addr); + WARN_ON(ring->status_page.obj); ring->disable_lite_restore_wa = false; ring->ctx_desc_template = 0; @@ -2447,6 +2449,11 @@ void intel_lr_context_free(struct intel_context *ctx) continue; if (ctx == ctx->i915->kernel_context) { + /* + * The HWSP is part of the kernel context + * object in LRC mode, so tear it down now. + */ + lrc_teardown_hardware_status_page(ringbuf->ring); intel_unpin_ringbuffer_obj(ringbuf); i915_gem_object_ggtt_unpin(ctx_obj); } @@ -2517,12 +2524,19 @@ static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring) POSTING_READ(RING_HWS_PGA(ring->mmio_base)); } +/* This should be called *before* the default context is destroyed */ static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring) { - if (ring->status_page.obj) { + struct drm_i915_gem_object *dctx_obj = ring->status_page.obj; + + WARN_ON(!dctx_obj); + + if (ring->status_page.page_addr) { kunmap(kmap_to_page(ring->status_page.page_addr)); - ring->status_page.obj = NULL; + ring->status_page.page_addr = NULL; } + + ring->status_page.obj = NULL; } /**
In LRC mode, the HWSP is part of the default context object, and therefore does not exist independently. Worse, it doesn't contribute to the refcount on the default context object either. Currently, the default context is deallocated in intel_lr_context_free(), but the HWSP kmapping is not torn down until the subsequent call to intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj continues to point to the (now non-existent) default context object, and the kernel mapping likewise points to a page which is now free. The solution is to dispose of the HWSP kmapping and pointer before the object itself is freed, so this patch moves the call to the teardown code from intel_lr_context_free() to intel_logical_ring_cleanup(). This code was introduced in 48d8238 drm/i915/bdw: Generic logical ring init and cleanup i.e. it has been there ever since LRC mode was first implemented. v3: Rebased. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)