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

[Chris Wilson]

On Tue, Jan 20, 2015 at 04:55:56PM +0200, Mika Kuoppala wrote:
> 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)

I did the latter. We treat it as part of the context state.
 
> > +	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?

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

Yes. I even tried to get you to read patches that did that several
months ago...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre