On Tue, Jan 20, 2015 at 04:55:56PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
