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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Dec 05, 2014 at 02:15:22PM +0000, Chris Wilson wrote:
> 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@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);

This is going to assume ring->head/tail are already valid?

>  
> +	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;
>  	}
>  
> -	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);

Here too.

So what happens on GPU reset?

> +
>  	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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx