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

Daniel Vetter
2014-08-13T15:29:56Z

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?

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