[Intel-gfx] [PATCH] drm/i915: Fix startup failure in LRC mode after recent init changes
Daniel, Thomas
thomas.daniel at intel.com
Tue Dec 2 06:53:43 PST 2014
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, December 02, 2014 2:45 PM
> To: Daniel, Thomas
> Cc: intel-gfx at 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 at 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 at 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
More information about the Intel-gfx
mailing list