| Message ID | 1417524648-3834-1-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> 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? 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Tuesday, December 02, 2014 2:45 PM > To: Daniel, Thomas > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [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@intel.com> > > 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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; }
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@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 25 ++++++++++++++++++------- drivers/gpu/drm/i915/intel_lrc.c | 30 +++++++++++++++++++----------- 2 files changed, 37 insertions(+), 18 deletions(-)