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

[Daniel, Thomas]

> -----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.
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch