I915_RESET_IN_PROGRESS is being used for both signaling the requirement to i915_mutex_lock_interruptible() to avoid taking the struct_mutex and to instruct a waiter (already holding the struct_mutex) to perform the reset. To allow for a little more coordination, split these two meaning into a couple of distinct flags. I915_RESET_BACKOFF tells i915_mutex_lock_interruptible() not to acquire the mutex and I915_RESET_HANDOFF tells the waiter to call i915_reset().
Signed-off-by: Chris Wilson <[email protected]> --- drivers/gpu/drm/i915/i915_debugfs.c | 16 +++++----- drivers/gpu/drm/i915/i915_drv.c | 7 +++-- drivers/gpu/drm/i915/i915_drv.h | 40 +++++++++++++++++++++--- drivers/gpu/drm/i915/i915_gem.c | 5 +-- drivers/gpu/drm/i915/i915_gem_request.c | 2 +- drivers/gpu/drm/i915/i915_irq.c | 38 ++++------------------ drivers/gpu/drm/i915/intel_display.c | 4 +-- drivers/gpu/drm/i915/selftests/intel_hangcheck.c | 18 +++++++---- 8 files changed, 71 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 9db6b041a799..5fdf8c137235 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1305,16 +1305,18 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused) enum intel_engine_id id; if (test_bit(I915_WEDGED, &dev_priv->gpu_error.flags)) - seq_printf(m, "Wedged\n"); - if (test_bit(I915_RESET_IN_PROGRESS, &dev_priv->gpu_error.flags)) - seq_printf(m, "Reset in progress\n"); + seq_puts(m, "Wedged\n"); + if (test_bit(I915_RESET_BACKOFF, &dev_priv->gpu_error.flags)) + seq_puts(m, "Reset in progress: struct_mutex backoff\n"); + if (test_bit(I915_RESET_HANDOFF, &dev_priv->gpu_error.flags)) + seq_puts(m, "Reset in progress: reset handoff to waiter\n"); if (waitqueue_active(&dev_priv->gpu_error.wait_queue)) - seq_printf(m, "Waiter holding struct mutex\n"); + seq_puts(m, "Waiter holding struct mutex\n"); if (waitqueue_active(&dev_priv->gpu_error.reset_queue)) - seq_printf(m, "struct_mutex blocked for reset\n"); + seq_puts(m, "struct_mutex blocked for reset\n"); if (!i915.enable_hangcheck) { - seq_printf(m, "Hangcheck disabled\n"); + seq_puts(m, "Hangcheck disabled\n"); return 0; } @@ -4121,7 +4123,7 @@ i915_wedged_set(void *data, u64 val) * while it is writing to 'i915_wedged' */ - if (i915_reset_in_progress(&dev_priv->gpu_error)) + if (i915_reset_backoff(&dev_priv->gpu_error)) return -EAGAIN; i915_handle_error(dev_priv, val, diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9164167cd147..be3c81221d11 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1815,8 +1815,9 @@ void i915_reset(struct drm_i915_private *dev_priv) int ret; lockdep_assert_held(&dev_priv->drm.struct_mutex); + GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, &error->flags)); - if (!test_and_clear_bit(I915_RESET_IN_PROGRESS, &error->flags)) + if (!test_bit(I915_RESET_HANDOFF, &error->flags)) return; /* Clear any previous failed attempts at recovery. Time to try again. */ @@ -1869,7 +1870,9 @@ void i915_reset(struct drm_i915_private *dev_priv) wakeup: i915_gem_reset_finish(dev_priv); enable_irq(dev_priv->drm.irq); - wake_up_bit(&error->flags, I915_RESET_IN_PROGRESS); + + clear_bit(I915_RESET_HANDOFF, &error->flags); + wake_up_bit(&error->flags, I915_RESET_HANDOFF); return; error: diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5156bcc59dea..0b02a6d710e1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1595,8 +1595,33 @@ struct i915_gpu_error { */ unsigned long reset_count; + /** + * flags: Control various stages of the GPU reset + * + * #I915_RESET_BACKOFF - When we start a reset, we want to stop any + * other users acquiring the struct_mutex. To do this we set the + * #I915_RESET_BACKOFF bit in the error flags when we detect a reset + * and then check for that bit before acquiring the struct_mutex (in + * i915_mutex_lock_interruptible()?). I915_RESET_BACKOFF serves a + * secondary role in preventing two concurrent global reset attempts. + * + * #I915_RESET_HANDOFF - To perform the actual GPU reset, we need the + * struct_mutex. We try to acquire the struct_mutex in the reset worker, + * but it may be held by some long running waiter (that we cannot + * interrupt without causing trouble). Once we are ready to do the GPU + * reset, we set the I915_RESET_HANDOFF bit and wakeup any waiters. If + * they already hold the struct_mutex and want to participate they can + * inspect the bit and do the reset directly, otherwise the worker + * waits for the struct_mutex. + * + * #I915_WEDGED - If reset fails and we can no longer use the GPU, + * we set the #I915_WEDGED bit. Prior to command submission, e.g. + * i915_gem_request_alloc(), this bit is checked and the sequence + * aborted (with -EIO reported to userspace) if set. + */ unsigned long flags; -#define I915_RESET_IN_PROGRESS 0 +#define I915_RESET_BACKOFF 0 +#define I915_RESET_HANDOFF 1 #define I915_WEDGED (BITS_PER_LONG - 1) /** @@ -3387,9 +3412,14 @@ 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_in_progress(struct i915_gpu_error *error) +static inline bool i915_reset_backoff(struct i915_gpu_error *error) +{ + return unlikely(test_bit(I915_RESET_BACKOFF, &error->flags)); +} + +static inline bool i915_reset_handoff(struct i915_gpu_error *error) { - return unlikely(test_bit(I915_RESET_IN_PROGRESS, &error->flags)); + return unlikely(test_bit(I915_RESET_HANDOFF, &error->flags)); } static inline bool i915_terminally_wedged(struct i915_gpu_error *error) @@ -3397,9 +3427,9 @@ static inline bool i915_terminally_wedged(struct i915_gpu_error *error) return unlikely(test_bit(I915_WEDGED, &error->flags)); } -static inline bool i915_reset_in_progress_or_wedged(struct i915_gpu_error *error) +static inline bool i915_reset_backoff_or_wedged(struct i915_gpu_error *error) { - return i915_reset_in_progress(error) | i915_terminally_wedged(error); + return i915_reset_backoff(error) | i915_terminally_wedged(error); } static inline u32 i915_reset_count(struct i915_gpu_error *error) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index d87983ba536f..ecd1b038318a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -103,16 +103,13 @@ i915_gem_wait_for_error(struct i915_gpu_error *error) might_sleep(); - if (!i915_reset_in_progress(error)) - return 0; - /* * Only wait 10 seconds for the gpu reset to complete to avoid hanging * userspace. If it takes that long something really bad is going on and * we should simply try to bail out and fail as gracefully as possible. */ ret = wait_event_interruptible_timeout(error->reset_queue, - !i915_reset_in_progress(error), + !i915_reset_backoff(error), I915_RESET_TIMEOUT); if (ret == 0) { DRM_ERROR("Timed out waiting for the gpu reset to complete\n"); diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c index 1e1d9f2072cd..0e8d1010cecb 100644 --- a/drivers/gpu/drm/i915/i915_gem_request.c +++ b/drivers/gpu/drm/i915/i915_gem_request.c @@ -1012,7 +1012,7 @@ bool __i915_spin_request(const struct drm_i915_gem_request *req, static bool __i915_wait_request_check_and_reset(struct drm_i915_gem_request *request) { - if (likely(!i915_reset_in_progress(&request->i915->gpu_error))) + if (likely(!i915_reset_handoff(&request->i915->gpu_error))) return false; __set_current_state(TASK_RUNNING); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index e646c4eba65d..495e6a79cacf 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2631,22 +2631,6 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) return ret; } -static void i915_error_wake_up(struct drm_i915_private *dev_priv) -{ - /* - * Notify all waiters for GPU completion events that reset state has - * been changed, and that they need to restart their wait after - * checking for potential errors (and bail out to drop locks if there is - * a gpu reset pending so that i915_error_work_func can acquire them). - */ - - /* Wake up __wait_seqno, potentially holding dev->struct_mutex. */ - wake_up_all(&dev_priv->gpu_error.wait_queue); - - /* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */ - wake_up_all(&dev_priv->pending_flip_queue); -} - /** * i915_reset_and_wakeup - do process context error handling work * @dev_priv: i915 device private @@ -2676,6 +2660,9 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv) intel_runtime_pm_get(dev_priv); intel_prepare_reset(dev_priv); + set_bit(I915_RESET_HANDOFF, &dev_priv->gpu_error.flags); + wake_up_all(&dev_priv->gpu_error.wait_queue); + do { /* * All state reset _must_ be completed before we update the @@ -2690,7 +2677,7 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv) /* We need to wait for anyone holding the lock to wakeup */ } while (wait_on_bit_timeout(&dev_priv->gpu_error.flags, - I915_RESET_IN_PROGRESS, + I915_RESET_HANDOFF, TASK_UNINTERRUPTIBLE, HZ)); @@ -2705,6 +2692,7 @@ static void i915_reset_and_wakeup(struct drm_i915_private *dev_priv) * Note: The wake_up also serves as a memory barrier so that * waiters see the updated value of the dev_priv->gpu_error. */ + clear_bit(I915_RESET_BACKOFF, &dev_priv->gpu_error.flags); wake_up_all(&dev_priv->gpu_error.reset_queue); } @@ -2788,24 +2776,10 @@ void i915_handle_error(struct drm_i915_private *dev_priv, if (!engine_mask) return; - if (test_and_set_bit(I915_RESET_IN_PROGRESS, + if (test_and_set_bit(I915_RESET_BACKOFF, &dev_priv->gpu_error.flags)) return; - /* - * Wakeup waiting processes so that the reset function - * i915_reset_and_wakeup doesn't deadlock trying to grab - * various locks. By bumping the reset counter first, the woken - * processes will see a reset in progress and back off, - * releasing their locks and then wait for the reset completion. - * We must do this for _all_ gpu waiters that might hold locks - * that the reset work needs to acquire. - * - * Note: The wake_up also provides a memory barrier to ensure that the - * waiters see the updated value of the reset flags. - */ - i915_error_wake_up(dev_priv); - i915_reset_and_wakeup(dev_priv); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4b73513b46c1..5959c9b6dc97 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3639,7 +3639,7 @@ static bool abort_flip_on_reset(struct intel_crtc *crtc) { struct i915_gpu_error *error = &to_i915(crtc->base.dev)->gpu_error; - if (i915_reset_in_progress(error)) + if (i915_reset_backoff(error)) return true; if (crtc->reset_count != i915_reset_count(error)) @@ -10595,7 +10595,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, goto cleanup; intel_crtc->reset_count = i915_reset_count(&dev_priv->gpu_error); - if (i915_reset_in_progress_or_wedged(&dev_priv->gpu_error)) { + if (i915_reset_backoff_or_wedged(&dev_priv->gpu_error)) { ret = -EIO; goto unlock; } diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c index d4acee6730e9..6ec7c731a267 100644 --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c @@ -301,7 +301,8 @@ static int igt_global_reset(void *arg) /* Check that we can issue a global GPU reset */ - set_bit(I915_RESET_IN_PROGRESS, &i915->gpu_error.flags); + set_bit(I915_RESET_BACKOFF, &i915->gpu_error.flags); + set_bit(I915_RESET_HANDOFF, &i915->gpu_error.flags); mutex_lock(&i915->drm.struct_mutex); reset_count = i915_reset_count(&i915->gpu_error); @@ -314,7 +315,8 @@ static int igt_global_reset(void *arg) } mutex_unlock(&i915->drm.struct_mutex); - GEM_BUG_ON(test_bit(I915_RESET_IN_PROGRESS, &i915->gpu_error.flags)); + GEM_BUG_ON(test_bit(I915_RESET_HANDOFF, &i915->gpu_error.flags)); + clear_bit(I915_RESET_BACKOFF, &i915->gpu_error.flags); if (i915_terminally_wedged(&i915->gpu_error)) err = -EIO; @@ -330,7 +332,7 @@ static u32 fake_hangcheck(struct drm_i915_gem_request *rq) reset_count = i915_reset_count(&rq->i915->gpu_error); - set_bit(I915_RESET_IN_PROGRESS, &rq->i915->gpu_error.flags); + set_bit(I915_RESET_HANDOFF, &rq->i915->gpu_error.flags); wake_up_all(&rq->i915->gpu_error.wait_queue); return reset_count; @@ -357,7 +359,7 @@ static int igt_wait_reset(void *arg) /* Check that we detect a stuck waiter and issue a reset */ - set_bit(I915_RESET_IN_PROGRESS, &i915->gpu_error.flags); + set_bit(I915_RESET_BACKOFF, &i915->gpu_error.flags); mutex_lock(&i915->drm.struct_mutex); err = hang_init(&h, i915); @@ -388,8 +390,8 @@ static int igt_wait_reset(void *arg) err = timeout; goto out_rq; } - GEM_BUG_ON(test_bit(I915_RESET_IN_PROGRESS, &i915->gpu_error.flags)); + GEM_BUG_ON(test_bit(I915_RESET_HANDOFF, &i915->gpu_error.flags)); if (i915_reset_count(&i915->gpu_error) == reset_count) { pr_err("No GPU reset recorded!\n"); err = -EINVAL; @@ -402,6 +404,7 @@ static int igt_wait_reset(void *arg) hang_fini(&h); unlock: mutex_unlock(&i915->drm.struct_mutex); + clear_bit(I915_RESET_BACKOFF, &i915->gpu_error.flags); if (i915_terminally_wedged(&i915->gpu_error)) return -EIO; @@ -422,6 +425,7 @@ static int igt_reset_queue(void *arg) if (!igt_can_mi_store_dword_imm(i915)) return 0; + set_bit(I915_RESET_BACKOFF, &i915->gpu_error.flags); mutex_lock(&i915->drm.struct_mutex); err = hang_init(&h, i915); if (err) @@ -470,8 +474,9 @@ static int igt_reset_queue(void *arg) i915_reset(i915); - GEM_BUG_ON(test_bit(I915_RESET_IN_PROGRESS, + GEM_BUG_ON(test_bit(I915_RESET_HANDOFF, &i915->gpu_error.flags)); + if (prev->fence.error != -EIO) { pr_err("GPU reset not recorded on hanging request [fence.error=%d]!\n", prev->fence.error); @@ -514,6 +519,7 @@ static int igt_reset_queue(void *arg) hang_fini(&h); unlock: mutex_unlock(&i915->drm.struct_mutex); + clear_bit(I915_RESET_BACKOFF, &i915->gpu_error.flags); if (i915_terminally_wedged(&i915->gpu_error)) return -EIO; -- 2.11.0 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
