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 <[email protected]>
> ---
>  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
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Reply via email to