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

[Daniel, Thomas]

> -----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).

Thomas.

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