> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > Vetter > Sent: Tuesday, December 02, 2014 2:45 PM > To: Daniel, Thomas > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/i915: Fix startup failure in LRC mode > after recent init changes > > On Tue, Dec 02, 2014 at 12:50:48PM +0000, Thomas Daniel wrote: > > A previous commit introduced engine init changes: > > > > commit 372ee59699d9 ("drm/i915: Only init engines once") > > > > This broke execlists as intel_lr_context_render_state_init was trying > > to emit commands to the RCS for the default context before the ring- > >init_hw was called. > > > > Made a new gen8_init_rcs_context function and assign in to render ring > > init_context. Moved call to intel_logical_ring_workarounds_emit into > > gen8_init_rcs_context to maintain previous functionality. > > > > Moved call to render_state_init from lr_context_deferred_create into > > gen8_init_rcs_context, and modified deferred_create to call > > ring->init_context for non-default contexts. > > > > Modified i915_gem_context_enable to call ring->init_context for the > > default context. > > > > So init_context will now always be called when the hw is ready - in > > i915_gem_context_enable for the default context and in > > lr_context_deferred_create for other contexts. > > > > Signed-off-by: Thomas Daniel <thomas.daniel@xxxxxxxxx> > > Oops, sorry for breaking things I didn't realize that we bash things into the hw > in the deferred create. So merged this patch right away to get the regression > out of the way. > > It's not quite there yet for lrc context init though. I think we need to > - split intel_lr_context_deferred_create into _alloc and _init functions. > - Call the _alloc part from logical_ring_init init (which is once at > driver load now) > - only call _init from ->init_context > - then move all the default context special-case handling out of _alloc > into logical_ring_init (i.e. the pinning and similar stuff) and into > ->init_hw (lrc hw setup, status page). > - Shuffle the code in i915_gem_context_enable into ring->init_hw > functions. > > I think this would reduce the confusion we have a lot here. And also remove > a bunch of execlist special cases. > > Thoughts? Signed up? Sounds sensible at first glance. I didn't want to go too far with this patch in case it got controversial ;) I'm going to be away for a few days from tomorrow - will think about it some more when I get back. Thomas. > > Cheers, Daniel > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 25 ++++++++++++++++++-- > ----- > > drivers/gpu/drm/i915/intel_lrc.c | 30 +++++++++++++++++++---------- > - > > 2 files changed, 37 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > b/drivers/gpu/drm/i915/i915_gem_context.c > > index 3c3a9ff..5cd2b97 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -408,14 +408,25 @@ int i915_gem_context_enable(struct > > drm_i915_private *dev_priv) > > > > BUG_ON(!dev_priv->ring[RCS].default_context); > > > > - if (i915.enable_execlists) > > - return 0; > > + if (i915.enable_execlists) { > > + for_each_ring(ring, dev_priv, i) { > > + if (ring->init_context) { > > + ret = ring->init_context(ring, > > + ring->default_context); > > + if (ret) { > > + DRM_ERROR("ring init context: > %d\n", > > + ret); > > + return ret; > > + } > > + } > > + } > > > > - for_each_ring(ring, dev_priv, i) { > > - ret = i915_switch_context(ring, ring->default_context); > > - if (ret) > > - return ret; > > - } > > + } else > > + for_each_ring(ring, dev_priv, i) { > > + ret = i915_switch_context(ring, ring- > >default_context); > > + if (ret) > > + return ret; > > + } > > > > return 0; > > } > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > > b/drivers/gpu/drm/i915/intel_lrc.c > > index 4ffb08c..79ef40c 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -1336,6 +1336,18 @@ static int gen8_emit_request(struct > intel_ringbuffer *ringbuf) > > return 0; > > } > > > > +static int gen8_init_rcs_context(struct intel_engine_cs *ring, > > + struct intel_context *ctx) > > +{ > > + int ret; > > + > > + ret = intel_logical_ring_workarounds_emit(ring, ctx); > > + if (ret) > > + return ret; > > + > > + return intel_lr_context_render_state_init(ring, ctx); } > > + > > /** > > * intel_logical_ring_cleanup() - deallocate the Engine Command Streamer > > * > > @@ -1409,7 +1421,7 @@ static int logical_render_ring_init(struct > drm_device *dev) > > ring->irq_keep_mask |= > GT_RENDER_L3_PARITY_ERROR_INTERRUPT; > > > > ring->init_hw = gen8_init_render_ring; > > - ring->init_context = intel_logical_ring_workarounds_emit; > > + ring->init_context = gen8_init_rcs_context; > > ring->cleanup = intel_fini_pipe_control; > > ring->get_seqno = gen8_get_seqno; > > ring->set_seqno = gen8_set_seqno; > > @@ -1905,21 +1917,17 @@ int intel_lr_context_deferred_create(struct > > intel_context *ctx, > > > > if (ctx == ring->default_context) > > lrc_setup_hardware_status_page(ring, ctx_obj); > > - > > - if (ring->id == RCS && !ctx->rcs_initialized) { > > + else if (ring->id == RCS && !ctx->rcs_initialized) { > > if (ring->init_context) { > > ret = ring->init_context(ring, ctx); > > - if (ret) > > + if (ret) { > > DRM_ERROR("ring init context: %d\n", ret); > > + ctx->engine[ring->id].ringbuf = NULL; > > + ctx->engine[ring->id].state = NULL; > > + goto error; > > + } > > } > > > > - ret = intel_lr_context_render_state_init(ring, ctx); > > - if (ret) { > > - DRM_ERROR("Init render state failed: %d\n", ret); > > - ctx->engine[ring->id].ringbuf = NULL; > > - ctx->engine[ring->id].state = NULL; > > - goto error; > > - } > > ctx->rcs_initialized = true; > > } > > > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > 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
