Attachment 59281 Details for Bug 45413 – Finish gpu before disabling pipe

[patch] Finish gpu before disabling pipe

0001-drm-i915-Finish-any-pending-operations-on-the-frameb.patch (text/plain), 4.66 KB, created by Chris Wilson on 2012-03-30 08:27:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2012-03-30 08:27:19 UTC

Size: 4.66 KB

patch

obsolete

>From fc63e2b88d5a8dcb887ed601d27276dd4b363987 Mon Sep 17 00:00:00 2001
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Date: Thu, 16 Jun 2011 12:59:17 +0100
>Subject: [PATCH] drm/i915: Finish any pending operations on the framebuffer
> before disabling
>
>Similar to the case where we are changing from one framebuffer to
>another, we need to be sure that there are no pending WAIT_FOR_EVENTs on
>the pipe for the current framebuffer before switching. If we disable the
>pipe, and then try to execute a WAIT_FOR_EVENT it will block
>indefinitely and cause a GPU hang.
>
>We attempted to fix this in commit 85345517fe6d4de27b0d6ca19fef9d28ac947c4a
>(drm/i915: Retire any pending operations on the old scanout when switching)
>for the case of mode switching, but this leaves the condition where we
>are switching off the pipe vulnerable.
>
>There still remains the race condition were a display may be unplugged,
>switched off by the core, a uevent sent to notify the DDX and the DDX
>may issue a WAIT_FOR_EVENT before it processes the uevent. This window
>does not exist if the pipe is only switched off in response to the
>uevent. Time to make sure that is so...
>
>Reported-by: Francis Leblanc <Francis.Leblanc-Lebeau@verint.com>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36515
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45413
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> drivers/gpu/drm/i915/intel_display.c |   65 ++++++++++++++++++++++++----------
> 1 files changed, 46 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 89f1af6..50fab82 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -2265,6 +2265,33 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> }
> 
> static int
>+intel_finish_fb(struct drm_framebuffer *old_fb)
>+{
>+	struct drm_i915_gem_object *obj = to_intel_framebuffer(old_fb)->obj;
>+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>+	bool was_interruptible = dev_priv->mm.interruptible;
>+	int ret;
>+
>+	wait_event(dev_priv->pending_flip_queue,
>+		   atomic_read(&dev_priv->mm.wedged) ||
>+		   atomic_read(&obj->pending_flip) == 0);
>+
>+	/* Big Hammer, we also need to ensure that any pending
>+	 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>+	 * current scanout is retired before unpinning the old
>+	 * framebuffer.
>+	 *
>+	 * This should only fail upon a hung GPU, in which case we
>+	 * can safely continue.
>+	 */
>+	dev_priv->mm.interruptible = false;
>+	ret = i915_gem_object_finish_gpu(obj);
>+	dev_priv->mm.interruptible = was_interruptible;
>+
>+	return ret;
>+}
>+
>+static int
> intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> 		    struct drm_framebuffer *old_fb)
> {
>@@ -2302,25 +2329,8 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> 		return ret;
> 	}
> 
>-	if (old_fb) {
>-		struct drm_i915_private *dev_priv = dev->dev_private;
>-		struct drm_i915_gem_object *obj = to_intel_framebuffer(old_fb)->obj;
>-
>-		wait_event(dev_priv->pending_flip_queue,
>-			   atomic_read(&dev_priv->mm.wedged) ||
>-			   atomic_read(&obj->pending_flip) == 0);
>-
>-		/* Big Hammer, we also need to ensure that any pending
>-		 * MI_WAIT_FOR_EVENT inside a user batch buffer on the
>-		 * current scanout is retired before unpinning the old
>-		 * framebuffer.
>-		 *
>-		 * This should only fail upon a hung GPU, in which case we
>-		 * can safely continue.
>-		 */
>-		ret = i915_gem_object_finish_gpu(obj);
>-		(void) ret;
>-	}
>+	if (old_fb)
>+		intel_finish_fb(old_fb);
> 
> 	ret = intel_pipe_set_base_atomic(crtc, crtc->fb, x, y,
> 					 LEAVE_ATOMIC_MODE_SET);
>@@ -3400,6 +3410,23 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
> 	struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> 	struct drm_device *dev = crtc->dev;
> 
>+	/* Flush any pending WAITs before we disable the pipe. Note that
>+	 * we need to drop the struct_mutex in order to acquire it again
>+	 * during the lowlevel dpms routines around a couple of the
>+	 * operations. It does not look trivial nor desirable to move
>+	 * that lokcing higher. So instead we leave a window for the
>+	 * submission of further commands on the fb before we can actually
>+	 * disable it. This race with userspace exists anyway, and we can
>+	 * only rely on the pipe being disabled by userspace after it
>+	 * receives the hotplug notification and has flushed any pending
>+	 * batches.
>+	 */
>+	if (crtc->fb) {
>+		mutex_lock(&dev->struct_mutex);
>+		intel_finish_fb(crtc->fb);
>+		mutex_unlock(&dev->struct_mutex);
>+	}
>+
> 	crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> 	assert_plane_disabled(dev->dev_private, to_intel_crtc(crtc)->plane);
> 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
>-- 
>1.7.9.1
>

Actions: View | Splinter Review

Attachments on bug 45413: 56342 | 56343 | 56344 | 56345 | 56889 | 57043 | 57172 | 57173 | 59279 | 59281 | 59288 | 59291

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.