Attachment 94763 Details for Bug 75502 – check that the pageflip is truly stuck before crying

[patch] check that the pageflip is truly stuck before crying

0001-drm-i915-Check-that-the-pageflip-is-truly-stuck-befo.patch (text/plain), 7.93 KB, created by Chris Wilson on 2014-02-26 10:21:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2014-02-26 10:21:21 UTC

Size: 7.93 KB

patch

obsolete

>From 99c6388ba1fb898337cacd32562136fe21ee856c Mon Sep 17 00:00:00 2001
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Date: Wed, 26 Feb 2014 10:01:29 +0000
>Subject: [PATCH] drm/i915: Check that the pageflip is truly stuck before
> declaring so
>
>A belt-and-braces approach to make sure the driver (or the hardware)
>doesn't miss an interrupt and cause us to stop updating the display
>should the unthinkable happen and the pageflip fail.
>
>In the past we used similar code to paper over races in our IRQ handling
>for pageflips on gen3. However, we now have similar reports that
>Sandybridge stops processing pageflips. In this case, the hardware
>generates a single interrupt for the completion of the mmio write to the
>DSPSURF and so we should not be loosing it due to a race. However, if
>userspace tries to flip again (before receiving the event notification
>from the last flip, for example due to a change in display servers)
>and the old flip is still marked as active, check the hardware state to
>see if it did update first. If the pageflip completed in hardware, we
>can simply update our bookkeeping and continue on with the new flip.
>
>This is a continuation of the effort started with
>commit 4e5359cd053bfb7d8dabe4a63624a5726848ffbc
>Author: Simon Farnsworth <simon.farnsworth@onelan.co.uk>
>Date:   Wed Sep 1 17:47:52 2010 +0100
>
>    drm/i915: Avoid pageflipping freeze when we miss the flip prepare interrupt
>
>Reported-by:  Simon Farnsworth <simon@farnz.org.uk>
>References: https://bugs.freedesktop.org/show_bug.cgi?id=75502
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>---
> drivers/gpu/drm/i915/i915_debugfs.c  |   34 +++++++++++++---
> drivers/gpu/drm/i915/intel_display.c |   72 ++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_drv.h     |    1 +
> 3 files changed, 97 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index 52cf0b6..563581f 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -581,6 +581,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> {
> 	struct drm_info_node *node = (struct drm_info_node *) m->private;
> 	struct drm_device *dev = node->minor->dev;
>+	struct drm_i915_private *dev_priv = dev->dev_private;
> 	unsigned long flags;
> 	struct intel_crtc *crtc;
> 
>@@ -602,6 +603,8 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> 				seq_printf(m, "Flip pending (waiting for vsync) on pipe %c (plane %c)\n",
> 					   pipe, plane);
> 			}
>+			seq_printf(m, "Flip queued on frame %d, now %d\n",
>+				   work->vblank, atomic_read(&dev->vblank[crtc->pipe].count));
> 			if (work->enable_stall_check)
> 				seq_puts(m, "Stall check enabled, ");
> 			else
>@@ -610,15 +613,34 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> 
> 			if (work->old_fb_obj) {
> 				struct drm_i915_gem_object *obj = work->old_fb_obj;
>-				if (obj)
>-					seq_printf(m, "Old framebuffer gtt_offset 0x%08lx\n",
>-						   i915_gem_obj_ggtt_offset(obj));
>+				seq_printf(m, "Old framebuffer gtt_offset 0x%08lx\n",
>+					   i915_gem_obj_ggtt_offset(obj));
> 			}
> 			if (work->pending_flip_obj) {
> 				struct drm_i915_gem_object *obj = work->pending_flip_obj;
>-				if (obj)
>-					seq_printf(m, "New framebuffer gtt_offset 0x%08lx\n",
>-						   i915_gem_obj_ggtt_offset(obj));
>+				bool complete;
>+
>+				seq_printf(m, "New framebuffer gtt_offset 0x%08lx\n",
>+					   i915_gem_obj_ggtt_offset(obj));
>+
>+				if (INTEL_INFO(dev)->gen >= 4) {
>+					int dspsurf = DSPSURF(crtc->plane);
>+					int x = crtc->base.x, y = crtc->base.y;
>+
>+					complete = I915_HI_DISPBASE(I915_READ(dspsurf)) ==
>+						i915_gem_obj_ggtt_offset(obj) +
>+						intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
>+									       crtc->base.fb->bits_per_pixel / 8,
>+									       crtc->base.fb->pitches[0]);
>+				} else {
>+					int dspaddr = DSPADDR(crtc->plane);
>+					complete = I915_READ(dspaddr) ==
>+						(i915_gem_obj_ggtt_offset(obj) +
>+						 crtc->base.y * crtc->base.fb->pitches[0] +
>+						 crtc->base.x * crtc->base.fb->bits_per_pixel/8);
>+				}
>+
>+				seq_printf(m, "MMIO update completed? %d\n", complete);
> 			}
> 		}
> 		spin_unlock_irqrestore(&dev->event_lock, flags);
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index e1992d8..c3a955b 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -8413,6 +8413,7 @@ inline static void intel_mark_page_flip_active(struct intel_crtc *intel_crtc)
> 	/* Ensure that the work item is consistent when activating it ... */
> 	smp_wmb();
> 	atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
>+	intel_crtc->unpin_work->vblank = atomic_read(&intel_crtc->base.dev->vblank[intel_crtc->pipe].count);
> 	/* and that it is marked active as soon as the irq could fire. */
> 	smp_wmb();
> }
>@@ -8701,6 +8702,46 @@ static int intel_default_queue_flip(struct drm_device *dev,
> 	return -ENODEV;
> }
> 
>+static bool intel_pageflip_stall_check(struct drm_device *dev,
>+				       struct drm_crtc *crtc,
>+				       struct intel_unpin_work *work)
>+{
>+	struct drm_i915_private *dev_priv = dev->dev_private;
>+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>+	struct drm_i915_gem_object *obj;
>+	bool stall_detected;
>+
>+	if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
>+		return true;
>+
>+	if (!work->enable_stall_check)
>+		return false;
>+
>+	if (intel_crtc->unpin_work->vblank == atomic_read(&intel_crtc->base.dev->vblank[intel_crtc->pipe].count))
>+		return false;
>+
>+	/* Potential stall - if we see that the flip has happened, assume a missed interrupt */
>+	obj = work->pending_flip_obj;
>+	if (INTEL_INFO(dev)->gen >= 4) {
>+		int dspsurf = DSPSURF(intel_crtc->plane);
>+		int x = crtc->x, y = crtc->y;
>+
>+		stall_detected = I915_HI_DISPBASE(I915_READ(dspsurf)) ==
>+			i915_gem_obj_ggtt_offset(obj) +
>+			intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
>+						       crtc->fb->bits_per_pixel / 8,
>+						       crtc->fb->pitches[0]);
>+	} else {
>+		int dspaddr = DSPADDR(intel_crtc->plane);
>+		stall_detected = I915_READ(dspaddr) ==
>+			(i915_gem_obj_ggtt_offset(obj) +
>+			 crtc->y * crtc->fb->pitches[0] +
>+			 crtc->x * crtc->fb->bits_per_pixel/8);
>+	}
>+
>+	return stall_detected;
>+}
>+
> static int intel_crtc_page_flip(struct drm_crtc *crtc,
> 				struct drm_framebuffer *fb,
> 				struct drm_pending_vblank_event *event,
>@@ -8747,12 +8788,35 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> 	/* We borrow the event spin lock for protecting unpin_work */
> 	spin_lock_irqsave(&dev->event_lock, flags);
> 	if (intel_crtc->unpin_work) {
>-		spin_unlock_irqrestore(&dev->event_lock, flags);
>-		kfree(work);
>+		bool stall;
>+
>+		/* Before declaring the flip queue wedged, check if
>+		 * the hardware completed the operation behind our backs.
>+		 */
>+		stall = intel_pageflip_stall_check(dev, crtc,
>+						   intel_crtc->unpin_work);
> 		drm_vblank_put(dev, intel_crtc->pipe);
> 
>-		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>-		return -EBUSY;
>+		if (stall) {
>+			DRM_DEBUG_DRIVER("flip queue: previous flip completed, continuing\n");
>+
>+			if (intel_crtc->unpin_work->event)
>+				drm_send_vblank_event(dev,
>+						      intel_crtc->pipe,
>+						      intel_crtc->unpin_work->event);
>+
>+			wake_up_all(&dev_priv->pending_flip_queue);
>+			queue_work(dev_priv->wq, &work->work);
>+
>+			trace_i915_flip_complete(intel_crtc->plane,
>+						 intel_crtc->unpin_work->pending_flip_obj);
>+		} else {
>+			DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>+			spin_unlock_irqrestore(&dev->event_lock, flags);
>+
>+			kfree(work);
>+			return -EBUSY;
>+		}
> 	}
> 	intel_crtc->unpin_work = work;
> 	spin_unlock_irqrestore(&dev->event_lock, flags);
>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>index e5e1a5c..e3dfd08 100644
>--- a/drivers/gpu/drm/i915/intel_drv.h
>+++ b/drivers/gpu/drm/i915/intel_drv.h
>@@ -562,6 +562,7 @@ struct intel_unpin_work {
> #define INTEL_FLIP_PENDING	1
> #define INTEL_FLIP_COMPLETE	2
> 	bool enable_stall_check;
>+	int vblank;
> };
> 
> struct intel_set_config {
>-- 
>1.7.9.5
>

Actions: View | Splinter Review

Attachments on bug 75502: 94724 | 94763 | 94883 | 95088 | 95792 | 95793 | 97218 | 97221 | 98285 | 98614

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.