| Message ID | 20170418202335.35232-6-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 18, 2017 at 01:23:20PM -0700, Michel Thierry wrote: > From: Arun Siluvery <arun.siluvery@linux.intel.com> > > This change implements support for per-engine reset as an initial, less > intrusive hang recovery option to be attempted before falling back to the > legacy full GPU reset recovery mode if necessary. This is only supported > from Gen8 onwards. > > Hangchecker determines which engines are hung and invokes error handler to > recover from it. Error handler schedules recovery for each of those engines > that are hung. The recovery procedure is as follows, > - identifies the request that caused the hang and it is dropped > - force engine to idle: this is done by issuing a reset request > - reset and re-init engine > - restart submissions to the engine > > If engine reset fails then we fall back to heavy weight full gpu reset > which resets all engines and reinitiazes complete state of HW and SW. > > v2: Rebase. > v3: s/*engine_reset*/*reset_engine*/; freeze engine and irqs before > calling i915_gem_reset_engine (Chris). > v4: Rebase, modify i915_gem_reset_prepare to use a ring mask and > reuse the function for reset_engine. > v5: intel_reset_engine_start/cancel instead of request/unrequest_reset. > v6: Clean up reset_engine function to not require mutex, i.e. no need to call > revoke/restore_fences and _retire_requests (Chris). > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Tomas Elf <tomas.elf@intel.com> > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 76 ++++++++++++++++++++++++-- > drivers/gpu/drm/i915/i915_drv.h | 12 +++- > drivers/gpu/drm/i915/i915_gem.c | 97 +++++++++++++++++++-------------- > drivers/gpu/drm/i915/i915_gem_request.c | 2 +- > drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++ > 5 files changed, 158 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index e03d0643dbd6..634893cd93b3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1810,7 +1810,7 @@ void i915_reset(struct drm_i915_private *dev_priv) > > pr_notice("drm/i915: Resetting chip after gpu hang\n"); > disable_irq(dev_priv->drm.irq); > - ret = i915_gem_reset_prepare(dev_priv); > + ret = i915_gem_reset_prepare(dev_priv, ALL_ENGINES); > if (ret) { > DRM_ERROR("GPU recovery failed\n"); > intel_gpu_reset(dev_priv, ALL_ENGINES); > @@ -1852,7 +1852,7 @@ void i915_reset(struct drm_i915_private *dev_priv) > i915_queue_hangcheck(dev_priv); > > finish: > - i915_gem_reset_finish(dev_priv); > + i915_gem_reset_finish(dev_priv, ALL_ENGINES); > enable_irq(dev_priv->drm.irq); > > wakeup: > @@ -1871,11 +1871,79 @@ void i915_reset(struct drm_i915_private *dev_priv) > * > * Reset a specific GPU engine. Useful if a hang is detected. > * Returns zero on successful reset or otherwise an error code. > + * > + * Procedure is: > + * - identifies the request that caused the hang and it is dropped > + * - force engine to idle: this is done by issuing a reset request > + * - reset engine > + * - restart submissions to the engine > */ > int i915_reset_engine(struct intel_engine_cs *engine) > { > - /* FIXME: replace me with engine reset sequence */ > - return -ENODEV; > + int ret; > + struct drm_i915_private *dev_priv = engine->i915; > + struct i915_gpu_error *error = &dev_priv->gpu_error; > + > + GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, &error->flags)); > + > + DRM_DEBUG_DRIVER("resetting %s\n", engine->name); > + > + /* > + * We need to first idle the engine by issuing a reset request, > + * then perform soft reset and re-initialize hw state, for all of > + * this GT power need to be awake so ensure it does throughout the > + * process > + */ > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); Hmm, what path did we take to get here without taking rpm? I thought I had fixed the last offender. > + disable_irq(dev_priv->drm.irq); I am 99% certain that we don't need to disable_irq() now for per-engine reset... I'd keep it in the global reset as simple paranoia. > + ret = i915_gem_reset_prepare_engine(engine); > + if (ret) { > + DRM_ERROR("Previous reset failed - promote to full reset\n"); > + goto error; > + } > + > + /* > + * the request that caused the hang is stuck on elsp, identify the > + * active request and drop it, adjust head to skip the offending > + * request to resume executing remaining requests in the queue. > + */ Hmm. Interesting. This relies on i915_gem_retire_requests() (i.e. struct_mutex) to skip replaying innocent requests, but here we should be asserting that we do have the hung request. i.e. request = i915_gem_find_active_request(engine); if (!request) goto skip. Bonus points for tying that into i915_gem_reset_prepare_engine() so that we only seach for the active_request once. > + i915_gem_reset_engine(engine); Where does skip_request() get called? > + > + /* forcing engine to idle */ > + ret = intel_reset_engine_start(engine); > + if (ret) { > + DRM_ERROR("Failed to disable %s\n", engine->name); > + goto error; > + } > + > + /* finally, reset engine */ > + ret = intel_gpu_reset(dev_priv, intel_engine_flag(engine)); > + if (ret) { > + DRM_ERROR("Failed to reset %s, ret=%d\n", engine->name, ret); > + intel_reset_engine_cancel(engine); > + goto error; > + } > + > + /* be sure the request reset bit gets cleared */ > + intel_reset_engine_cancel(engine); > + > + i915_gem_reset_finish_engine(engine); > + > + /* replay remaining requests in the queue */ > + ret = engine->init_hw(engine); > + if (ret) > + goto error; > + > +wakeup: > + enable_irq(dev_priv->drm.irq); > + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > + wake_up_bit(&error->flags, I915_RESET_HANDOFF); No handoff here anymore. > + return ret; > + > +error: > + /* use full gpu reset to recover on error */ > + goto wakeup; > }
On Wed, Apr 19, 2017 at 11:49:26AM +0100, Chris Wilson wrote: > On Tue, Apr 18, 2017 at 01:23:20PM -0700, Michel Thierry wrote: > > + ret = i915_gem_reset_prepare_engine(engine); > > + if (ret) { > > + DRM_ERROR("Previous reset failed - promote to full reset\n"); > > + goto error; > > + } > > + > > + /* > > + * the request that caused the hang is stuck on elsp, identify the > > + * active request and drop it, adjust head to skip the offending > > + * request to resume executing remaining requests in the queue. > > + */ > > Hmm. Interesting. This relies on i915_gem_retire_requests() (i.e. > struct_mutex) to skip replaying innocent requests, but here we should be > asserting that we do have the hung request. > > i.e. > request = i915_gem_find_active_request(engine); > if (!request) > goto skip. Forgot to mention this should include a "pardoned" check, i.e. that the active request still matches the watchdog seqno. -Chris
On 19/04/17 03:49, Chris Wilson wrote: > On Tue, Apr 18, 2017 at 01:23:20PM -0700, Michel Thierry wrote: >> From: Arun Siluvery <arun.siluvery@linux.intel.com> >> >> This change implements support for per-engine reset as an initial, less >> intrusive hang recovery option to be attempted before falling back to the >> legacy full GPU reset recovery mode if necessary. This is only supported >> from Gen8 onwards. >> >> Hangchecker determines which engines are hung and invokes error handler to >> recover from it. Error handler schedules recovery for each of those engines >> that are hung. The recovery procedure is as follows, >> - identifies the request that caused the hang and it is dropped >> - force engine to idle: this is done by issuing a reset request >> - reset and re-init engine >> - restart submissions to the engine >> >> If engine reset fails then we fall back to heavy weight full gpu reset >> which resets all engines and reinitiazes complete state of HW and SW. >> >> v2: Rebase. >> v3: s/*engine_reset*/*reset_engine*/; freeze engine and irqs before >> calling i915_gem_reset_engine (Chris). >> v4: Rebase, modify i915_gem_reset_prepare to use a ring mask and >> reuse the function for reset_engine. >> v5: intel_reset_engine_start/cancel instead of request/unrequest_reset. >> v6: Clean up reset_engine function to not require mutex, i.e. no need to call >> revoke/restore_fences and _retire_requests (Chris). >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Signed-off-by: Tomas Elf <tomas.elf@intel.com> >> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 76 ++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/i915_drv.h | 12 +++- >> drivers/gpu/drm/i915/i915_gem.c | 97 +++++++++++++++++++-------------- >> drivers/gpu/drm/i915/i915_gem_request.c | 2 +- >> drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++ >> 5 files changed, 158 insertions(+), 49 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index e03d0643dbd6..634893cd93b3 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -1810,7 +1810,7 @@ void i915_reset(struct drm_i915_private *dev_priv) >> >> pr_notice("drm/i915: Resetting chip after gpu hang\n"); >> disable_irq(dev_priv->drm.irq); >> - ret = i915_gem_reset_prepare(dev_priv); >> + ret = i915_gem_reset_prepare(dev_priv, ALL_ENGINES); >> if (ret) { >> DRM_ERROR("GPU recovery failed\n"); >> intel_gpu_reset(dev_priv, ALL_ENGINES); >> @@ -1852,7 +1852,7 @@ void i915_reset(struct drm_i915_private *dev_priv) >> i915_queue_hangcheck(dev_priv); >> >> finish: >> - i915_gem_reset_finish(dev_priv); >> + i915_gem_reset_finish(dev_priv, ALL_ENGINES); >> enable_irq(dev_priv->drm.irq); >> >> wakeup: >> @@ -1871,11 +1871,79 @@ void i915_reset(struct drm_i915_private *dev_priv) >> * >> * Reset a specific GPU engine. Useful if a hang is detected. >> * Returns zero on successful reset or otherwise an error code. >> + * >> + * Procedure is: >> + * - identifies the request that caused the hang and it is dropped >> + * - force engine to idle: this is done by issuing a reset request >> + * - reset engine >> + * - restart submissions to the engine >> */ >> int i915_reset_engine(struct intel_engine_cs *engine) >> { >> - /* FIXME: replace me with engine reset sequence */ >> - return -ENODEV; >> + int ret; >> + struct drm_i915_private *dev_priv = engine->i915; >> + struct i915_gpu_error *error = &dev_priv->gpu_error; >> + >> + GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, &error->flags)); >> + >> + DRM_DEBUG_DRIVER("resetting %s\n", engine->name); >> + >> + /* >> + * We need to first idle the engine by issuing a reset request, >> + * then perform soft reset and re-initialize hw state, for all of >> + * this GT power need to be awake so ensure it does throughout the >> + * process >> + */ >> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > > Hmm, what path did we take to get here without taking rpm? I thought I > had fixed the last offender. > Too many rebases... As you say, this is no longer needed after 1604a86d08053 "drm/i915: Extend rpm wakelock during i915_handle_error()" >> + disable_irq(dev_priv->drm.irq); > > I am 99% certain that we don't need to disable_irq() now for per-engine > reset... I'd keep it in the global reset as simple paranoia. > 100% correct. >> + ret = i915_gem_reset_prepare_engine(engine); >> + if (ret) { >> + DRM_ERROR("Previous reset failed - promote to full reset\n"); >> + goto error; >> + } >> + >> + /* >> + * the request that caused the hang is stuck on elsp, identify the >> + * active request and drop it, adjust head to skip the offending >> + * request to resume executing remaining requests in the queue. >> + */ > > Hmm. Interesting. This relies on i915_gem_retire_requests() (i.e. > struct_mutex) to skip replaying innocent requests, but here we should be > asserting that we do have the hung request. > > i.e. > request = i915_gem_find_active_request(engine); > if (!request) > goto skip. > > Bonus points for tying that into i915_gem_reset_prepare_engine() so that > we only seach for the active_request once. > What about something like this? int i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) ... if (engine_stalled(engine)) { request = i915_gem_find_active_request(engine); + if (!request) + err = -ECANCELED; + if (request && request->fence.error == -EIO) err = -EIO; /* Previous reset failed! */ } and int i915_reset_engine(struct intel_engine_cs *engine) ... DRM_DEBUG_DRIVER("resetting %s\n", engine->name); ret = i915_gem_reset_prepare_engine(engine); - if (ret) { + if (ret == -ECANCELED) { + DRM_INFO("no active request found, skip reset\n"); + goto skip; + } else if (ret) { DRM_ERROR("Previous reset failed - promote to full reset\n"); goto out; ... +skip: + i915_gem_reset_finish_engine(engine); + goto out; ... I'll still have to keep the request (if found) and pass it to i915_gem_reset_engine, to avoid the extra find_active_request. >> + i915_gem_reset_engine(engine); > > Where does skip_request() get called? > Sorry, I didn't get this, skip_request happens under this path: i915_gem_reset_engine --> i915_gem_reset_request -----> if (guilty) >> + >> + /* forcing engine to idle */ >> + ret = intel_reset_engine_start(engine); >> + if (ret) { >> + DRM_ERROR("Failed to disable %s\n", engine->name); >> + goto error; >> + } >> + >> + /* finally, reset engine */ >> + ret = intel_gpu_reset(dev_priv, intel_engine_flag(engine)); >> + if (ret) { >> + DRM_ERROR("Failed to reset %s, ret=%d\n", engine->name, ret); >> + intel_reset_engine_cancel(engine); >> + goto error; >> + } >> + >> + /* be sure the request reset bit gets cleared */ >> + intel_reset_engine_cancel(engine); >> + >> + i915_gem_reset_finish_engine(engine); >> + >> + /* replay remaining requests in the queue */ >> + ret = engine->init_hw(engine); >> + if (ret) >> + goto error; >> + >> +wakeup: >> + enable_irq(dev_priv->drm.irq); >> + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); >> + wake_up_bit(&error->flags, I915_RESET_HANDOFF); > > No handoff here anymore. > All these are leftovers from the previous version. >> + return ret; >> + >> +error: >> + /* use full gpu reset to recover on error */ >> + goto wakeup; >> } >
On 20/04/17 17:17, Michel Thierry wrote: >> Hmm. Interesting. This relies on i915_gem_retire_requests() (i.e. >> struct_mutex) to skip replaying innocent requests, but here we should be >> asserting that we do have the hung request. >> >> i.e. >> request = i915_gem_find_active_request(engine); >> if (!request) >> goto skip. >> >> Bonus points for tying that into i915_gem_reset_prepare_engine() so that >> we only seach for the active_request once. >> Will this do it? https://patchwork.freedesktop.org/patch/152494/ (ignore the DRM_ERROR I still have to change) I'm not sure about reusing the active request in full-reset (what if we have more than one engine hung?). Thanks
On Mon, Apr 24, 2017 at 02:22:21PM -0700, Michel Thierry wrote: > > > On 20/04/17 17:17, Michel Thierry wrote: > >>Hmm. Interesting. This relies on i915_gem_retire_requests() (i.e. > >>struct_mutex) to skip replaying innocent requests, but here we should be > >>asserting that we do have the hung request. > >> > >>i.e. > >>request = i915_gem_find_active_request(engine); > >>if (!request) > >> goto skip. > >> > >>Bonus points for tying that into i915_gem_reset_prepare_engine() so that > >>we only seach for the active_request once. > >> > > Will this do it? > https://patchwork.freedesktop.org/patch/152494/ (ignore the > DRM_ERROR I still have to change) That's a little uglier than I was hoping for :( > I'm not sure about reusing the active request in full-reset (what if > we have more than one engine hung?). We can have more than one active request, just don't forget a if (i915_gem_request_completed(engine->hangcheck.hung_request)) /* skip the reset! */ -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e03d0643dbd6..634893cd93b3 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1810,7 +1810,7 @@ void i915_reset(struct drm_i915_private *dev_priv) pr_notice("drm/i915: Resetting chip after gpu hang\n"); disable_irq(dev_priv->drm.irq); - ret = i915_gem_reset_prepare(dev_priv); + ret = i915_gem_reset_prepare(dev_priv, ALL_ENGINES); if (ret) { DRM_ERROR("GPU recovery failed\n"); intel_gpu_reset(dev_priv, ALL_ENGINES); @@ -1852,7 +1852,7 @@ void i915_reset(struct drm_i915_private *dev_priv) i915_queue_hangcheck(dev_priv); finish: - i915_gem_reset_finish(dev_priv); + i915_gem_reset_finish(dev_priv, ALL_ENGINES); enable_irq(dev_priv->drm.irq); wakeup: @@ -1871,11 +1871,79 @@ void i915_reset(struct drm_i915_private *dev_priv) * * Reset a specific GPU engine. Useful if a hang is detected. * Returns zero on successful reset or otherwise an error code. + * + * Procedure is: + * - identifies the request that caused the hang and it is dropped + * - force engine to idle: this is done by issuing a reset request + * - reset engine + * - restart submissions to the engine */ int i915_reset_engine(struct intel_engine_cs *engine) { - /* FIXME: replace me with engine reset sequence */ - return -ENODEV; + int ret; + struct drm_i915_private *dev_priv = engine->i915; + struct i915_gpu_error *error = &dev_priv->gpu_error; + + GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, &error->flags)); + + DRM_DEBUG_DRIVER("resetting %s\n", engine->name); + + /* + * We need to first idle the engine by issuing a reset request, + * then perform soft reset and re-initialize hw state, for all of + * this GT power need to be awake so ensure it does throughout the + * process + */ + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); + + disable_irq(dev_priv->drm.irq); + ret = i915_gem_reset_prepare_engine(engine); + if (ret) { + DRM_ERROR("Previous reset failed - promote to full reset\n"); + goto error; + } + + /* + * the request that caused the hang is stuck on elsp, identify the + * active request and drop it, adjust head to skip the offending + * request to resume executing remaining requests in the queue. + */ + i915_gem_reset_engine(engine); + + /* forcing engine to idle */ + ret = intel_reset_engine_start(engine); + if (ret) { + DRM_ERROR("Failed to disable %s\n", engine->name); + goto error; + } + + /* finally, reset engine */ + ret = intel_gpu_reset(dev_priv, intel_engine_flag(engine)); + if (ret) { + DRM_ERROR("Failed to reset %s, ret=%d\n", engine->name, ret); + intel_reset_engine_cancel(engine); + goto error; + } + + /* be sure the request reset bit gets cleared */ + intel_reset_engine_cancel(engine); + + i915_gem_reset_finish_engine(engine); + + /* replay remaining requests in the queue */ + ret = engine->init_hw(engine); + if (ret) + goto error; + +wakeup: + enable_irq(dev_priv->drm.irq); + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); + wake_up_bit(&error->flags, I915_RESET_HANDOFF); + return ret; + +error: + /* use full gpu reset to recover on error */ + goto wakeup; } static int i915_pm_suspend(struct device *kdev) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7bc5f552add7..eb3e5bcda478 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3025,6 +3025,8 @@ extern bool intel_has_gpu_reset(struct drm_i915_private *dev_priv); extern void i915_reset(struct drm_i915_private *dev_priv); extern int i915_reset_engine(struct intel_engine_cs *engine); extern bool intel_has_reset_engine(struct drm_i915_private *dev_priv); +extern int intel_reset_engine_start(struct intel_engine_cs *engine); +extern void intel_reset_engine_cancel(struct intel_engine_cs *engine); extern int intel_guc_reset(struct drm_i915_private *dev_priv); extern void intel_engine_init_hangcheck(struct intel_engine_cs *engine); extern void intel_hangcheck_init(struct drm_i915_private *dev_priv); @@ -3413,7 +3415,6 @@ int __must_check i915_gem_set_global_seqno(struct drm_device *dev, u32 seqno); struct drm_i915_gem_request * i915_gem_find_active_request(struct intel_engine_cs *engine); - void i915_gem_retire_requests(struct drm_i915_private *dev_priv); static inline bool i915_reset_backoff(struct i915_gpu_error *error) @@ -3441,11 +3442,16 @@ static inline u32 i915_reset_count(struct i915_gpu_error *error) return READ_ONCE(error->reset_count); } -int i915_gem_reset_prepare(struct drm_i915_private *dev_priv); +int i915_gem_reset_prepare_engine(struct intel_engine_cs *engine); +int i915_gem_reset_prepare(struct drm_i915_private *dev_priv, + unsigned int engine_mask); void i915_gem_reset(struct drm_i915_private *dev_priv); -void i915_gem_reset_finish(struct drm_i915_private *dev_priv); +void i915_gem_reset_finish_engine(struct intel_engine_cs *engine); +void i915_gem_reset_finish(struct drm_i915_private *dev_priv, + unsigned int engine_mask); void i915_gem_set_wedged(struct drm_i915_private *dev_priv); bool i915_gem_unset_wedged(struct drm_i915_private *dev_priv); +void i915_gem_reset_engine(struct intel_engine_cs *engine); void i915_gem_init_mmio(struct drm_i915_private *i915); int __must_check i915_gem_init(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 33fb11cc5acc..bce38062f94e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2793,48 +2793,57 @@ static bool engine_stalled(struct intel_engine_cs *engine) return true; } -int i915_gem_reset_prepare(struct drm_i915_private *dev_priv) +/* Ensure irq handler finishes, and not run again. */ +int i915_gem_reset_prepare_engine(struct intel_engine_cs *engine) { - struct intel_engine_cs *engine; - enum intel_engine_id id; + struct drm_i915_gem_request *request; int err = 0; - /* Ensure irq handler finishes, and not run again. */ - for_each_engine(engine, dev_priv, id) { - struct drm_i915_gem_request *request; - - /* 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); - - /* Prevent request submission to the hardware until we have - * completed the reset in i915_gem_reset_finish(). If a request - * is completed by one engine, it may then queue a request - * to a second via its engine->irq_tasklet *just* as we are - * calling engine->init_hw() and also writing the ELSP. - * Turning off the engine->irq_tasklet until the reset is over - * prevents the race. - */ - tasklet_kill(&engine->irq_tasklet); - tasklet_disable(&engine->irq_tasklet); - if (engine->irq_seqno_barrier) - engine->irq_seqno_barrier(engine); + /* 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); + + /* Prevent request submission to the hardware until we have + * completed the reset in i915_gem_reset_finish(). If a request + * is completed by one engine, it may then queue a request + * to a second via its engine->irq_tasklet *just* as we are + * calling engine->init_hw() and also writing the ELSP. + * Turning off the engine->irq_tasklet until the reset is over + * prevents the race. + */ + tasklet_kill(&engine->irq_tasklet); + tasklet_disable(&engine->irq_tasklet); - if (engine_stalled(engine)) { - request = i915_gem_find_active_request(engine); - if (request && request->fence.error == -EIO) - err = -EIO; /* Previous reset failed! */ - } + if (engine->irq_seqno_barrier) + engine->irq_seqno_barrier(engine); + + if (engine_stalled(engine)) { + request = i915_gem_find_active_request(engine); + if (request && request->fence.error == -EIO) + err = -EIO; /* Previous reset failed! */ } + return err; +} + +int i915_gem_reset_prepare(struct drm_i915_private *dev_priv, + unsigned int engine_mask) +{ + struct intel_engine_cs *engine; + unsigned int tmp; + int err = 0; + + for_each_engine_masked(engine, dev_priv, engine_mask, tmp) + err = i915_gem_reset_prepare_engine(engine); + i915_gem_revoke_fences(dev_priv); return err; @@ -2920,7 +2929,7 @@ static bool i915_gem_reset_request(struct drm_i915_gem_request *request) return guilty; } -static void i915_gem_reset_engine(struct intel_engine_cs *engine) +void i915_gem_reset_engine(struct intel_engine_cs *engine) { struct drm_i915_gem_request *request; @@ -2966,16 +2975,22 @@ void i915_gem_reset(struct drm_i915_private *dev_priv) } } -void i915_gem_reset_finish(struct drm_i915_private *dev_priv) +void i915_gem_reset_finish_engine(struct intel_engine_cs *engine) +{ + tasklet_enable(&engine->irq_tasklet); + kthread_unpark(engine->breadcrumbs.signaler); +} + +void i915_gem_reset_finish(struct drm_i915_private *dev_priv, + unsigned int engine_mask) { struct intel_engine_cs *engine; - enum intel_engine_id id; + unsigned int tmp; lockdep_assert_held(&dev_priv->drm.struct_mutex); - for_each_engine(engine, dev_priv, id) { - tasklet_enable(&engine->irq_tasklet); - kthread_unpark(engine->breadcrumbs.signaler); + for_each_engine_masked(engine, dev_priv, engine_mask, tmp) { + i915_gem_reset_finish_engine(engine); } } diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c index 095cccc2e8b2..085405e55a29 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.c +++ b/drivers/gpu/drm/i915/i915_gem_request.c @@ -1205,7 +1205,7 @@ long i915_wait_request(struct drm_i915_gem_request *req, return timeout; } -static void engine_retire_requests(struct intel_engine_cs *engine) +void engine_retire_requests(struct intel_engine_cs *engine) { struct drm_i915_gem_request *request, *next; u32 seqno = intel_engine_get_seqno(engine); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index ab5bdd110ac3..3ebba6b2dd74 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1801,6 +1801,26 @@ int intel_guc_reset(struct drm_i915_private *dev_priv) return ret; } +/* + * On gen8+ a reset request has to be issued via the reset control register + * before a GPU engine can be reset in order to stop the command streamer + * and idle the engine. This replaces the legacy way of stopping an engine + * by writing to the stop ring bit in the MI_MODE register. + */ +int intel_reset_engine_start(struct intel_engine_cs *engine) +{ + return gen8_reset_engine_start(engine); +} + +/* + * It is possible to back off from a previously issued reset request by simply + * clearing the reset request bit in the reset control register. + */ +void intel_reset_engine_cancel(struct intel_engine_cs *engine) +{ + gen8_reset_engine_cancel(engine); +} + bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv) { return check_for_unclaimed_mmio(dev_priv);