From patchwork Tue Jul 21 13:58:50 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tomas Elf X-Patchwork-Id: 6836511 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id B4485C05AC for ; Tue, 21 Jul 2015 14:00:42 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0246F206A0 for ; Tue, 21 Jul 2015 14:00:38 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id B62CB20670 for ; Tue, 21 Jul 2015 14:00:32 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3D2BA6E937; Tue, 21 Jul 2015 07:00:32 -0700 (PDT) X-Original-To: Intel-GFX@lists.freedesktop.org Delivered-To: Intel-GFX@lists.freedesktop.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 7E4B16E937 for ; Tue, 21 Jul 2015 07:00:30 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP; 21 Jul 2015 07:00:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,516,1432623600"; d="scan'208";a="732757632" Received: from telf-linux2.isw.intel.com ([10.102.226.163]) by orsmga001.jf.intel.com with ESMTP; 21 Jul 2015 07:00:03 -0700 From: Tomas Elf To: Intel-GFX@Lists.FreeDesktop.Org Date: Tue, 21 Jul 2015 14:58:50 +0100 Message-Id: <1437487135-32520-8-git-send-email-tomas.elf@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1437487135-32520-1-git-send-email-tomas.elf@intel.com> References: <1437487135-32520-1-git-send-email-tomas.elf@intel.com> Subject: [Intel-gfx] [RFCv2 07/12] drm/i915: Fake lost context interrupts through forced CSB check. X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP A recurring issue during long-duration operations testing of concurrent rendering tasks with intermittent hangs is that context completion interrupts following engine resets are sometimes lost. This becomes a real problem since the hardware might have completed a previously hung context following a per-engine hang recovery and then gone idle somehow without sending an interrupt telling the driver about this. At this point the driver would be stuck waiting for context completion, thinking that the context is still active, even though the hardware would be idle and waiting for more work. The way this is solved is by periodically checking for context submission status inconsistencies. What this means is that the ID of the currently running context on a given engine is compared against the context ID in the EXECLIST_STATUS register of the respective engine. If the two do not match and if the state does not change over time it is assumed that an interrupt was missed and that the driver is now stuck in an inconsistent state. Following the decision that the driver and the hardware are irreversibly stuck in an inconsistent state on a certain engine, the presumably lost interrupt is faked by simply calling the execlist interrupt handler from a non-interrupt context. Even though interrupts might be lost that does not mean that the hardware does not always update the context status buffer (CSB) when appropriate, which means that any context state transitions would be captured there regardless of the interrupt being sent or not. By faking the lost interrupt the interrupt handler could act on the outstanding context status transition events in the CSB, e.g. a context completion event. In the case where the hardware would be idle but the driver would be waiting for completion, faking an interrupt and finding a context completion status event would cause the driver to remove the currently active request from the execlist queue and go idle - thereby reestablishing a consistent context submission status between the hardware and the driver. The way this is implemented is that the hang checker will always keep alive as long as there is outstanding work. Even if the enable_hangcheck flag is disabled one part of the hang checker will always keep alive and reschedule itself, only to scan for inconsistent context submission states on all engines. As long as the context submission status of the currently running context on a given engine is consistent the hang checker works as normal and schedules hang recoveries as expected. If the status is not consistent no hang recoveries will be scheduled since no context resubmission will be possible anyway, so there is no point in trying until the status becomes consistent again. Of course, if enough hangs on the same engine are detected without any change in consistency the hang checker will go straight for the full GPU reset so there is no chance of getting stuck in this state. It's worth keeping in mind that the watchdog timeout hang detection mechanism relies entirely on the per-engine hang recovery path. So if we have an inconsistent context submission status on the engine that the watchdog timeout has detected a hang there is no way to recover from that hang if the period hangchecker is turned off since the per-engine hang recovery cannot do its final context resubmission if the context submission status is inconsistent. That's why we need to make sure that there is always a thread alive that keeps an eye out for inconsistent context submission states, not only for the periodic hang checker but also for watchdog timeout. Finally, since a non-interrupt context thread could end up in the interrupt handler as part of the forced CSB checking there's the chance of a race condition between the interrupt handler and the ring init code since both update ring->next_context_status_buffer. Therefore we've had to update the interrupt handler so that it grabs the execlist spinlock before updating the variable. We've also had to make sure that the ring init code grabs the execlist spinlock before initing this variable. * v2: (Chris Wilson) Remove context submission status consistency pre-check from i915_hangcheck_elapsed() and turn it into a pre-check to per-engine reset. The following describes the change in philosphy in how context submission state inconsistencies are detected: Previously we would let the periodic hang checker ensure that there were no context submission status inconsistencies on any engine, at any point. If an inconsistency was detected in the per-engine hang recovery path we would back off and defer to the next hang check since per-engine hang recovery is not effective during inconsistent context submission states. What we do in this new version is to move the consistency pre-check from the hang checker to the earliest point in the per-engine hang recovery path. If we detect an inconsistency at that point we fake a potentially lost context event interrupt by forcing a CSB check. If there are outstanding events in the CSB these will be acted upon and hopefully that will bring the driver up to speed with the hardware. If the CSB check did not amount to anything it is concluded that the inconsistency is unresolvable and the per-engine hang recovery fails and promotes to full GPU reset instead. In the hang checker-based consistency checking we would check the inconsistency for a number of times to make sure the detected state was stable before attempting to rectify the situation. This is possible since hang checking is a recurring event. Having moved the consistency checking to the recovery path instead (i.e. a one-time, fire & forget-style event) it is assumed that the hang detection that brought on the hang recovery has detected a stable hang and therefore, if an inconsistency is detected at that point, the inconsistency must be stable and not the result of a momentary context state transition. Therefore, unlike in the hang checker case, at the very first indication of an inconsistent context submission status the interrupt is faked speculatively. If outstanding CSB events are found it is determined that the hang was in fact just a context submission status inconsistency and no hang recovery is done. If the inconsistency cannot be resolved the per-engine hang recovery is failed and the hang is promoted to full GPU reset instead. Signed-off-by: Tomas Elf Cc: Chris Wilson Cc: Mika Kuoppala --- drivers/gpu/drm/i915/i915_drv.c | 118 ++++++++++++++++++++++++++--------- drivers/gpu/drm/i915/i915_irq.c | 32 ++-------- drivers/gpu/drm/i915/intel_lrc.c | 67 ++++++++++++++++++-- drivers/gpu/drm/i915/intel_lrc.h | 2 +- drivers/gpu/drm/i915/intel_lrc_tdr.h | 3 + 5 files changed, 159 insertions(+), 63 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index c7ba64e..2ccb2e8 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -917,6 +917,71 @@ int i915_reset(struct drm_device *dev) return 0; } +static bool i915_gem_reset_engine_CSSC_precheck( + struct drm_i915_private *dev_priv, + struct intel_engine_cs *engine, + struct drm_i915_gem_request **req, + int *ret) +{ + bool precheck_ok = true; + enum context_submission_status status; + + WARN_ON(!ret); + + *ret = 0; + + status = intel_execlists_TDR_get_current_request(engine, req); + + /* + * If the hardware and driver states do not coincide + * or if there for some reason is no current context + * in the process of being submitted then bail out and + * try again. Do not proceed unless we have reliable + * current context state information. + */ + if (status == CONTEXT_SUBMISSION_STATUS_NONE_SUBMITTED) { + /* + * No work in flight. This state is possible to get + * into if calling the error handler directly from + * debugfs. Just do early exit and forget it happened. + */ + WARN(1, "No work in flight! Aborting recovery on %s\n", + engine->name); + + precheck_ok = false; + *ret = 0; + + } else if (status == CONTEXT_SUBMISSION_STATUS_INCONSISTENT) { + if (!intel_execlists_TDR_force_CSB_check(dev_priv, engine)) { + /* + * Context submission state is inconsistent and + * faking a context event IRQ did not help. + * Fail and promote to higher level of + * recovery! + */ + precheck_ok = false; + *ret = -EINVAL; + } else { + /* + * Rectifying the inconsistent context + * submission status helped! No reset required, + * just exit and move on! + */ + precheck_ok = false; + *ret = 0; + } + + } else if (status != CONTEXT_SUBMISSION_STATUS_OK) { + WARN(1, "Unexpected context submission status (%u) on %s\n", + status, engine->name); + + precheck_ok = false; + *ret = -EINVAL; + } + + return precheck_ok; +} + /** * i915_reset_engine - reset GPU engine after a hang * @engine: engine to reset @@ -958,20 +1023,17 @@ int i915_reset_engine(struct intel_engine_cs *engine) i915_gem_reset_ring_status(dev_priv, engine); if (i915.enable_execlists) { - enum context_submission_status status = - intel_execlists_TDR_get_current_request(engine, NULL); - /* - * If the hardware and driver states do not coincide - * or if there for some reason is no current context - * in the process of being submitted then bail out and - * try again. Do not proceed unless we have reliable - * current context state information. + * Check context submission status consistency (CSSC) before + * moving on. If the driver and hardware have different + * opinions about what is going on just fail and escalate to a + * higher form of hang recovery. */ - if (status != CONTEXT_SUBMISSION_STATUS_OK) { - ret = -EAGAIN; + if (!i915_gem_reset_engine_CSSC_precheck(dev_priv, + engine, + NULL, + &ret)) goto reset_engine_error; - } } ret = intel_ring_disable(engine); @@ -981,27 +1043,21 @@ int i915_reset_engine(struct intel_engine_cs *engine) } if (i915.enable_execlists) { - enum context_submission_status status; - bool inconsistent; - - status = intel_execlists_TDR_get_current_request(engine, - ¤t_request); - - inconsistent = (status != CONTEXT_SUBMISSION_STATUS_OK); - if (inconsistent) { - /* - * If we somehow have reached this point with - * an inconsistent context submission status then - * back out of the previously requested reset and - * retry later. - */ - WARN(inconsistent, - "Inconsistent context status on %s: %u\n", - engine->name, status); - - ret = -EAGAIN; + /* + * Get a hold of the currently executing context. + * + * Context submission status consistency is done implicitly so + * we might as well check it post-engine disablement since we + * get that option for free. Also, it's conceivable that the + * context submission state might have changed as part of the + * reset request on gen8+ so it's not completely devoid of + * value to do this. + */ + if (!i915_gem_reset_engine_CSSC_precheck(dev_priv, + engine, + ¤t_request, + &ret)) goto reenable_reset_engine_error; - } } /* Sample the current ring head position */ diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 5672a2c..22dd96c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -36,6 +36,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "intel_lrc_tdr.h" /** * DOC: interrupt handling @@ -1286,7 +1287,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv, ret = IRQ_HANDLED; if (tmp & (GT_CONTEXT_SWITCH_INTERRUPT << GEN8_RCS_IRQ_SHIFT)) - intel_lrc_irq_handler(&dev_priv->ring[RCS]); + intel_lrc_irq_handler(&dev_priv->ring[RCS], true); if (tmp & (GT_RENDER_USER_INTERRUPT << GEN8_RCS_IRQ_SHIFT)) notify_ring(&dev_priv->ring[RCS]); if (tmp & (GT_GEN8_RCS_WATCHDOG_INTERRUPT << GEN8_RCS_IRQ_SHIFT)) { @@ -1303,7 +1304,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv, } if (tmp & (GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT)) - intel_lrc_irq_handler(&dev_priv->ring[BCS]); + intel_lrc_irq_handler(&dev_priv->ring[BCS], true); if (tmp & (GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT)) notify_ring(&dev_priv->ring[BCS]); } else @@ -1317,7 +1318,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv, ret = IRQ_HANDLED; if (tmp & (GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT)) - intel_lrc_irq_handler(&dev_priv->ring[VCS]); + intel_lrc_irq_handler(&dev_priv->ring[VCS], true); if (tmp & (GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT)) notify_ring(&dev_priv->ring[VCS]); if (tmp & (GT_GEN8_VCS_WATCHDOG_INTERRUPT << GEN8_VCS1_IRQ_SHIFT)) { @@ -1334,7 +1335,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv, } if (tmp & (GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS2_IRQ_SHIFT)) - intel_lrc_irq_handler(&dev_priv->ring[VCS2]); + intel_lrc_irq_handler(&dev_priv->ring[VCS2], true); if (tmp & (GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT)) notify_ring(&dev_priv->ring[VCS2]); if (tmp & (GT_GEN8_VCS_WATCHDOG_INTERRUPT << GEN8_VCS2_IRQ_SHIFT)) { @@ -1360,7 +1361,7 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_i915_private *dev_priv, ret = IRQ_HANDLED; if (tmp & (GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT)) - intel_lrc_irq_handler(&dev_priv->ring[VECS]); + intel_lrc_irq_handler(&dev_priv->ring[VECS], true); if (tmp & (GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT)) notify_ring(&dev_priv->ring[VECS]); } else @@ -2381,27 +2382,6 @@ static void i915_error_work_func(struct work_struct *work) ret = i915_reset_engine(ring); - /* - * Execlist mode only: - * - * -EAGAIN means that between detecting a hang (and - * also determining that the currently submitted - * context is stable and valid) and trying to recover - * from the hang the current context changed state. - * This means that we are probably not completely hung - * after all. Just fail and retry by exiting all the - * way back and wait for the next hang detection. If we - * have a true hang on our hands then we will detect it - * again, otherwise we will continue like nothing - * happened. - */ - if (ret == -EAGAIN) { - DRM_ERROR("Reset of %s aborted due to " \ - "change in context submission " \ - "state - retrying!", ring->name); - ret = 0; - } - if (ret) { DRM_ERROR("Reset of %s failed! (%d)", ring->name, ret); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index ff9d27cb..476eff0 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -672,7 +672,7 @@ static bool execlists_check_remove_request(struct intel_engine_cs *ring, * Check the unread Context Status Buffers and manage the submission of new * contexts to the ELSP accordingly. */ -void intel_lrc_irq_handler(struct intel_engine_cs *ring) +int intel_lrc_irq_handler(struct intel_engine_cs *ring, bool do_lock) { struct drm_i915_private *dev_priv = ring->dev->dev_private; u32 status_pointer; @@ -684,13 +684,14 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring)); + if (do_lock) + spin_lock(&ring->execlist_lock); + read_pointer = ring->next_context_status_buffer; write_pointer = status_pointer & 0x07; if (read_pointer > write_pointer) write_pointer += 6; - spin_lock(&ring->execlist_lock); - while (read_pointer < write_pointer) { read_pointer++; status = I915_READ(RING_CONTEXT_STATUS_BUF(ring) + @@ -716,13 +717,16 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) if (submit_contexts != 0) execlists_context_unqueue(ring, false); - spin_unlock(&ring->execlist_lock); - WARN(submit_contexts > 2, "More than two context complete events?\n"); ring->next_context_status_buffer = write_pointer % 6; + if (do_lock) + spin_unlock(&ring->execlist_lock); + I915_WRITE(RING_CONTEXT_STATUS_PTR(ring), ((u32)ring->next_context_status_buffer & 0x07) << 8); + + return submit_contexts; } static int execlists_context_queue(struct intel_engine_cs *ring, @@ -1356,6 +1360,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long flags; I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask)); I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff); @@ -1364,7 +1369,11 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring) _MASKED_BIT_DISABLE(GFX_REPLAY_MODE) | _MASKED_BIT_ENABLE(GFX_RUN_LIST_ENABLE)); POSTING_READ(RING_MODE_GEN7(ring)); + + spin_lock_irqsave(&ring->execlist_lock, flags); ring->next_context_status_buffer = 0; + spin_unlock_irqrestore(&ring->execlist_lock, flags); + DRM_DEBUG_DRIVER("Execlists enabled for %s\n", ring->name); i915_hangcheck_reinit(ring); @@ -2586,3 +2595,51 @@ intel_execlists_TDR_get_current_request(struct intel_engine_cs *ring, return status; } + +/** + * execlists_TDR_force_CSB_check() - check CSB manually to act on pending + * context status events. + * + * @dev_priv: ... + * @engine: engine whose CSB is to be checked. + * + * Return: + * True: Consistency restored. + * False: Failed trying to restore consistency. + * + * In case we missed a context event interrupt we can fake this interrupt by + * acting on pending CSB events manually by calling this function. This is + * normally what would happen in interrupt context but that does not prevent us + * from calling it from a user thread. + */ +bool intel_execlists_TDR_force_CSB_check(struct drm_i915_private *dev_priv, + struct intel_engine_cs *engine) +{ + unsigned long flags; + bool hw_active; + int was_effective; + + hw_active = + (I915_READ(RING_EXECLIST_STATUS(engine)) & + EXECLIST_STATUS_CURRENT_ACTIVE_ELEMENT_STATUS) ? + true : false; + if (hw_active) { + u32 hw_context; + + hw_context = I915_READ(RING_EXECLIST_STATUS_CTX_ID(engine)); + WARN(hw_active, "Context (%x) executing on %s - " \ + "No need for faked IRQ!\n", + hw_context, engine->name); + return false; + } + + spin_lock_irqsave(&engine->execlist_lock, flags); + if (!(was_effective = intel_lrc_irq_handler(engine, false))) + DRM_ERROR("Forced CSB check of %s ineffective!\n", engine->name); + spin_unlock_irqrestore(&engine->execlist_lock, flags); + + wake_up_all(&engine->irq_queue); + + return !!was_effective; +} + diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index d2f497c..6fae3c8 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -88,7 +88,7 @@ int intel_execlists_submission(struct drm_device *dev, struct drm_file *file, u64 exec_start, u32 dispatch_flags); u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj); -void intel_lrc_irq_handler(struct intel_engine_cs *ring); +int intel_lrc_irq_handler(struct intel_engine_cs *ring, bool do_lock); void intel_execlists_retire_requests(struct intel_engine_cs *ring); int intel_execlists_read_tail(struct intel_engine_cs *ring, diff --git a/drivers/gpu/drm/i915/intel_lrc_tdr.h b/drivers/gpu/drm/i915/intel_lrc_tdr.h index 4520753..041c808 100644 --- a/drivers/gpu/drm/i915/intel_lrc_tdr.h +++ b/drivers/gpu/drm/i915/intel_lrc_tdr.h @@ -32,5 +32,8 @@ enum context_submission_status intel_execlists_TDR_get_current_request(struct intel_engine_cs *ring, struct drm_i915_gem_request **req); +bool intel_execlists_TDR_force_CSB_check(struct drm_i915_private *dev_priv, + struct intel_engine_cs *engine); + #endif /* _INTEL_LRC_TDR_H_ */