[Intel-gfx] [PATCH 2/2] drm/i915: Reorder hw init to avoid executing with invalid context/mm state

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> Currently we initialise the rings, add the first context switch to the
> ring and execute our golden state then enable (aliasing or full) ppgtt.
> However, as we enable ppgtt using direct MMIO but load the PD using
> MI_LRI, we end up executing the context switch and golden render state
> with an invalid PD generating page faults. To solve this issue, first do
> the ppgtt PD setup, then set the default context and write the commands
> to run the render state into the ring, before we activate the ring. This
> allows us to be sure that the register state is valid before we begin
> execution.
>
> This was spotted when writing the seqno/request conversion, but only with
> the ERROR capture did I realise that it was a necessity now.
>
> RFC: cleanup the error handling in i915_gem_init_hw.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 20 ++++++++++----------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  9 ++++++---
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c1c11418231b..c13842d3cbc9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4796,15 +4796,15 @@ i915_gem_init_hw(struct drm_device *dev)
>  	 */
>  	init_unused_rings(dev);
>  
> -	for_each_ring(ring, dev_priv, i) {
> -		ret = ring->init_hw(ring);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	for (i = 0; i < NUM_L3_SLICES(dev); i++)
>  		i915_gem_l3_remap(&dev_priv->ring[RCS], i);
>  

Should we move this after the ppgtt has been init? (or alternatively
let do_switch handle it)

> +	ret = i915_ppgtt_init_hw(dev);
> +	if (ret && ret != -EIO) {
> +		DRM_ERROR("PPGTT enable failed %d\n", ret);
> +		i915_gem_cleanup_ringbuffer(dev);
> +	}
> +
>  	/*
>  	 * XXX: Contexts should only be initialized once. Doing a switch to the
>  	 * default context switch however is something we'd like to do after
> @@ -4820,10 +4820,10 @@ i915_gem_init_hw(struct drm_device *dev)
>  		return ret;
>  	}

The comment needs tweaking as it reads 

/* Context switching requires rings (for * the do_switch), but before
 * enabling PPGTT. So don't move this."
*/

If I read the patch correctly you want to keep the ring head/tail
bookkeeping intact so that you can push the ctx_switch into the buffer
before the actual ring hw is activated?

Does this then enable you to get rid of the whole pre emitting of
l3remap stuff and just let the do_switch handle l3remap?

-Mika

> -	ret = i915_ppgtt_init_hw(dev);
> -	if (ret && ret != -EIO) {
> -		DRM_ERROR("PPGTT enable failed %d\n", ret);
> -		i915_gem_cleanup_ringbuffer(dev);
> +	for_each_ring(ring, dev_priv, i) {
> +		ret = ring->init_hw(ring);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index de38b04135d2..95ebce73d46d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -585,6 +585,9 @@ static int init_ring_common(struct intel_engine_cs *ring)
>  	I915_WRITE_HEAD(ring, 0);
>  	(void)I915_READ_HEAD(ring);
>  
> +	I915_WRITE_HEAD(ring, ringbuf->head);
> +	I915_WRITE_TAIL(ring, ringbuf->head);
> +
>  	I915_WRITE_CTL(ring,
>  			((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES)
>  			| RING_VALID);
> @@ -592,7 +595,7 @@ static int init_ring_common(struct intel_engine_cs *ring)
>  	/* If the head is still not zero, the ring is dead */
>  	if (wait_for((I915_READ_CTL(ring) & RING_VALID) != 0 &&
>  		     I915_READ_START(ring) == i915_gem_obj_ggtt_offset(obj) &&
> -		     (I915_READ_HEAD(ring) & HEAD_ADDR) == 0, 50)) {
> +		     (I915_READ_HEAD(ring) & HEAD_ADDR) == ringbuf->head, 50)) {
>  		DRM_ERROR("%s initialization failed "
>  			  "ctl %08x (valid? %d) head %08x tail %08x start %08x [expected %08lx]\n",
>  			  ring->name,
> @@ -603,9 +606,9 @@ static int init_ring_common(struct intel_engine_cs *ring)
>  		goto out;
>  	}
>  
> +	I915_WRITE_TAIL(ring, ringbuf->tail);
> +
>  	ringbuf->last_retired_head = -1;
> -	ringbuf->head = I915_READ_HEAD(ring);
> -	ringbuf->tail = I915_READ_TAIL(ring) & TAIL_ADDR;
>  	intel_ring_update_space(ringbuf);
>  
>  	memset(&ring->hangcheck, 0, sizeof(ring->hangcheck));
> -- 
> 2.1.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx