[Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for Execlists

[Daniel Vetter]

On Wed, Aug 20, 2014 at 03:55:42PM +0000, Daniel, Thomas wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel, Thomas
> > Sent: Friday, August 15, 2014 9:44 AM
> > To: 'Daniel Vetter'
> > Cc: intel-gfx at lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for
> > Execlists
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Thursday, August 14, 2014 9:00 PM
> > > To: Daniel, Thomas
> > > Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init
> > > for Execlists
> > >
> > > On Wed, Aug 13, 2014 at 05:30:07PM +0200, Daniel Vetter wrote:
> > > > On Wed, Aug 13, 2014 at 03:07:29PM +0000, Daniel, Thomas wrote:
> > > > > > -----Original Message-----
> > > > > > From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> > > > > > Daniel Vetter
> > > > > > Sent: Monday, August 11, 2014 10:25 PM
> > > > > > To: Daniel, Thomas
> > > > > > Cc: intel-gfx at lists.freedesktop.org
> > > > > > Subject: Re: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render
> > > > > > state init for Execlists
> > > > > >
> > > > > > On Thu, Jul 24, 2014 at 05:04:35PM +0100, Thomas Daniel wrote:
> > > > > > > From: Oscar Mateo <oscar.mateo at intel.com>
> > > >  > > index 9085ff1..0dc6992 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > > > > @@ -513,8 +513,23 @@ int i915_gem_context_enable(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > > >  		ppgtt->enable(ppgtt);
> > > > > > >  	}
> > > > > > >
> > > > > > > -	if (i915.enable_execlists)
> > > > > > > +	if (i915.enable_execlists) {
> > > > > > > +		struct intel_context *dctx;
> > > > > > > +
> > > > > > > +		ring = &dev_priv->ring[RCS];
> > > > > > > +		dctx = ring->default_context;
> > > > > > > +
> > > > > > > +		if (!dctx->rcs_initialized) {
> > > > > > > +			ret = intel_lr_context_render_state_init(ring,
> > > dctx);
> > > > > > > +			if (ret) {
> > > > > > > +				DRM_ERROR("Init render state failed:
> > > %d\n",
> > > > > > ret);
> > > > > > > +				return ret;
> > > > > > > +			}
> > > > > > > +			dctx->rcs_initialized = true;
> > > > > > > +		}
> > > > > > > +
> > > > > > >  		return 0;
> > > > > > > +	}
> > > > > >
> > > > > > This looks very much like the wrong place. We should init the
> > > > > > render state when we create the context, or when we switch to it
> > > > > > for
> > > the first time.
> > > > > > The later is what the legacy contexts currently do in do_switch.
> > > > > >
> > > > > > But ctx_enable should do the switch to the default context and
> > > > > > that's about
> > > > > Well, a side-effect of switching to the default context in legacy
> > > > > mode is that the render state gets initialized.  I could move the
> > > > > lr render state init call into an enable_execlists branch in
> > > > > i915_switch_context() but that doen't seem like the right place.
> > > > >
> > > > > How about in i915_gem_init() after calling i915_gem_init_hw()?
> > > > >
> > > > > > it. If there's some depency then I guess we should stall the
> > > > > > creation of the default context a bit, maybe.
> > > > > >
> > > > > > In any case someone needs to explain this better and if there's
> > > > > > not other wey this at least needs a bit comment. So I'll punt for now.
> > > > > When the default context is created the driver is not ready to
> > > > > execute a batch.  That is why the render state init can't be done then.
> > > >
> > > > That sounds like the default context is created too early.
> > > > Essentially I want to avoid needless divergence between the default
> > > > context and normal contexts, because sooner or later that will means
> > > > someone will creep in with a _really_ subtle bug.
> > > >
> > > > What about:
> > > > - We create the default lrc contexs in context_init, but like with a
> > > >   normal context we don't do any of the deferred setup.
> > > > - In context_enable (which since yesterday properly propagates errors to
> > > >   callers) we force the deferred lrc ctx setup for the default contexts on
> > > >   all engines.
> > > > - The render state init is done as part of the deferred ctx setup for the
> > > >   render engine in all cases.
> > > >
> > > > Totally off the track or do you see a workable solution somewhere in
> > > > that direction?
> > >
> > > I'd like to discuss this first a bit more, so will punt on this patch for now.
> > > -Daniel
> > I think that your proposal will work.  I've been having some trouble with my
> > RVP board so haven't had a chance to test it out yet.
> > 
> > Thomas.
> I've now tried this out and I don't think it can work without introducing
> more problems than the original patch.  Trouble is that in lrc mode the
> Hardware Status Page is offset 0 from the context.  All contexts use the
> default context's HWSP for writing seqnos, this is stored in
> ring->status_page.  We can't populate this until the deferred creation of
> the default context is done, so we can't execute any instructions in the
> deferred creation (unless we check for default context in the deferred
> creation which is what we wanted to avoid in the first place).

That just sounds like we should either allocate the HWS at engine init
time (since it's shared across all rings). Or that we should switch to
per-ring/ctx hws, but that's probably only something for when the
scheduler shows up.

Or is there some other blocker?

Generally I really don't like to bend over backwards in the init code if
we get such initialization ordering constraints wrong - in the past that
really has been a fairly awesome source of bugs. What's needed though is a
pile of WARN_ON to check that the initialization ordering is done in the
right way, in case someone touches the code. That still means runtime
testing, but that's still a lot better than silent failures.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch