Re: [PATCH 27/43] drm/i915/bdw: Render state init for Execlists

[Daniel Vetter <daniel@ffwll.ch>]

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@ffwll.ch] On Behalf Of Daniel
> > > Vetter
> > > Sent: Monday, August 11, 2014 10:25 PM
> > > To: Daniel, Thomas
> > > Cc: intel-gfx@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@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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch