Re: [PATCH v4 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown sequence

[Chris Wilson <chris@chris-wilson.co.uk>]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx