| Message ID | 20180516064741.30912-7-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/05/2018 07:47, Chris Wilson wrote: > We cannot call kthread_park() from softirq context, so let's avoid it > entirely during the reset. We wanted to suspend the signaler so that it > would not mark a request as complete at the same time as we marked it as > being in error. Instead of parking the signaling, stop the engine from > advancing so that the GPU doesn't emit the breadcrumb for our chosen > "guilty" request. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > CC: Michel Thierry <michel.thierry@intel.com> > Cc: Jeff McGee <jeff.mcgee@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 14 -------------- > drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++++++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++ > 3 files changed, 39 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index abf661d40641..b0fe452ce17c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3015,18 +3015,6 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) > */ > intel_uncore_forcewake_get(engine->i915, FORCEWAKE_ALL); > > - /* > - * Prevent the signaler thread from updating the request > - * state (by calling dma_fence_signal) as we are processing > - * the reset. The write from the GPU of the seqno is > - * asynchronous and the signaler thread may see a different > - * value to us and declare the request complete, even though > - * the reset routine have picked that request as the active > - * (incomplete) request. This conflict is not handled > - * gracefully! > - */ > - kthread_park(engine->breadcrumbs.signaler); > - > request = engine->reset.prepare(engine); > if (request && request->fence.error == -EIO) > request = ERR_PTR(-EIO); /* Previous reset failed! */ > @@ -3229,8 +3217,6 @@ void i915_gem_reset_finish_engine(struct intel_engine_cs *engine) > { > engine->reset.finish(engine); > > - kthread_unpark(engine->breadcrumbs.signaler); > - > intel_uncore_forcewake_put(engine->i915, FORCEWAKE_ALL); > } > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index cae10ebf9432..16e4f3f6bbbf 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1839,6 +1839,21 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) > return 0; > } > > +static void set_stop_engine(struct intel_engine_cs *engine) > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + const u32 base = engine->mmio_base; > + const i915_reg_t mode = RING_MI_MODE(base); > + > + GEM_TRACE("%s\n", engine->name); > + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); > + if (__intel_wait_for_register_fw(dev_priv, > + mode, MODE_IDLE, MODE_IDLE, > + 1000, 0, > + NULL)) > + GEM_TRACE("%s: timed out on STOP_RING -> IDLE\n", engine->name); > +} > + > static struct i915_request * > execlists_reset_prepare(struct intel_engine_cs *engine) > { > @@ -1878,6 +1893,12 @@ execlists_reset_prepare(struct intel_engine_cs *engine) > if (request) { > unsigned long flags; > > + /* > + * Prevent the breadcrumb from advancing before we decide > + * which request is currently active. > + */ > + set_stop_engine(engine); > + > spin_lock_irqsave(&engine->timeline.lock, flags); > list_for_each_entry_from_reverse(request, > &engine->timeline.requests, > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index af5a178366ed..bb88a92fcc1e 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -531,8 +531,26 @@ static int init_ring_common(struct intel_engine_cs *engine) > return ret; > } > > +static void set_stop_engine(struct intel_engine_cs *engine) > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + const u32 base = engine->mmio_base; > + const i915_reg_t mode = RING_MI_MODE(base); > + > + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); > + if (__intel_wait_for_register_fw(dev_priv, > + mode, MODE_IDLE, MODE_IDLE, > + 1000, 0, > + NULL)) > + DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n", > + engine->name); > +} Looks to be exactly the same implementation as in intel_lrc.c apart from debug vs trace. Move to intel_engine_cs.c? > + > static struct i915_request *reset_prepare(struct intel_engine_cs *engine) > { > + if (INTEL_GEN(engine->i915) >= 3) > + set_stop_engine(engine); > + > if (engine->irq_seqno_barrier) > engine->irq_seqno_barrier(engine); > > Most important question - is stopping the engine expect to mostly work with a palette of different hang types? Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-05-16 10:15:34) > > On 16/05/2018 07:47, Chris Wilson wrote: > > We cannot call kthread_park() from softirq context, so let's avoid it > > entirely during the reset. We wanted to suspend the signaler so that it > > would not mark a request as complete at the same time as we marked it as > > being in error. Instead of parking the signaling, stop the engine from > > advancing so that the GPU doesn't emit the breadcrumb for our chosen > > "guilty" request. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > CC: Michel Thierry <michel.thierry@intel.com> > > Cc: Jeff McGee <jeff.mcgee@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index af5a178366ed..bb88a92fcc1e 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -531,8 +531,26 @@ static int init_ring_common(struct intel_engine_cs *engine) > > return ret; > > } > > > > +static void set_stop_engine(struct intel_engine_cs *engine) > > +{ > > + struct drm_i915_private *dev_priv = engine->i915; > > + const u32 base = engine->mmio_base; > > + const i915_reg_t mode = RING_MI_MODE(base); > > + > > + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); > > + if (__intel_wait_for_register_fw(dev_priv, > > + mode, MODE_IDLE, MODE_IDLE, > > + 1000, 0, > > + NULL)) > > + DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n", > > + engine->name); > > +} > > Looks to be exactly the same implementation as in intel_lrc.c apart from > debug vs trace. Move to intel_engine_cs.c? Not unless you also suggest a name I like ;) Mika once had plans for engine->stop() so I didn't think too hard about this bit, expecting it to be superseded. > > static struct i915_request *reset_prepare(struct intel_engine_cs *engine) > > { > > + if (INTEL_GEN(engine->i915) >= 3) > > + set_stop_engine(engine); > > + > > if (engine->irq_seqno_barrier) > > engine->irq_seqno_barrier(engine); > > > > > > Most important question - is stopping the engine expect to mostly work > with a palette of different hang types? The good news is that if the engine is hung, it doesn't matter! So what it reliably stops is Command Streamer execution along the ring, and we only need to rely on it stopping before the MI_STORE_DWORD of the breadcrumb to be sure that we won't see the breadcrumb advance as we process reset. Now what's stopping the breadcrumb still being in flight (from the GPU) as we sample it (from the CPU)? Not much. -Chris
On 16/05/2018 10:25, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-05-16 10:15:34) >> >> On 16/05/2018 07:47, Chris Wilson wrote: >>> We cannot call kthread_park() from softirq context, so let's avoid it >>> entirely during the reset. We wanted to suspend the signaler so that it >>> would not mark a request as complete at the same time as we marked it as >>> being in error. Instead of parking the signaling, stop the engine from >>> advancing so that the GPU doesn't emit the breadcrumb for our chosen >>> "guilty" request. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>> Cc: Michał Winiarski <michal.winiarski@intel.com> >>> CC: Michel Thierry <michel.thierry@intel.com> >>> Cc: Jeff McGee <jeff.mcgee@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- > >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >>> index af5a178366ed..bb88a92fcc1e 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >>> @@ -531,8 +531,26 @@ static int init_ring_common(struct intel_engine_cs *engine) >>> return ret; >>> } >>> >>> +static void set_stop_engine(struct intel_engine_cs *engine) >>> +{ >>> + struct drm_i915_private *dev_priv = engine->i915; >>> + const u32 base = engine->mmio_base; >>> + const i915_reg_t mode = RING_MI_MODE(base); >>> + >>> + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); >>> + if (__intel_wait_for_register_fw(dev_priv, >>> + mode, MODE_IDLE, MODE_IDLE, >>> + 1000, 0, >>> + NULL)) >>> + DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n", >>> + engine->name); >>> +} >> >> Looks to be exactly the same implementation as in intel_lrc.c apart from >> debug vs trace. Move to intel_engine_cs.c? > > Not unless you also suggest a name I like ;) > > Mika once had plans for engine->stop() so I didn't think too hard about > this bit, expecting it to be superseded. Vfunc, or intel_engine_stop, anything but two of the same. Timed out message should also possibly even be upgraded to notice level. >>> static struct i915_request *reset_prepare(struct intel_engine_cs *engine) >>> { >>> + if (INTEL_GEN(engine->i915) >= 3) >>> + set_stop_engine(engine); >>> + >>> if (engine->irq_seqno_barrier) >>> engine->irq_seqno_barrier(engine); >>> >>> >> >> Most important question - is stopping the engine expect to mostly work >> with a palette of different hang types? > > The good news is that if the engine is hung, it doesn't matter! So what > it reliably stops is Command Streamer execution along the ring, and we > only need to rely on it stopping before the MI_STORE_DWORD of the > breadcrumb to be sure that we won't see the breadcrumb advance as we > process reset. > > Now what's stopping the breadcrumb still being in flight (from the GPU) > as we sample it (from the CPU)? > Not much. If it reliably stops the command streamer then that sounds good enough to me. The in-flight write should be unlikely, since we detected a hang that means any activity happened long time ago. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-05-16 10:49:34) > > On 16/05/2018 10:25, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-05-16 10:15:34) > >> > >> On 16/05/2018 07:47, Chris Wilson wrote: > >>> We cannot call kthread_park() from softirq context, so let's avoid it > >>> entirely during the reset. We wanted to suspend the signaler so that it > >>> would not mark a request as complete at the same time as we marked it as > >>> being in error. Instead of parking the signaling, stop the engine from > >>> advancing so that the GPU doesn't emit the breadcrumb for our chosen > >>> "guilty" request. > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >>> Cc: Michał Winiarski <michal.winiarski@intel.com> > >>> CC: Michel Thierry <michel.thierry@intel.com> > >>> Cc: Jeff McGee <jeff.mcgee@intel.com> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> --- > > > >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >>> index af5a178366ed..bb88a92fcc1e 100644 > >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >>> @@ -531,8 +531,26 @@ static int init_ring_common(struct intel_engine_cs *engine) > >>> return ret; > >>> } > >>> > >>> +static void set_stop_engine(struct intel_engine_cs *engine) > >>> +{ > >>> + struct drm_i915_private *dev_priv = engine->i915; > >>> + const u32 base = engine->mmio_base; > >>> + const i915_reg_t mode = RING_MI_MODE(base); > >>> + > >>> + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); > >>> + if (__intel_wait_for_register_fw(dev_priv, > >>> + mode, MODE_IDLE, MODE_IDLE, > >>> + 1000, 0, > >>> + NULL)) > >>> + DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n", > >>> + engine->name); > >>> +} > >> > >> Looks to be exactly the same implementation as in intel_lrc.c apart from > >> debug vs trace. Move to intel_engine_cs.c? > > > > Not unless you also suggest a name I like ;) > > > > Mika once had plans for engine->stop() so I didn't think too hard about > > this bit, expecting it to be superseded. > > Vfunc, or intel_engine_stop, anything but two of the same. > > Timed out message should also possibly even be upgraded to notice level. It doesn't make any difference unless an error occurs later. The STOP_RING will time out, we know it does. MODE_IDLE will only be set on the next arbitration point (aiui) :( Such cases are usually only resolved by wedging as the reset itself also fail. > >>> static struct i915_request *reset_prepare(struct intel_engine_cs *engine) > >>> { > >>> + if (INTEL_GEN(engine->i915) >= 3) > >>> + set_stop_engine(engine); > >>> + > >>> if (engine->irq_seqno_barrier) > >>> engine->irq_seqno_barrier(engine); > >>> > >>> > >> > >> Most important question - is stopping the engine expect to mostly work > >> with a palette of different hang types? > > > > The good news is that if the engine is hung, it doesn't matter! So what > > it reliably stops is Command Streamer execution along the ring, and we > > only need to rely on it stopping before the MI_STORE_DWORD of the > > breadcrumb to be sure that we won't see the breadcrumb advance as we > > process reset. > > > > Now what's stopping the breadcrumb still being in flight (from the GPU) > > as we sample it (from the CPU)? > > Not much. > > If it reliably stops the command streamer then that sounds good enough > to me. > > The in-flight write should be unlikely, since we detected a hang that > means any activity happened long time ago. Rule of thumb is to always use "hang" ;) - we may have falsely declared a hang and the gpu was just slow - another engine is hung, this one is chugging along oblivious However, the uncached read is just what I would use to allow the write from the GPU to be visible to the CPU, so maybe just add another? -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index abf661d40641..b0fe452ce17c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3015,18 +3015,6 @@ i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) */ intel_uncore_forcewake_get(engine->i915, FORCEWAKE_ALL); - /* - * Prevent the signaler thread from updating the request - * state (by calling dma_fence_signal) as we are processing - * the reset. The write from the GPU of the seqno is - * asynchronous and the signaler thread may see a different - * value to us and declare the request complete, even though - * the reset routine have picked that request as the active - * (incomplete) request. This conflict is not handled - * gracefully! - */ - kthread_park(engine->breadcrumbs.signaler); - request = engine->reset.prepare(engine); if (request && request->fence.error == -EIO) request = ERR_PTR(-EIO); /* Previous reset failed! */ @@ -3229,8 +3217,6 @@ void i915_gem_reset_finish_engine(struct intel_engine_cs *engine) { engine->reset.finish(engine); - kthread_unpark(engine->breadcrumbs.signaler); - intel_uncore_forcewake_put(engine->i915, FORCEWAKE_ALL); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index cae10ebf9432..16e4f3f6bbbf 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1839,6 +1839,21 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine) return 0; } +static void set_stop_engine(struct intel_engine_cs *engine) +{ + struct drm_i915_private *dev_priv = engine->i915; + const u32 base = engine->mmio_base; + const i915_reg_t mode = RING_MI_MODE(base); + + GEM_TRACE("%s\n", engine->name); + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); + if (__intel_wait_for_register_fw(dev_priv, + mode, MODE_IDLE, MODE_IDLE, + 1000, 0, + NULL)) + GEM_TRACE("%s: timed out on STOP_RING -> IDLE\n", engine->name); +} + static struct i915_request * execlists_reset_prepare(struct intel_engine_cs *engine) { @@ -1878,6 +1893,12 @@ execlists_reset_prepare(struct intel_engine_cs *engine) if (request) { unsigned long flags; + /* + * Prevent the breadcrumb from advancing before we decide + * which request is currently active. + */ + set_stop_engine(engine); + spin_lock_irqsave(&engine->timeline.lock, flags); list_for_each_entry_from_reverse(request, &engine->timeline.requests, diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index af5a178366ed..bb88a92fcc1e 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -531,8 +531,26 @@ static int init_ring_common(struct intel_engine_cs *engine) return ret; } +static void set_stop_engine(struct intel_engine_cs *engine) +{ + struct drm_i915_private *dev_priv = engine->i915; + const u32 base = engine->mmio_base; + const i915_reg_t mode = RING_MI_MODE(base); + + I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING)); + if (__intel_wait_for_register_fw(dev_priv, + mode, MODE_IDLE, MODE_IDLE, + 1000, 0, + NULL)) + DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n", + engine->name); +} + static struct i915_request *reset_prepare(struct intel_engine_cs *engine) { + if (INTEL_GEN(engine->i915) >= 3) + set_stop_engine(engine); + if (engine->irq_seqno_barrier) engine->irq_seqno_barrier(engine);
We cannot call kthread_park() from softirq context, so let's avoid it entirely during the reset. We wanted to suspend the signaler so that it would not mark a request as complete at the same time as we marked it as being in error. Instead of parking the signaling, stop the engine from advancing so that the GPU doesn't emit the breadcrumb for our chosen "guilty" request. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> CC: Michel Thierry <michel.thierry@intel.com> Cc: Jeff McGee <jeff.mcgee@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 14 -------------- drivers/gpu/drm/i915/intel_lrc.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++ 3 files changed, 39 insertions(+), 14 deletions(-)