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@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? 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
