On Wed, Aug 13, 2014 at 03:07:29PM +0000, Daniel, Thomas wrote: > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > > Vetter > > Sent: Monday, August 11, 2014 10:25 PM > > To: Daniel, Thomas > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > Subject: Re: [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@xxxxxxxxx> > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
