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

[Daniel Vetter]

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?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch