From patchwork Mon Jan 11 09:16:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 8000121 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 956819F32E for ; Mon, 11 Jan 2016 09:20:44 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 575D0201FE for ; Mon, 11 Jan 2016 09:20:43 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 0E1CC20204 for ; Mon, 11 Jan 2016 09:20:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C2F136E250; Mon, 11 Jan 2016 01:20:38 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by gabe.freedesktop.org (Postfix) with ESMTPS id B83EE6E228 for ; Mon, 11 Jan 2016 01:19:48 -0800 (PST) Received: by mail-wm0-f66.google.com with SMTP id u188so25292185wmu.0 for ; Mon, 11 Jan 2016 01:19:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=i90AlE5sa8cNvSm0oKdLELYi8xE4k/MH6CLrjE98dIs=; b=C8nEQYvL4B0aPyYWhOTsL6dMnomUp3Ou5Wns4ItxmMXn0SAnbQTbWntzfMo7s7pSA8 JDyTyjHpw9zsK8fROcPJMeLxU5U/n3WIJ1iKS7TXWwRqoON4/nBBU3eZc34J3w4JnTYg 9LfF2Hxt45GxrHo3qT+W/Jsx9LQWTvd20X/+0xHFnxvzYCRAY6cLWAkxX01KF9h6r3CE QwE5MjuFgTaKb7idmHPIgyAH4T8oZf9KdJcTkmitF/n/v2WkhsBwxjA4MRF6hqm6M+in E0YmeDzFt+2TZ/eF/4Pw018Ahjx316DrJ4rbOIAr5VpJYD9VPBN6/jd/zRLZpbjhwTCL /Xdg== X-Received: by 10.28.177.10 with SMTP id a10mr12202142wmf.4.1452503987526; Mon, 11 Jan 2016 01:19:47 -0800 (PST) Received: from haswell.alporthouse.com ([78.156.65.138]) by smtp.gmail.com with ESMTPSA id v2sm11834679wmv.12.2016.01.11.01.19.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 11 Jan 2016 01:19:46 -0800 (PST) From: Chris Wilson To: intel-gfx@lists.freedesktop.org Date: Mon, 11 Jan 2016 09:16:23 +0000 Message-Id: <1452503961-14837-12-git-send-email-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.7.0.rc3 In-Reply-To: <1452503961-14837-1-git-send-email-chris@chris-wilson.co.uk> References: <1452503961-14837-1-git-send-email-chris@chris-wilson.co.uk> Cc: Daniel Vetter Subject: [Intel-gfx] [PATCH 012/190] drm/i915: Prevent leaking of -EIO from i915_wait_request() 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=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,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 Reporting -EIO from i915_wait_request() has proven very troublematic over the years, with numerous hard-to-reproduce bugs cropping up in the corner case of where a reset occurs and the code wasn't expecting such an error. If the we reset the GPU or have detected a hang and wish to reset the GPU, the request is forcibly complete and the wait broken. Currently, we report either -EAGAIN or -EIO in order for the caller to retreat and restart the wait (if appropriate) after dropping and then reacquiring the struct_mutex (essential to allow the GPU reset to proceed). However, if we take the view that the request is complete (no further work will be done on it by the GPU because it is dead and soon to be reset), then we can proceed with the task at hand and then drop the struct_mutex allowing the reset to occur. This transfers the burden of checking whether it is safe to proceed to the caller, which in all but one instance it is safe - completely eliminating the source of all spurious -EIO. Of note, we only have two API entry points where we expect that userspace can observe an EIO. First is when submitting an execbuf, if the GPU is terminally wedged, then the operation cannot succeed and an -EIO is reported. Secondly, existing userspace uses the throttle ioctl to detect an already wedged GPU before starting using HW acceleration (or to confirm that the GPU is wedged after an error condition). So if the GPU is wedged when the user calls throttle, also report -EIO. v2: Split more carefully the change to i915_wait_request() and assorted ABI from the reset handling. v3: Add a couple of WARN_ON(EIO) to the interruptible modesetting code so that we don't start to leak EIO there in future (and break our hang resistant modesetting). Signed-off-by: Chris Wilson Cc: Daniel Vetter Reviewed-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h | 2 -- drivers/gpu/drm/i915/i915_gem.c | 44 ++++++++++++++++----------------- drivers/gpu/drm/i915/i915_gem_userptr.c | 6 ++--- drivers/gpu/drm/i915/intel_display.c | 13 +++++----- drivers/gpu/drm/i915/intel_lrc.c | 2 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- 6 files changed, 32 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f74bca326b79..bbdb056d2a8e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2978,8 +2978,6 @@ i915_gem_find_active_request(struct intel_engine_cs *ring); bool i915_gem_retire_requests(struct drm_device *dev); void i915_gem_retire_requests_ring(struct intel_engine_cs *ring); -int __must_check i915_gem_check_wedge(struct i915_gpu_error *error, - bool interruptible); static inline u32 i915_reset_counter(struct i915_gpu_error *error) { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 56069bdada85..f570990f03e0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -206,11 +206,10 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj) BUG_ON(obj->madv == __I915_MADV_PURGED); ret = i915_gem_object_set_to_cpu_domain(obj, true); - if (ret) { + if (WARN_ON(ret)) { /* In the event of a disaster, abandon all caches and * hope for the best. */ - WARN_ON(ret != -EIO); obj->base.read_domains = obj->base.write_domain = I915_GEM_DOMAIN_CPU; } @@ -1104,15 +1103,13 @@ put_rpm: return ret; } -int -i915_gem_check_wedge(struct i915_gpu_error *error, - bool interruptible) +static int +i915_gem_check_wedge(unsigned reset_counter, bool interruptible) { - if (i915_reset_in_progress_or_wedged(error)) { - /* Recovery complete, but the reset failed ... */ - if (i915_terminally_wedged(error)) - return -EIO; + if (__i915_terminally_wedged(reset_counter)) + return -EIO; + if (__i915_reset_in_progress(reset_counter)) { /* Non-interruptible callers can't handle -EAGAIN, hence return * -EIO unconditionally for these. */ if (!interruptible) @@ -1283,13 +1280,14 @@ int __i915_wait_request(struct drm_i915_gem_request *req, prepare_to_wait(&ring->irq_queue, &wait, state); /* We need to check whether any gpu reset happened in between - * the caller grabbing the seqno and now ... */ + * the request being submitted and now. If a reset has occurred, + * the request is effectively complete (we either are in the + * process of or have discarded the rendering and completely + * reset the GPU. The results of the request are lost and we + * are free to continue on with the original operation. + */ if (req->reset_counter != i915_reset_counter(&dev_priv->gpu_error)) { - /* ... but upgrade the -EAGAIN to an -EIO if the gpu - * is truely gone. */ - ret = i915_gem_check_wedge(&dev_priv->gpu_error, interruptible); - if (ret == 0) - ret = -EAGAIN; + ret = 0; break; } @@ -2162,11 +2160,10 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) BUG_ON(obj->madv == __I915_MADV_PURGED); ret = i915_gem_object_set_to_cpu_domain(obj, true); - if (ret) { + if (WARN_ON(ret)) { /* In the event of a disaster, abandon all caches and * hope for the best. */ - WARN_ON(ret != -EIO); i915_gem_clflush_object(obj, true); obj->base.read_domains = obj->base.write_domain = I915_GEM_DOMAIN_CPU; } @@ -2686,8 +2683,11 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring, *req_out = NULL; - ret = i915_gem_check_wedge(&dev_priv->gpu_error, - dev_priv->mm.interruptible); + /* ABI: Before userspace accesses the GPU (e.g. execbuffer), report + * EIO if the GPU is already wedged, or EAGAIN to drop the struct_mutex + * and restart. + */ + ret = i915_gem_check_wedge(reset_counter, dev_priv->mm.interruptible); if (ret) return ret; @@ -4088,9 +4088,9 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file) if (ret) return ret; - ret = i915_gem_check_wedge(&dev_priv->gpu_error, false); - if (ret) - return ret; + /* ABI: return -EIO if already wedged */ + if (i915_terminally_wedged(&dev_priv->gpu_error)) + return -EIO; spin_lock(&file_priv->mm.lock); list_for_each_entry(request, &file_priv->mm.request_list, client_list) { diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index 19fb0bddc1cd..1a5f89dba4af 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -81,10 +81,8 @@ static void __cancel_userptr__worker(struct work_struct *work) was_interruptible = dev_priv->mm.interruptible; dev_priv->mm.interruptible = false; - list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) { - int ret = i915_vma_unbind(vma); - WARN_ON(ret && ret != -EIO); - } + list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) + WARN_ON(i915_vma_unbind(vma)); WARN_ON(i915_gem_object_put_pages(obj)); dev_priv->mm.interruptible = was_interruptible; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ee0ec72b16b4..7e36f85d3109 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13516,11 +13516,9 @@ static int intel_atomic_prepare_commit(struct drm_device *dev, ret = __i915_wait_request(intel_plane_state->wait_req, true, NULL, NULL); - - /* Swallow -EIO errors to allow updates during hw lockup. */ - if (ret == -EIO) - ret = 0; if (ret) { + /* Any hang should be swallowed by the wait */ + WARN_ON(ret == -EIO); mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state); mutex_unlock(&dev->struct_mutex); @@ -13889,10 +13887,11 @@ intel_prepare_plane_fb(struct drm_plane *plane, */ if (needs_modeset(crtc_state)) ret = i915_gem_object_wait_rendering(old_obj, true); - - /* Swallow -EIO errors to allow updates during hw lockup. */ - if (ret && ret != -EIO) + if (ret) { + /* GPU hangs should have been swallowed by the wait */ + WARN_ON(ret == -EIO); return ret; + } } /* For framebuffer backed by dmabuf, wait for fence */ diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 3b436eb86ac7..32644338e6f8 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1004,7 +1004,7 @@ void intel_logical_ring_stop(struct intel_engine_cs *ring) return; ret = intel_ring_idle(ring); - if (ret && !i915_reset_in_progress_or_wedged(&to_i915(ring->dev)->gpu_error)) + if (ret) DRM_ERROR("failed to quiesce %s whilst cleaning up: %d\n", ring->name, ret); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 15121f3fd4f7..99780b674311 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -3062,7 +3062,7 @@ intel_stop_ring_buffer(struct intel_engine_cs *ring) return; ret = intel_ring_idle(ring); - if (ret && !i915_reset_in_progress_or_wedged(&to_i915(ring->dev)->gpu_error)) + if (ret) DRM_ERROR("failed to quiesce %s whilst cleaning up: %d\n", ring->name, ret);