| Message ID | 1453504211-7982-6-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 22, 2016 at 11:10:10PM +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 kmap 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. > > v2: > Split from "handle teardown of HWSP when context is freed". > > 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 | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index a3ab2b4..c702fc2 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1986,13 +1986,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); > > - if (ring->status_page.obj) { > - struct page *page; > - > - page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN); > - kunmap(page); > - ring->status_page.obj = NULL; > - } > + /* 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; > @@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx) > continue; > > if (ctx == ctx->i915->kernel_context) { > + struct intel_engine_cs *ring = ringbuf->ring; > + > + /* > + * The HWSP is part of the kernel context > + * object in LRC mode, so tear it down now. > + */ > + if (ring->status_page.obj) { > + struct page *page; > + > + page = i915_gem_object_get_page( > + ring->status_page.obj, > + LRC_PPHWSP_PN); > + kunmap(page); > + ring->status_page.obj = NULL; > + } No. I see no reason why we need to add special code to the context-free. -Chris
On 25/01/16 10:56, Chris Wilson wrote: > On Fri, Jan 22, 2016 at 11:10:10PM +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 kmap 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. >> >> v2: >> Split from "handle teardown of HWSP when context is freed". >> >> 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 | 26 +++++++++++++++++++------- >> 1 file changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index a3ab2b4..c702fc2 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -1986,13 +1986,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); >> >> - if (ring->status_page.obj) { >> - struct page *page; >> - >> - page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN); >> - kunmap(page); >> - ring->status_page.obj = NULL; >> - } >> + /* 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; >> @@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx) >> continue; >> >> if (ctx == ctx->i915->kernel_context) { >> + struct intel_engine_cs *ring = ringbuf->ring; >> + >> + /* >> + * The HWSP is part of the kernel context >> + * object in LRC mode, so tear it down now. >> + */ >> + if (ring->status_page.obj) { >> + struct page *page; >> + >> + page = i915_gem_object_get_page( >> + ring->status_page.obj, >> + LRC_PPHWSP_PN); >> + kunmap(page); >> + ring->status_page.obj = NULL; >> + } > > No. I see no reason why we need to add special code to the context-free. > -Chris The special-case-kernel-context test /is already there/. I'm just making it do the right thing by moving this block out of intel_logical_ring_cleanup() and into the /existing/ special case. Getting rid of the special case entirely is a project for another day, whereas this is a bug fix (NOT unmapping it here leaves a dangling reference to an object that has been freed). OTOH I can change this to use: kunmap(kmap_to_page(ring->status_page.page_addr)); per the previous reply, which will make it a bit neater. .Dave.
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index a3ab2b4..c702fc2 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1986,13 +1986,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); - if (ring->status_page.obj) { - struct page *page; - - page = i915_gem_object_get_page(ring->status_page.obj, LRC_PPHWSP_PN); - kunmap(page); - ring->status_page.obj = NULL; - } + /* 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; @@ -2431,6 +2427,22 @@ void intel_lr_context_free(struct intel_context *ctx) continue; if (ctx == ctx->i915->kernel_context) { + struct intel_engine_cs *ring = ringbuf->ring; + + /* + * The HWSP is part of the kernel context + * object in LRC mode, so tear it down now. + */ + if (ring->status_page.obj) { + struct page *page; + + page = i915_gem_object_get_page( + ring->status_page.obj, + LRC_PPHWSP_PN); + kunmap(page); + ring->status_page.obj = NULL; + } + intel_unpin_ringbuffer_obj(ringbuf); i915_gem_object_ggtt_unpin(ctx_obj); }
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 kmap 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. v2: Split from "handle teardown of HWSP when context is freed". 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 | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)