[Intel-gfx] [PATCH v2 5/6] drm/i915: HWSP should be unmapped earlier in LRC teardown sequence

[Chris Wilson]

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 at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at 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

-- 
Chris Wilson, Intel Open Source Technology Centre