Attachment 48043 Details for Bug 36515 – Apply the big hammer to finish the fb before disabling it.

[patch] Apply the big hammer to finish the fb before disabling it.

0001-drm-i915-Finish-any-pending-operations-on-the-frameb.patch (text/plain), 4.29 KB, created by Chris Wilson on 2011-06-16 05:55:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2011-06-16 05:55:04 UTC

Size: 4.29 KB

patch

obsolete

>From f9414c39032d3aab9e2e41f9abbca7576d92a19d 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...
>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36515
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> drivers/gpu/drm/i915/intel_display.c |   61 +++++++++++++++++++++++----------
> 1 files changed, 42 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 81a9059..cedd53c 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -1923,6 +1923,29 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> 	return 0;
> }
> 
>+static void
>+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;
>+	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.
>+	 */
>+	ret = i915_gem_object_flush_gpu(obj);
>+	(void)ret;
>+}
>+
> static int
> intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> 		    struct drm_framebuffer *old_fb)
>@@ -1955,25 +1978,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_flush_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);
>@@ -2977,6 +2983,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);
> 
> 	if (crtc->fb) {
>-- 
>1.7.5.4
>

Actions: View | Splinter Review

Attachments on bug 36515: 45980 | 45981 | 45982 | 48039 | 48043 | 48066 | 48067

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.