[Intel-gfx] [PATCH 1/4] drm/i915: handle teardown of HWSP when context is freed

[Mika Kuoppala]

Dave Gordon <david.s.gordon at intel.com> writes:

> Existing code did a kunmap() on the wrong page, and didn't account for the
> fact that the HWSP and the default context are different offsets within the
> same GEM object.
>

Just to note here that usually we try to split bug fixes early in the
series with minimal code changes. This helps cherrypickups
to fixes/stable. And also lessens the probability that we accidentally
revert bugfix when some other problem occurs with the patch.

> This patch improves symmetry by creating lrc_teardown_hardware_status_page()
> to complement lrc_setup_hardware_status_page(), making sure that they both
> take the same argument (pointer to engine). They encapsulate all the details
> of the relationship between the default context and the status page, so the
> rest of the LRC code doesn't have to know about it.
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 57 ++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 73d4347..3914120 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -226,9 +226,8 @@ enum {
>  #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT  0x17
>  
>  static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
> -static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> -		struct drm_i915_gem_object *default_ctx_obj);
> -
> +static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring);
> +static void lrc_teardown_hardware_status_page(struct intel_engine_cs *ring);
>  
>  /**
>   * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
> @@ -1536,8 +1535,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u8 next_context_status_buffer_hw;
>  
> -	lrc_setup_hardware_status_page(ring,
> -				dev_priv->kernel_context->engine[ring->id].state);
> +	lrc_setup_hardware_status_page(ring);
>  
>  	I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
>  	I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
> @@ -1992,10 +1990,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) {
> -		kunmap(sg_page(ring->status_page.obj->pages->sgl));
> -		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;
> @@ -2434,6 +2431,11 @@ void intel_lr_context_free(struct intel_context *ctx)
>  			continue;
>  
>  		if (ctx == ctx->i915->kernel_context) {
> +			/*
> +			 * The HWSP is part of the default context
> +			 * object in LRC mode.
> +			 */
> +			lrc_teardown_hardware_status_page(ringbuf->ring);
>  			intel_unpin_ringbuffer_obj(ringbuf);
>  			i915_gem_object_ggtt_unpin(ctx_obj);
>  		}
> @@ -2482,24 +2484,45 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *ring)
>  	return ret;
>  }
>  
> -static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
> -		struct drm_i915_gem_object *default_ctx_obj)
> +static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring)
>  {
> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(ring->dev);
> +	struct intel_context *dctx = dev_priv->kernel_context;
> +	struct drm_i915_gem_object *dctx_obj = dctx->engine[ring->id].state;
> +	u64 dctx_addr = i915_gem_obj_ggtt_offset(dctx_obj);
>  	struct page *page;
>  
> -	/* The HWSP is part of the default context object in LRC mode. */
> -	ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(default_ctx_obj)
> -			+ LRC_PPHWSP_PN * PAGE_SIZE;
> -	page = i915_gem_object_get_page(default_ctx_obj, LRC_PPHWSP_PN);
> +	/*
> +	 * The HWSP is part of the default context object in LRC mode.
> +	 * Note that it doesn't contribute to the refcount!
> +	 */
> +	page = i915_gem_object_get_page(dctx_obj, LRC_PPHWSP_PN);
>  	ring->status_page.page_addr = kmap(page);
> -	ring->status_page.obj = default_ctx_obj;
> +	ring->status_page.gfx_addr = dctx_addr + LRC_PPHWSP_PN * PAGE_SIZE;
> +	ring->status_page.obj = dctx_obj;
>  
>  	I915_WRITE(RING_HWS_PGA(ring->mmio_base),
>  			(u32)ring->status_page.gfx_addr);
>  	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)
> +{
> +	struct drm_i915_gem_object *dctx_obj = ring->status_page.obj;
> +	struct page *page;
> +
> +	WARN_ON(!dctx_obj);
> +
> +	if (ring->status_page.page_addr) {
> +		page = i915_gem_object_get_page(dctx_obj, LRC_PPHWSP_PN);
> +		kunmap(page);
> +		ring->status_page.page_addr = NULL;
> +	}
> +

As the page is always kmapped along with setting of status_page.obj,
I fail to see why we need the check here for the page_addr.

How about just:

if (WARN_ON(!dctx_obj))
   return;

-Mika

> +	ring->status_page.obj = NULL;
> +}
> +
>  /**
>   * intel_lr_context_deferred_alloc() - create the LRC specific bits of a context
>   * @ctx: LR context to create.
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx