From patchwork Mon Dec 3 11:25:05 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 1832561 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork1.kernel.org (Postfix) with ESMTP id BF1633FCA5 for ; Mon, 3 Dec 2012 11:26:50 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 802EDE5D26 for ; Mon, 3 Dec 2012 03:26:50 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from relay.fireflyinternet.com (relay1.fireflyinternet.com [217.160.24.105]) by gabe.freedesktop.org (Postfix) with ESMTP id 5A18CE616B for ; Mon, 3 Dec 2012 03:26:36 -0800 (PST) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=109.228.6.235; Received: from fireflyinternet.com (unverified [109.228.6.235]) by relay.fireflyinternet.com (FireflyRelay1) with ESMTP id 781938-2000100 for ; Mon, 03 Dec 2012 11:28:33 +0000 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.73.22; Received: from arrandale.alporthouse.com (unverified [78.156.73.22]) by fireflyinternet.com (Firefly Internet SMTP) with ESMTP id 125476473-1500050 for multiple; Mon, 03 Dec 2012 11:26:29 +0000 From: Chris Wilson To: intel-gfx@lists.freedesktop.org Date: Mon, 3 Dec 2012 11:25:05 +0000 Message-Id: <1354533905-2952-1-git-send-email-chris@chris-wilson.co.uk> X-Mailer: git-send-email 1.7.10.4 X-Originating-IP: 78.156.73.22 Cc: stable@vger.kernel.org Subject: [Intel-gfx] [PATCH] drm/i915: Close race between processing unpin task and queueing the flip X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+patchwork-intel-gfx=patchwork.kernel.org@lists.freedesktop.org Before queuing the flip but crucially after attaching the unpin-work to the crtc, we continue to setup the unpin-work. However, should the hardware fire early, we see the connected unpin-work and queue the task. The task then promptly runs and unpins the fb before we finish taking the required references or even pinning it... Havoc. To close the race, we use the flip-pending atomic to indicate when the flip is finally setup and enqueued. So during the flip-done processing, we can check more accurately whether the flip was expected. v2: Add the appropriate mb() to ensure that the writes to the page-flip worker are complete prior to marking it active and emitting the MI_FLIP. On the read side, the mb should be enforced by the spinlocks. Signed-off-by: Chris Wilson Cc: stable@vger.kernel.org --- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- drivers/gpu/drm/i915/i915_irq.c | 4 +++- drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 5 ++++- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 8afc0dd..e6a11ca 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -317,7 +317,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) seq_printf(m, "No flip due on pipe %c (plane %c)\n", pipe, plane); } else { - if (!work->pending) { + if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) { seq_printf(m, "Flip queued on pipe %c (plane %c)\n", pipe, plane); } else { @@ -328,7 +328,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) seq_printf(m, "Stall check enabled, "); else seq_printf(m, "Stall check waiting for page flip ioctl, "); - seq_printf(m, "%d prepares\n", work->pending); + seq_printf(m, "%d prepares\n", atomic_read(&work->pending)); if (work->old_fb_obj) { struct drm_i915_gem_object *obj = work->old_fb_obj; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 6cd3dc9..a4dc97f 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1466,7 +1466,9 @@ static void i915_pageflip_stall_check(struct drm_device *dev, int pipe) spin_lock_irqsave(&dev->event_lock, flags); work = intel_crtc->unpin_work; - if (work == NULL || work->pending || !work->enable_stall_check) { + if (work == NULL || + atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE || + !work->enable_stall_check) { /* Either the pending flip IRQ arrived, or we're too early. Don't check */ spin_unlock_irqrestore(&dev->event_lock, flags); return; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 78d12c4..e400075 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6929,7 +6929,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev, spin_lock_irqsave(&dev->event_lock, flags); work = intel_crtc->unpin_work; - if (work == NULL || !work->pending) { + if (work == NULL || atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) { spin_unlock_irqrestore(&dev->event_lock, flags); return; } @@ -6977,16 +6977,22 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane) to_intel_crtc(dev_priv->plane_to_crtc_mapping[plane]); unsigned long flags; + /* NB: An MMIO update of the plane base pointer will also + * generate a page-flip completion irq, i.e. every modeset + * is also accompanied by a spurious intel_prepare_page_flip(). + */ spin_lock_irqsave(&dev->event_lock, flags); - if (intel_crtc->unpin_work) { - if ((++intel_crtc->unpin_work->pending) > 1) - DRM_ERROR("Prepared flip multiple times\n"); - } else { - DRM_DEBUG_DRIVER("preparing flip with no unpin work?\n"); - } + if (intel_crtc->unpin_work) + atomic_inc_not_zero(&intel_crtc->unpin_work->pending); spin_unlock_irqrestore(&dev->event_lock, flags); } +inline static void intel_mark_page_flip_active(struct intel_crtc *intel_crtc) +{ + smp_mb__before_atomic_inc(); + atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING); +} + static int intel_gen2_queue_flip(struct drm_device *dev, struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -7020,6 +7026,8 @@ static int intel_gen2_queue_flip(struct drm_device *dev, intel_ring_emit(ring, fb->pitches[0]); intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset); intel_ring_emit(ring, 0); /* aux display base address, unused */ + + intel_mark_page_flip_active(intel_crtc); intel_ring_advance(ring); return 0; @@ -7060,6 +7068,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev, intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset); intel_ring_emit(ring, MI_NOOP); + intel_mark_page_flip_active(intel_crtc); intel_ring_advance(ring); return 0; @@ -7106,6 +7115,8 @@ static int intel_gen4_queue_flip(struct drm_device *dev, pf = 0; pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff; intel_ring_emit(ring, pf | pipesrc); + + intel_mark_page_flip_active(intel_crtc); intel_ring_advance(ring); return 0; @@ -7148,6 +7159,8 @@ static int intel_gen6_queue_flip(struct drm_device *dev, pf = 0; pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff; intel_ring_emit(ring, pf | pipesrc); + + intel_mark_page_flip_active(intel_crtc); intel_ring_advance(ring); return 0; @@ -7202,6 +7215,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev, intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode)); intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset); intel_ring_emit(ring, (MI_NOOP)); + + intel_mark_page_flip_active(intel_crtc); intel_ring_advance(ring); return 0; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 522061c..3915ca9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -401,7 +401,10 @@ struct intel_unpin_work { struct drm_i915_gem_object *old_fb_obj; struct drm_i915_gem_object *pending_flip_obj; struct drm_pending_vblank_event *event; - int pending; + atomic_t pending; +#define INTEL_FLIP_INACTIVE 0 +#define INTEL_FLIP_PENDING 1 +#define INTEL_FLIP_COMPLETE 2 bool enable_stall_check; };