[Intel-gfx] [PATCH] drm/i915: Fix startup failure in LRC mode after recent init changes

[Daniel Vetter]

On Tue, Dec 02, 2014 at 12:50:48PM +0000, Thomas Daniel wrote:
> A previous commit introduced engine init changes:
> 
>     commit 372ee59699d9 ("drm/i915: Only init engines once")
> 
> This broke execlists as intel_lr_context_render_state_init was trying to emit
> commands to the RCS for the default context before the ring->init_hw was called.
> 
> Made a new gen8_init_rcs_context function and assign in to render ring
> init_context.  Moved call to intel_logical_ring_workarounds_emit into
> gen8_init_rcs_context to maintain previous functionality.
> 
> Moved call to render_state_init from lr_context_deferred_create into
> gen8_init_rcs_context, and modified deferred_create to call ring->init_context
> for non-default contexts.
> 
> Modified i915_gem_context_enable to call ring->init_context for the default
> context.
> 
> So init_context will now always be called when the hw is ready - in
> i915_gem_context_enable for the default context and in lr_context_deferred_create
> for other contexts.
> 
> Signed-off-by: Thomas Daniel <thomas.daniel at intel.com>

Oops, sorry for breaking things I didn't realize that we bash things into
the hw in the deferred create. So merged this patch right away to get the
regression out of the way.

It's not quite there yet for lrc context init though. I think we need to
- split intel_lr_context_deferred_create into _alloc and _init functions.
- Call the _alloc part from logical_ring_init init (which is once at
  driver load now)
- only call _init from ->init_context
- then move all the default context special-case handling out of _alloc
  into logical_ring_init (i.e. the pinning and similar stuff) and into
  ->init_hw (lrc hw setup, status page).
- Shuffle the code in i915_gem_context_enable into ring->init_hw
  functions.

I think this would reduce the confusion we have a lot here. And also
remove a bunch of execlist special cases.

Thoughts? Signed up?

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   25 ++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_lrc.c        |   30 +++++++++++++++++++-----------
>  2 files changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3c3a9ff..5cd2b97 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -408,14 +408,25 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
>  
>  	BUG_ON(!dev_priv->ring[RCS].default_context);
>  
> -	if (i915.enable_execlists)
> -		return 0;
> +	if (i915.enable_execlists) {
> +		for_each_ring(ring, dev_priv, i) {
> +			if (ring->init_context) {
> +				ret = ring->init_context(ring,
> +						ring->default_context);
> +				if (ret) {
> +					DRM_ERROR("ring init context: %d\n",
> +							ret);
> +					return ret;
> +				}
> +			}
> +		}
>  
> -	for_each_ring(ring, dev_priv, i) {
> -		ret = i915_switch_context(ring, ring->default_context);
> -		if (ret)
> -			return ret;
> -	}
> +	} else
> +		for_each_ring(ring, dev_priv, i) {
> +			ret = i915_switch_context(ring, ring->default_context);
> +			if (ret)
> +				return ret;
> +		}
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4ffb08c..79ef40c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1336,6 +1336,18 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf)
>  	return 0;
>  }
>  
> +static int gen8_init_rcs_context(struct intel_engine_cs *ring,
> +		       struct intel_context *ctx)
> +{
> +	int ret;
> +
> +	ret = intel_logical_ring_workarounds_emit(ring, ctx);
> +	if (ret)
> +		return ret;
> +
> +	return intel_lr_context_render_state_init(ring, ctx);
> +}
> +
>  /**
>   * intel_logical_ring_cleanup() - deallocate the Engine Command Streamer
>   *
> @@ -1409,7 +1421,7 @@ static int logical_render_ring_init(struct drm_device *dev)
>  		ring->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>  
>  	ring->init_hw = gen8_init_render_ring;
> -	ring->init_context = intel_logical_ring_workarounds_emit;
> +	ring->init_context = gen8_init_rcs_context;
>  	ring->cleanup = intel_fini_pipe_control;
>  	ring->get_seqno = gen8_get_seqno;
>  	ring->set_seqno = gen8_set_seqno;
> @@ -1905,21 +1917,17 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
>  
>  	if (ctx == ring->default_context)
>  		lrc_setup_hardware_status_page(ring, ctx_obj);
> -
> -	if (ring->id == RCS && !ctx->rcs_initialized) {
> +	else if (ring->id == RCS && !ctx->rcs_initialized) {
>  		if (ring->init_context) {
>  			ret = ring->init_context(ring, ctx);
> -			if (ret)
> +			if (ret) {
>  				DRM_ERROR("ring init context: %d\n", ret);
> +				ctx->engine[ring->id].ringbuf = NULL;
> +				ctx->engine[ring->id].state = NULL;
> +				goto error;
> +			}
>  		}
>  
> -		ret = intel_lr_context_render_state_init(ring, ctx);
> -		if (ret) {
> -			DRM_ERROR("Init render state failed: %d\n", ret);
> -			ctx->engine[ring->id].ringbuf = NULL;
> -			ctx->engine[ring->id].state = NULL;
> -			goto error;
> -		}
>  		ctx->rcs_initialized = true;
>  	}
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch