* [PATCH 2/3] drm/i915: Decouple the stuck pageflip on modeset
2014-06-10 12:46 [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank Chris Wilson
@ 2014-06-10 12:46 ` Chris Wilson
2014-06-17 21:33 ` Daniel Vetter
2014-06-10 12:46 ` [PATCH 3/3] drm/i915: Boost GPU frequency if we detect outstanding pageflips Chris Wilson
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2014-06-10 12:46 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
If we successfully confuse the hardware, and cause it to drop a queued
pageflip, we wait for 60s and issue a warning before continuing on with
the modeset. However, this leaves the pending pageflip still stuck
indefinitely. Pretend to userspace that it does complete, and let us
start afresh following the modeset.
v2: Rebase after refactor
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ec166e41a27e..5261c145e633 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3363,9 +3363,19 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
- WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
- !intel_crtc_has_pending_flip(crtc),
- 60*HZ) == 0);
+ if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
+ !intel_crtc_has_pending_flip(crtc),
+ 60*HZ) == 0)) {
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->event_lock, flags);
+ if (intel_crtc->unpin_work) {
+ WARN_ONCE(1, "Removing stuck page flip\n");
+ page_flip_completed(intel_crtc);
+ }
+ spin_unlock_irqrestore(&dev->event_lock, flags);
+ }
mutex_lock(&dev->struct_mutex);
intel_finish_fb(crtc->primary->fb);
--
2.0.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] drm/i915: Decouple the stuck pageflip on modeset
2014-06-10 12:46 ` [PATCH 2/3] drm/i915: Decouple the stuck pageflip on modeset Chris Wilson
@ 2014-06-17 21:33 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-06-17 21:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx
On Tue, Jun 10, 2014 at 01:46:55PM +0100, Chris Wilson wrote:
> If we successfully confuse the hardware, and cause it to drop a queued
> pageflip, we wait for 60s and issue a warning before continuing on with
> the modeset. However, this leaves the pending pageflip still stuck
> indefinitely. Pretend to userspace that it does complete, and let us
> start afresh following the modeset.
>
> v2: Rebase after refactor
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Since I've shredded a few machines with this ...
Queued for -next, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ec166e41a27e..5261c145e633 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3363,9 +3363,19 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>
> WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
>
> - WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
> - !intel_crtc_has_pending_flip(crtc),
> - 60*HZ) == 0);
> + if (WARN_ON(wait_event_timeout(dev_priv->pending_flip_queue,
> + !intel_crtc_has_pending_flip(crtc),
> + 60*HZ) == 0)) {
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + if (intel_crtc->unpin_work) {
> + WARN_ONCE(1, "Removing stuck page flip\n");
> + page_flip_completed(intel_crtc);
> + }
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> + }
>
> mutex_lock(&dev->struct_mutex);
> intel_finish_fb(crtc->primary->fb);
> --
> 2.0.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/i915: Boost GPU frequency if we detect outstanding pageflips
2014-06-10 12:46 [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank Chris Wilson
2014-06-10 12:46 ` [PATCH 2/3] drm/i915: Decouple the stuck pageflip on modeset Chris Wilson
@ 2014-06-10 12:46 ` Chris Wilson
2014-06-17 21:16 ` Daniel Vetter
2014-06-10 13:46 ` [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank Ville Syrjälä
2014-06-11 10:47 ` [PATCH] " Chris Wilson
3 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2014-06-10 12:46 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
If we hit a vblank and see that have a pageflip queue but not yet
processed, ensure that the GPU is running at maximum in order to clear
the backlog. Pageflips are only queued for the following vblank, if we
miss it, there will be a visible stutter. Boosting the GPU frequency
doesn't prevent us from missing the target vblank, but it should help
the subsequent frames hitting theirs.
v2: Reorder vblank vs flip-complete so that we only check for a missed
flip after processing the completion events, and avoid spurious boosts.
v3: Rename missed_vblank
v4: Rebase
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 6 ++++++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
4 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 10dd80a12658..33ed0c6b8a9c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -910,6 +910,7 @@ struct intel_gen6_power_mgmt {
bool enabled;
struct delayed_work delayed_resume_work;
+ struct work_struct boost_work;
/*
* Protects RPS/RC6 register access and PCU communication.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5261c145e633..89a4d01fefb4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9326,6 +9326,7 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
unsigned long flags;
+ bool missed_vblank;
if (crtc == NULL)
return;
@@ -9336,7 +9337,12 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
page_flip_completed(intel_crtc);
}
+ missed_vblank = (intel_crtc->unpin_work != NULL &&
+ drm_vblank_count(dev, pipe) - intel_crtc->unpin_work->flip_queued_vblank > 1);
spin_unlock_irqrestore(&dev->event_lock, flags);
+
+ if (missed_vblank)
+ intel_queue_rps_boost(dev);
}
static int intel_crtc_page_flip(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 10665716eb10..2e3c65812e72 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -969,6 +969,7 @@ void ironlake_teardown_rc6(struct drm_device *dev);
void gen6_update_ring_freq(struct drm_device *dev);
void gen6_rps_idle(struct drm_i915_private *dev_priv);
void gen6_rps_boost(struct drm_i915_private *dev_priv);
+void intel_queue_rps_boost(struct drm_device *dev);
void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b06f896740b5..ab760e5abc9a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6722,6 +6722,19 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val)
return DIV_ROUND_CLOSEST(4 * mul * val, dev_priv->mem_freq) + 0xbd - 6;
}
+static void __intel_rps_boost_work(struct work_struct *work)
+{
+ gen6_rps_boost(container_of(work, struct drm_i915_private, rps.boost_work));
+}
+
+void intel_queue_rps_boost(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = to_i915(dev);
+
+ if (INTEL_INFO(dev)->gen >= 6)
+ queue_work(dev_priv->wq, &dev_priv->rps.boost_work);
+}
+
void intel_pm_setup(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -6730,6 +6743,8 @@ void intel_pm_setup(struct drm_device *dev)
INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
intel_gen6_powersave_work);
+ INIT_WORK(&dev_priv->rps.boost_work,
+ __intel_rps_boost_work);
dev_priv->pm.suspended = false;
dev_priv->pm.irqs_disabled = false;
--
2.0.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] drm/i915: Boost GPU frequency if we detect outstanding pageflips
2014-06-10 12:46 ` [PATCH 3/3] drm/i915: Boost GPU frequency if we detect outstanding pageflips Chris Wilson
@ 2014-06-17 21:16 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-06-17 21:16 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx
On Tue, Jun 10, 2014 at 01:46:56PM +0100, Chris Wilson wrote:
> If we hit a vblank and see that have a pageflip queue but not yet
> processed, ensure that the GPU is running at maximum in order to clear
> the backlog. Pageflips are only queued for the following vblank, if we
> miss it, there will be a visible stutter. Boosting the GPU frequency
> doesn't prevent us from missing the target vblank, but it should help
> the subsequent frames hitting theirs.
>
> v2: Reorder vblank vs flip-complete so that we only check for a missed
> flip after processing the completion events, and avoid spurious boosts.
>
> v3: Rename missed_vblank
> v4: Rebase
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Found one more: boost_work never gets cancelled. Probably not want we
want. I guess the best place is in the gt powersave suspend function, since
at that point interrupts are no more.
First patch from this series is merged now.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++++
> 4 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 10dd80a12658..33ed0c6b8a9c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -910,6 +910,7 @@ struct intel_gen6_power_mgmt {
>
> bool enabled;
> struct delayed_work delayed_resume_work;
> + struct work_struct boost_work;
>
> /*
> * Protects RPS/RC6 register access and PCU communication.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5261c145e633..89a4d01fefb4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9326,6 +9326,7 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
> struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> unsigned long flags;
> + bool missed_vblank;
>
> if (crtc == NULL)
> return;
> @@ -9336,7 +9337,12 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
> intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
> page_flip_completed(intel_crtc);
> }
> + missed_vblank = (intel_crtc->unpin_work != NULL &&
> + drm_vblank_count(dev, pipe) - intel_crtc->unpin_work->flip_queued_vblank > 1);
> spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> + if (missed_vblank)
> + intel_queue_rps_boost(dev);
> }
>
> static int intel_crtc_page_flip(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 10665716eb10..2e3c65812e72 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -969,6 +969,7 @@ void ironlake_teardown_rc6(struct drm_device *dev);
> void gen6_update_ring_freq(struct drm_device *dev);
> void gen6_rps_idle(struct drm_i915_private *dev_priv);
> void gen6_rps_boost(struct drm_i915_private *dev_priv);
> +void intel_queue_rps_boost(struct drm_device *dev);
> void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b06f896740b5..ab760e5abc9a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6722,6 +6722,19 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val)
> return DIV_ROUND_CLOSEST(4 * mul * val, dev_priv->mem_freq) + 0xbd - 6;
> }
>
> +static void __intel_rps_boost_work(struct work_struct *work)
> +{
> + gen6_rps_boost(container_of(work, struct drm_i915_private, rps.boost_work));
> +}
> +
> +void intel_queue_rps_boost(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (INTEL_INFO(dev)->gen >= 6)
> + queue_work(dev_priv->wq, &dev_priv->rps.boost_work);
> +}
> +
> void intel_pm_setup(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -6730,6 +6743,8 @@ void intel_pm_setup(struct drm_device *dev)
>
> INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
> intel_gen6_powersave_work);
> + INIT_WORK(&dev_priv->rps.boost_work,
> + __intel_rps_boost_work);
>
> dev_priv->pm.suspended = false;
> dev_priv->pm.irqs_disabled = false;
> --
> 2.0.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank
2014-06-10 12:46 [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank Chris Wilson
2014-06-10 12:46 ` [PATCH 2/3] drm/i915: Decouple the stuck pageflip on modeset Chris Wilson
2014-06-10 12:46 ` [PATCH 3/3] drm/i915: Boost GPU frequency if we detect outstanding pageflips Chris Wilson
@ 2014-06-10 13:46 ` Ville Syrjälä
2014-06-11 10:47 ` [PATCH] " Chris Wilson
3 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2014-06-10 13:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx
On Tue, Jun 10, 2014 at 01:46:54PM +0100, Chris Wilson wrote:
> Long ago, back in the racy haydays of 915gm interrupt handling, page
> flips would occasionally go astray and leave the hardware stuck, and the
> display not updating. This annoyed people who relied on their systems
> being able to display continuously updating information 24/7, and so
> some code to detect when the driver missed the page flip completion
> signal was added. Until recently, it was presumed that the interrupt
> handling was now flawless, but once again Simon Farnsworth has found a
> system whose display will stall. Reinstate the pageflip stall detection,
> which works by checking to see if the hardware has been updated to the
> new framebuffer address following each vblank. If the hardware is
> scanning out from the new framebuffer, but we still think the flip is
> pending, then we kick our driver into submision.
>
> 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
>
> This now includes 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 - i.e.
> that the user is able to continue submitting flips.
>
> v2: Cleanup, refactor, and rename
> v3: Only start counting vblanks after the flip command has been seen by
> the hardware.
>
> Reported-by: Simon Farnsworth <simon@farnz.org.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Someone may want to add the flip counter check later. At least there's a
comment about danger of false positives.
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 29 ++++++---
> drivers/gpu/drm/i915/i915_irq.c | 85 ++++++++----------------
> drivers/gpu/drm/i915/intel_display.c | 121 ++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 5 ++
> 4 files changed, 147 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e8ea1efd3810..eaffffe1a184 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 = 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;
>
> @@ -595,6 +596,8 @@ 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 {
> + u32 addr;
> +
> if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
> seq_printf(m, "Flip queued on pipe %c (plane %c)\n",
> pipe, plane);
> @@ -602,23 +605,29 @@ 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 %s at seqno %u, now %u\n",
> + work->ring->name,
> + work->flip_queued_seqno,
> + work->ring->get_seqno(work->ring, true));
> + seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
> + work->flip_queued_vblank,
> + work->flip_ready_vblank,
> + drm_vblank_count(dev, crtc->pipe));
> if (work->enable_stall_check)
> seq_puts(m, "Stall check enabled, ");
> else
> seq_puts(m, "Stall check waiting for page flip ioctl, ");
> 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;
> - if (obj)
> - seq_printf(m, "Old framebuffer gtt_offset 0x%08lx\n",
> - i915_gem_obj_ggtt_offset(obj));
> - }
> + if (INTEL_INFO(dev)->gen >= 4)
> + addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
> + else
> + addr = I915_READ(DSPADDR(crtc->plane));
> + seq_printf(m, "Current scanout address 0x%08x\n", addr);
> +
> 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));
> + seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset);
> + seq_printf(m, "MMIO update completed? %d\n", addr == work->gtt_offset);
> }
> }
> spin_unlock_irqrestore(&dev->event_lock, flags);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 229e1f18fe7a..c3db85f2bff0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1802,8 +1802,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> spin_unlock(&dev_priv->irq_lock);
>
> for_each_pipe(pipe) {
> - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> - intel_pipe_handle_vblank(dev, pipe);
> + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
> intel_prepare_page_flip(dev, pipe);
> @@ -2079,8 +2080,9 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> DRM_ERROR("Poison interrupt\n");
>
> for_each_pipe(pipe) {
> - if (de_iir & DE_PIPE_VBLANK(pipe))
> - intel_pipe_handle_vblank(dev, pipe);
> + if (de_iir & DE_PIPE_VBLANK(pipe) &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -2129,8 +2131,9 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> intel_opregion_asle_intr(dev);
>
> for_each_pipe(pipe) {
> - if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
> - intel_pipe_handle_vblank(dev, pipe);
> + if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)) &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> /* plane/pipes map 1:1 on ilk+ */
> if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> @@ -2265,8 +2268,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> continue;
>
> pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> - if (pipe_iir & GEN8_PIPE_VBLANK)
> - intel_pipe_handle_vblank(dev, pipe);
> +
> + if (pipe_iir & GEN8_PIPE_VBLANK &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
> intel_prepare_page_flip(dev, pipe);
> @@ -2575,52 +2580,6 @@ void i915_handle_error(struct drm_device *dev, bool wedged,
> schedule_work(&dev_priv->gpu_error.work);
> }
>
> -static void __always_unused i915_pageflip_stall_check(struct drm_device *dev, int pipe)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct drm_i915_gem_object *obj;
> - struct intel_unpin_work *work;
> - unsigned long flags;
> - bool stall_detected;
> -
> - /* Ignore early vblank irqs */
> - if (intel_crtc == NULL)
> - return;
> -
> - spin_lock_irqsave(&dev->event_lock, flags);
> - work = intel_crtc->unpin_work;
> -
> - 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;
> - }
> -
> - /* 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);
> - stall_detected = I915_HI_DISPBASE(I915_READ(dspsurf)) ==
> - i915_gem_obj_ggtt_offset(obj);
> - } else {
> - int dspaddr = DSPADDR(intel_crtc->plane);
> - stall_detected = I915_READ(dspaddr) == (i915_gem_obj_ggtt_offset(obj) +
> - crtc->y * crtc->primary->fb->pitches[0] +
> - crtc->x * crtc->primary->fb->bits_per_pixel/8);
> - }
> -
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - if (stall_detected) {
> - DRM_DEBUG_DRIVER("Pageflip stall detected\n");
> - intel_prepare_page_flip(dev, intel_crtc->plane);
> - }
> -}
> -
> /* Called from drm generic code, passed 'crtc' which
> * we use as a pipe index
> */
> @@ -3726,7 +3685,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> return false;
>
> if ((iir & flip_pending) == 0)
> - return false;
> + goto check_page_flip;
>
> intel_prepare_page_flip(dev, plane);
>
> @@ -3737,11 +3696,14 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> * an interrupt per se, we watch for the change at vblank.
> */
> if (I915_READ16(ISR) & flip_pending)
> - return false;
> + goto check_page_flip;
>
> intel_finish_page_flip(dev, pipe);
> -
> return true;
> +
> +check_page_flip:
> + intel_check_page_flip(dev, pipe);
> + return false;
> }
>
> static irqreturn_t i8xx_irq_handler(int irq, void *arg)
> @@ -3911,7 +3873,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
> return false;
>
> if ((iir & flip_pending) == 0)
> - return false;
> + goto check_page_flip;
>
> intel_prepare_page_flip(dev, plane);
>
> @@ -3922,11 +3884,14 @@ static bool i915_handle_vblank(struct drm_device *dev,
> * an interrupt per se, we watch for the change at vblank.
> */
> if (I915_READ(ISR) & flip_pending)
> - return false;
> + goto check_page_flip;
>
> intel_finish_page_flip(dev, pipe);
> -
> return true;
> +
> +check_page_flip:
> + intel_check_page_flip(dev, pipe);
> + return false;
> }
>
> static irqreturn_t i915_irq_handler(int irq, void *arg)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb2830420..ec166e41a27e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3330,6 +3330,29 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
> return false;
> }
>
> +static void page_flip_completed(struct intel_crtc *intel_crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> + struct intel_unpin_work *work = intel_crtc->unpin_work;
> +
> + /* ensure that the unpin work is consistent wrt ->pending. */
> + smp_rmb();
> + intel_crtc->unpin_work = NULL;
> +
> + if (work->event)
> + drm_send_vblank_event(intel_crtc->base.dev,
> + intel_crtc->pipe,
> + work->event);
> +
> + drm_crtc_vblank_put(&intel_crtc->base);
> +
> + wake_up_all(&dev_priv->pending_flip_queue);
> + queue_work(dev_priv->wq, &work->work);
> +
> + trace_i915_flip_complete(intel_crtc->plane,
> + work->pending_flip_obj);
> +}
> +
> void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -8895,7 +8918,6 @@ static void intel_unpin_work_fn(struct work_struct *__work)
> static void do_intel_finish_page_flip(struct drm_device *dev,
> struct drm_crtc *crtc)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_unpin_work *work;
> unsigned long flags;
> @@ -8915,23 +8937,9 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> return;
> }
>
> - /* and that the unpin work is consistent wrt ->pending. */
> - smp_rmb();
> -
> - intel_crtc->unpin_work = NULL;
> -
> - if (work->event)
> - drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
> -
> - drm_crtc_vblank_put(crtc);
> + page_flip_completed(intel_crtc);
>
> spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - wake_up_all(&dev_priv->pending_flip_queue);
> -
> - queue_work(dev_priv->wq, &work->work);
> -
> - trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
> }
>
> void intel_finish_page_flip(struct drm_device *dev, int pipe)
> @@ -9265,6 +9273,62 @@ 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 drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_unpin_work *work = intel_crtc->unpin_work;
> + u32 addr;
> +
> + if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
> + return true;
> +
> + if (!work->enable_stall_check)
> + return false;
> +
> + if (work->flip_ready_vblank == 0) {
> + if (!i915_seqno_passed(work->ring->get_seqno(work->ring, true),
> + work->flip_queued_seqno))
> + return false;
> +
> + work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> + }
> +
> + if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3)
> + return false;
> +
> + /* Potential stall - if we see that the flip has happened, assume a missed interrupt */
> + if (INTEL_INFO(dev)->gen >= 4)
> + addr = I915_HI_DISPBASE(I915_READ(DSPSURF(intel_crtc->plane)));
> + else
> + addr = I915_READ(DSPADDR(intel_crtc->plane));
> +
> + /* There is a potential issue here with a false positive after a flip
> + * to the same address.
> + */
> + return addr == work->gtt_offset;
> +}
> +
> +void intel_check_page_flip(struct drm_device *dev, int pipe)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + unsigned long flags;
> +
> + if (crtc == NULL)
> + return;
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
> + WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
> + intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
> + page_flip_completed(intel_crtc);
> + }
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> +}
> +
> static int intel_crtc_page_flip(struct drm_crtc *crtc,
> struct drm_framebuffer *fb,
> struct drm_pending_vblank_event *event,
> @@ -9312,12 +9376,20 @@ 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);
> - drm_crtc_vblank_put(crtc);
> + /* Before declaring the flip queue wedged, check if
> + * the hardware completed the operation behind our backs.
> + */
> + if (__intel_pageflip_stall_check(dev, crtc)) {
> + DRM_DEBUG_DRIVER("flip queue: previous flip completed, continuing\n");
> + page_flip_completed(intel_crtc);
> + } else {
> + DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> + spin_unlock_irqrestore(&dev->event_lock, flags);
>
> - DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> - return -EBUSY;
> + drm_crtc_vblank_put(crtc);
> + kfree(work);
> + return -EBUSY;
> + }
> }
> intel_crtc->unpin_work = work;
> spin_unlock_irqrestore(&dev->event_lock, flags);
> @@ -9337,8 +9409,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> work->pending_flip_obj = obj;
>
> - work->enable_stall_check = true;
> -
> atomic_inc(&intel_crtc->unpin_work_count);
> intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
>
> @@ -9359,8 +9429,13 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> if (ret)
> goto cleanup_pending;
>
> + work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> + work->flip_queued_seqno = intel_ring_get_seqno(ring);
> + work->ring = ring;
> +
> work->gtt_offset =
> i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> + work->enable_stall_check = true;
>
> ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring, page_flip_flags);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 78d4124dba84..10665716eb10 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -612,12 +612,16 @@ 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;
> + struct intel_engine_cs *ring;
> atomic_t pending;
> #define INTEL_FLIP_INACTIVE 0
> #define INTEL_FLIP_PENDING 1
> #define INTEL_FLIP_COMPLETE 2
> u32 flip_count;
> u32 gtt_offset;
> + u32 flip_queued_seqno;
> + int flip_queued_vblank;
> + int flip_ready_vblank;
> bool enable_stall_check;
> };
>
> @@ -763,6 +767,7 @@ __intel_framebuffer_create(struct drm_device *dev,
> void intel_prepare_page_flip(struct drm_device *dev, int plane);
> void intel_finish_page_flip(struct drm_device *dev, int pipe);
> void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> +void intel_check_page_flip(struct drm_device *dev, int pipe);
> struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> void assert_shared_dpll(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll,
> --
> 2.0.0
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH] drm/i915: Check for a stalled page flip after each vblank
2014-06-10 12:46 [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank Chris Wilson
` (2 preceding siblings ...)
2014-06-10 13:46 ` [PATCH 1/3] drm/i915: Check for a stalled page flip after each vblank Ville Syrjälä
@ 2014-06-11 10:47 ` Chris Wilson
2014-06-24 12:44 ` Jani Nikula
3 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2014-06-11 10:47 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
Long ago, back in the racy haydays of 915gm interrupt handling, page
flips would occasionally go astray and leave the hardware stuck, and the
display not updating. This annoyed people who relied on their systems
being able to display continuously updating information 24/7, and so
some code to detect when the driver missed the page flip completion
signal was added. Until recently, it was presumed that the interrupt
handling was now flawless, but once again Simon Farnsworth has found a
system whose display will stall. Reinstate the pageflip stall detection,
which works by checking to see if the hardware has been updated to the
new framebuffer address following each vblank. If the hardware is
scanning out from the new framebuffer, but we still think the flip is
pending, then we kick our driver into submision.
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
This now includes 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 - i.e.
that the user is able to continue submitting flips.
v2: Cleanup, refactor, and rename
v3: Only start counting vblanks after the flip command has been seen by
the hardware.
v4: Record the seqno after we touch the ring, or else there may be no
seqno allocated yet.
Reported-by: Simon Farnsworth <simon@farnz.org.uk>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++---
drivers/gpu/drm/i915/i915_irq.c | 85 +++++++-----------------
drivers/gpu/drm/i915/intel_display.c | 124 ++++++++++++++++++++++++++++-------
drivers/gpu/drm/i915/intel_drv.h | 5 ++
4 files changed, 150 insertions(+), 93 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e8ea1efd3810..eaffffe1a184 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 = 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;
@@ -595,6 +596,8 @@ 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 {
+ u32 addr;
+
if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
seq_printf(m, "Flip queued on pipe %c (plane %c)\n",
pipe, plane);
@@ -602,23 +605,29 @@ 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 %s at seqno %u, now %u\n",
+ work->ring->name,
+ work->flip_queued_seqno,
+ work->ring->get_seqno(work->ring, true));
+ seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
+ work->flip_queued_vblank,
+ work->flip_ready_vblank,
+ drm_vblank_count(dev, crtc->pipe));
if (work->enable_stall_check)
seq_puts(m, "Stall check enabled, ");
else
seq_puts(m, "Stall check waiting for page flip ioctl, ");
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;
- if (obj)
- seq_printf(m, "Old framebuffer gtt_offset 0x%08lx\n",
- i915_gem_obj_ggtt_offset(obj));
- }
+ if (INTEL_INFO(dev)->gen >= 4)
+ addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
+ else
+ addr = I915_READ(DSPADDR(crtc->plane));
+ seq_printf(m, "Current scanout address 0x%08x\n", addr);
+
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));
+ seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset);
+ seq_printf(m, "MMIO update completed? %d\n", addr == work->gtt_offset);
}
}
spin_unlock_irqrestore(&dev->event_lock, flags);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3ef1b7e48f07..d5fcac1f4468 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1802,8 +1802,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
spin_unlock(&dev_priv->irq_lock);
for_each_pipe(pipe) {
- if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
- intel_pipe_handle_vblank(dev, pipe);
+ if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
+ intel_pipe_handle_vblank(dev, pipe))
+ intel_check_page_flip(dev, pipe);
if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
intel_prepare_page_flip(dev, pipe);
@@ -2079,8 +2080,9 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
DRM_ERROR("Poison interrupt\n");
for_each_pipe(pipe) {
- if (de_iir & DE_PIPE_VBLANK(pipe))
- intel_pipe_handle_vblank(dev, pipe);
+ if (de_iir & DE_PIPE_VBLANK(pipe) &&
+ intel_pipe_handle_vblank(dev, pipe))
+ intel_check_page_flip(dev, pipe);
if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -2129,8 +2131,9 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
intel_opregion_asle_intr(dev);
for_each_pipe(pipe) {
- if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
- intel_pipe_handle_vblank(dev, pipe);
+ if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)) &&
+ intel_pipe_handle_vblank(dev, pipe))
+ intel_check_page_flip(dev, pipe);
/* plane/pipes map 1:1 on ilk+ */
if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2265,8 +2268,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
continue;
pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
- if (pipe_iir & GEN8_PIPE_VBLANK)
- intel_pipe_handle_vblank(dev, pipe);
+
+ if (pipe_iir & GEN8_PIPE_VBLANK &&
+ intel_pipe_handle_vblank(dev, pipe))
+ intel_check_page_flip(dev, pipe);
if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
intel_prepare_page_flip(dev, pipe);
@@ -2575,52 +2580,6 @@ void i915_handle_error(struct drm_device *dev, bool wedged,
schedule_work(&dev_priv->gpu_error.work);
}
-static void __always_unused i915_pageflip_stall_check(struct drm_device *dev, int pipe)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_i915_gem_object *obj;
- struct intel_unpin_work *work;
- unsigned long flags;
- bool stall_detected;
-
- /* Ignore early vblank irqs */
- if (intel_crtc == NULL)
- return;
-
- spin_lock_irqsave(&dev->event_lock, flags);
- work = intel_crtc->unpin_work;
-
- 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;
- }
-
- /* 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);
- stall_detected = I915_HI_DISPBASE(I915_READ(dspsurf)) ==
- i915_gem_obj_ggtt_offset(obj);
- } else {
- int dspaddr = DSPADDR(intel_crtc->plane);
- stall_detected = I915_READ(dspaddr) == (i915_gem_obj_ggtt_offset(obj) +
- crtc->y * crtc->primary->fb->pitches[0] +
- crtc->x * crtc->primary->fb->bits_per_pixel/8);
- }
-
- spin_unlock_irqrestore(&dev->event_lock, flags);
-
- if (stall_detected) {
- DRM_DEBUG_DRIVER("Pageflip stall detected\n");
- intel_prepare_page_flip(dev, intel_crtc->plane);
- }
-}
-
/* Called from drm generic code, passed 'crtc' which
* we use as a pipe index
*/
@@ -3736,7 +3695,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
return false;
if ((iir & flip_pending) == 0)
- return false;
+ goto check_page_flip;
intel_prepare_page_flip(dev, plane);
@@ -3747,11 +3706,14 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
* an interrupt per se, we watch for the change at vblank.
*/
if (I915_READ16(ISR) & flip_pending)
- return false;
+ goto check_page_flip;
intel_finish_page_flip(dev, pipe);
-
return true;
+
+check_page_flip:
+ intel_check_page_flip(dev, pipe);
+ return false;
}
static irqreturn_t i8xx_irq_handler(int irq, void *arg)
@@ -3921,7 +3883,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
return false;
if ((iir & flip_pending) == 0)
- return false;
+ goto check_page_flip;
intel_prepare_page_flip(dev, plane);
@@ -3932,11 +3894,14 @@ static bool i915_handle_vblank(struct drm_device *dev,
* an interrupt per se, we watch for the change at vblank.
*/
if (I915_READ(ISR) & flip_pending)
- return false;
+ goto check_page_flip;
intel_finish_page_flip(dev, pipe);
-
return true;
+
+check_page_flip:
+ intel_check_page_flip(dev, pipe);
+ return false;
}
static irqreturn_t i915_irq_handler(int irq, void *arg)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb2830420..f489f40e43cf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3330,6 +3330,29 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
return false;
}
+static void page_flip_completed(struct intel_crtc *intel_crtc)
+{
+ struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+ struct intel_unpin_work *work = intel_crtc->unpin_work;
+
+ /* ensure that the unpin work is consistent wrt ->pending. */
+ smp_rmb();
+ intel_crtc->unpin_work = NULL;
+
+ if (work->event)
+ drm_send_vblank_event(intel_crtc->base.dev,
+ intel_crtc->pipe,
+ work->event);
+
+ drm_crtc_vblank_put(&intel_crtc->base);
+
+ wake_up_all(&dev_priv->pending_flip_queue);
+ queue_work(dev_priv->wq, &work->work);
+
+ trace_i915_flip_complete(intel_crtc->plane,
+ work->pending_flip_obj);
+}
+
void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
@@ -8895,7 +8918,6 @@ static void intel_unpin_work_fn(struct work_struct *__work)
static void do_intel_finish_page_flip(struct drm_device *dev,
struct drm_crtc *crtc)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_unpin_work *work;
unsigned long flags;
@@ -8915,23 +8937,9 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
return;
}
- /* and that the unpin work is consistent wrt ->pending. */
- smp_rmb();
-
- intel_crtc->unpin_work = NULL;
-
- if (work->event)
- drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
-
- drm_crtc_vblank_put(crtc);
+ page_flip_completed(intel_crtc);
spin_unlock_irqrestore(&dev->event_lock, flags);
-
- wake_up_all(&dev_priv->pending_flip_queue);
-
- queue_work(dev_priv->wq, &work->work);
-
- trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
}
void intel_finish_page_flip(struct drm_device *dev, int pipe)
@@ -9265,6 +9273,64 @@ 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 drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_unpin_work *work = intel_crtc->unpin_work;
+ u32 addr;
+
+ if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
+ return true;
+
+ if (!work->enable_stall_check)
+ return false;
+
+ if (work->flip_ready_vblank == 0) {
+ if (!i915_seqno_passed(work->ring->get_seqno(work->ring, true),
+ work->flip_queued_seqno))
+ return false;
+
+ work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
+ }
+
+ if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3)
+ return false;
+
+ /* Potential stall - if we see that the flip has happened,
+ * assume a missed interrupt. */
+ if (INTEL_INFO(dev)->gen >= 4)
+ addr = I915_HI_DISPBASE(I915_READ(DSPSURF(intel_crtc->plane)));
+ else
+ addr = I915_READ(DSPADDR(intel_crtc->plane));
+
+ /* There is a potential issue here with a false positive after a flip
+ * to the same address. We could address this by checking for a
+ * non-incrementing frame counter.
+ */
+ return addr == work->gtt_offset;
+}
+
+void intel_check_page_flip(struct drm_device *dev, int pipe)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ unsigned long flags;
+
+ if (crtc == NULL)
+ return;
+
+ spin_lock_irqsave(&dev->event_lock, flags);
+ if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
+ WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
+ intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
+ page_flip_completed(intel_crtc);
+ }
+ spin_unlock_irqrestore(&dev->event_lock, flags);
+}
+
static int intel_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event,
@@ -9312,12 +9378,20 @@ 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);
- drm_crtc_vblank_put(crtc);
+ /* Before declaring the flip queue wedged, check if
+ * the hardware completed the operation behind our backs.
+ */
+ if (__intel_pageflip_stall_check(dev, crtc)) {
+ DRM_DEBUG_DRIVER("flip queue: previous flip completed, continuing\n");
+ page_flip_completed(intel_crtc);
+ } else {
+ DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
+ spin_unlock_irqrestore(&dev->event_lock, flags);
- DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
- return -EBUSY;
+ drm_crtc_vblank_put(crtc);
+ kfree(work);
+ return -EBUSY;
+ }
}
intel_crtc->unpin_work = work;
spin_unlock_irqrestore(&dev->event_lock, flags);
@@ -9337,8 +9411,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
work->pending_flip_obj = obj;
- work->enable_stall_check = true;
-
atomic_inc(&intel_crtc->unpin_work_count);
intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
@@ -9366,6 +9438,12 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
if (ret)
goto cleanup_unpin;
+ work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
+ work->flip_queued_seqno = intel_ring_get_seqno(ring);
+ work->ring = ring;
+
+ work->enable_stall_check = true;
+
intel_disable_fbc(dev);
intel_mark_fb_busy(obj, NULL);
mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 78d4124dba84..10665716eb10 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -612,12 +612,16 @@ 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;
+ struct intel_engine_cs *ring;
atomic_t pending;
#define INTEL_FLIP_INACTIVE 0
#define INTEL_FLIP_PENDING 1
#define INTEL_FLIP_COMPLETE 2
u32 flip_count;
u32 gtt_offset;
+ u32 flip_queued_seqno;
+ int flip_queued_vblank;
+ int flip_ready_vblank;
bool enable_stall_check;
};
@@ -763,6 +767,7 @@ __intel_framebuffer_create(struct drm_device *dev,
void intel_prepare_page_flip(struct drm_device *dev, int plane);
void intel_finish_page_flip(struct drm_device *dev, int pipe);
void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
+void intel_check_page_flip(struct drm_device *dev, int pipe);
struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
void assert_shared_dpll(struct drm_i915_private *dev_priv,
struct intel_shared_dpll *pll,
--
2.0.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] drm/i915: Check for a stalled page flip after each vblank
2014-06-11 10:47 ` [PATCH] " Chris Wilson
@ 2014-06-24 12:44 ` Jani Nikula
2014-06-24 12:46 ` Chris Wilson
0 siblings, 1 reply; 9+ messages in thread
From: Jani Nikula @ 2014-06-24 12:44 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Daniel Vetter
Chris, I'm lost with the state of this series. Patchwork and a mail from
Daniel claim that some of the patches were merged, but they are nowhere
to be found. Did Daniel first push and then change his mind?
BR,
Jani.
On Wed, 11 Jun 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Long ago, back in the racy haydays of 915gm interrupt handling, page
> flips would occasionally go astray and leave the hardware stuck, and the
> display not updating. This annoyed people who relied on their systems
> being able to display continuously updating information 24/7, and so
> some code to detect when the driver missed the page flip completion
> signal was added. Until recently, it was presumed that the interrupt
> handling was now flawless, but once again Simon Farnsworth has found a
> system whose display will stall. Reinstate the pageflip stall detection,
> which works by checking to see if the hardware has been updated to the
> new framebuffer address following each vblank. If the hardware is
> scanning out from the new framebuffer, but we still think the flip is
> pending, then we kick our driver into submision.
>
> 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
>
> This now includes 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 - i.e.
> that the user is able to continue submitting flips.
>
> v2: Cleanup, refactor, and rename
> v3: Only start counting vblanks after the flip command has been seen by
> the hardware.
> v4: Record the seqno after we touch the ring, or else there may be no
> seqno allocated yet.
>
> Reported-by: Simon Farnsworth <simon@farnz.org.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75502
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 29 +++++---
> drivers/gpu/drm/i915/i915_irq.c | 85 +++++++-----------------
> drivers/gpu/drm/i915/intel_display.c | 124 ++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 5 ++
> 4 files changed, 150 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e8ea1efd3810..eaffffe1a184 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 = 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;
>
> @@ -595,6 +596,8 @@ 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 {
> + u32 addr;
> +
> if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
> seq_printf(m, "Flip queued on pipe %c (plane %c)\n",
> pipe, plane);
> @@ -602,23 +605,29 @@ 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 %s at seqno %u, now %u\n",
> + work->ring->name,
> + work->flip_queued_seqno,
> + work->ring->get_seqno(work->ring, true));
> + seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
> + work->flip_queued_vblank,
> + work->flip_ready_vblank,
> + drm_vblank_count(dev, crtc->pipe));
> if (work->enable_stall_check)
> seq_puts(m, "Stall check enabled, ");
> else
> seq_puts(m, "Stall check waiting for page flip ioctl, ");
> 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;
> - if (obj)
> - seq_printf(m, "Old framebuffer gtt_offset 0x%08lx\n",
> - i915_gem_obj_ggtt_offset(obj));
> - }
> + if (INTEL_INFO(dev)->gen >= 4)
> + addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
> + else
> + addr = I915_READ(DSPADDR(crtc->plane));
> + seq_printf(m, "Current scanout address 0x%08x\n", addr);
> +
> 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));
> + seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset);
> + seq_printf(m, "MMIO update completed? %d\n", addr == work->gtt_offset);
> }
> }
> spin_unlock_irqrestore(&dev->event_lock, flags);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3ef1b7e48f07..d5fcac1f4468 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1802,8 +1802,9 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> spin_unlock(&dev_priv->irq_lock);
>
> for_each_pipe(pipe) {
> - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> - intel_pipe_handle_vblank(dev, pipe);
> + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
> intel_prepare_page_flip(dev, pipe);
> @@ -2079,8 +2080,9 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> DRM_ERROR("Poison interrupt\n");
>
> for_each_pipe(pipe) {
> - if (de_iir & DE_PIPE_VBLANK(pipe))
> - intel_pipe_handle_vblank(dev, pipe);
> + if (de_iir & DE_PIPE_VBLANK(pipe) &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
> if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -2129,8 +2131,9 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> intel_opregion_asle_intr(dev);
>
> for_each_pipe(pipe) {
> - if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
> - intel_pipe_handle_vblank(dev, pipe);
> + if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)) &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> /* plane/pipes map 1:1 on ilk+ */
> if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> @@ -2265,8 +2268,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
> continue;
>
> pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> - if (pipe_iir & GEN8_PIPE_VBLANK)
> - intel_pipe_handle_vblank(dev, pipe);
> +
> + if (pipe_iir & GEN8_PIPE_VBLANK &&
> + intel_pipe_handle_vblank(dev, pipe))
> + intel_check_page_flip(dev, pipe);
>
> if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
> intel_prepare_page_flip(dev, pipe);
> @@ -2575,52 +2580,6 @@ void i915_handle_error(struct drm_device *dev, bool wedged,
> schedule_work(&dev_priv->gpu_error.work);
> }
>
> -static void __always_unused i915_pageflip_stall_check(struct drm_device *dev, int pipe)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct drm_i915_gem_object *obj;
> - struct intel_unpin_work *work;
> - unsigned long flags;
> - bool stall_detected;
> -
> - /* Ignore early vblank irqs */
> - if (intel_crtc == NULL)
> - return;
> -
> - spin_lock_irqsave(&dev->event_lock, flags);
> - work = intel_crtc->unpin_work;
> -
> - 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;
> - }
> -
> - /* 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);
> - stall_detected = I915_HI_DISPBASE(I915_READ(dspsurf)) ==
> - i915_gem_obj_ggtt_offset(obj);
> - } else {
> - int dspaddr = DSPADDR(intel_crtc->plane);
> - stall_detected = I915_READ(dspaddr) == (i915_gem_obj_ggtt_offset(obj) +
> - crtc->y * crtc->primary->fb->pitches[0] +
> - crtc->x * crtc->primary->fb->bits_per_pixel/8);
> - }
> -
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - if (stall_detected) {
> - DRM_DEBUG_DRIVER("Pageflip stall detected\n");
> - intel_prepare_page_flip(dev, intel_crtc->plane);
> - }
> -}
> -
> /* Called from drm generic code, passed 'crtc' which
> * we use as a pipe index
> */
> @@ -3736,7 +3695,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> return false;
>
> if ((iir & flip_pending) == 0)
> - return false;
> + goto check_page_flip;
>
> intel_prepare_page_flip(dev, plane);
>
> @@ -3747,11 +3706,14 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
> * an interrupt per se, we watch for the change at vblank.
> */
> if (I915_READ16(ISR) & flip_pending)
> - return false;
> + goto check_page_flip;
>
> intel_finish_page_flip(dev, pipe);
> -
> return true;
> +
> +check_page_flip:
> + intel_check_page_flip(dev, pipe);
> + return false;
> }
>
> static irqreturn_t i8xx_irq_handler(int irq, void *arg)
> @@ -3921,7 +3883,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
> return false;
>
> if ((iir & flip_pending) == 0)
> - return false;
> + goto check_page_flip;
>
> intel_prepare_page_flip(dev, plane);
>
> @@ -3932,11 +3894,14 @@ static bool i915_handle_vblank(struct drm_device *dev,
> * an interrupt per se, we watch for the change at vblank.
> */
> if (I915_READ(ISR) & flip_pending)
> - return false;
> + goto check_page_flip;
>
> intel_finish_page_flip(dev, pipe);
> -
> return true;
> +
> +check_page_flip:
> + intel_check_page_flip(dev, pipe);
> + return false;
> }
>
> static irqreturn_t i915_irq_handler(int irq, void *arg)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb2830420..f489f40e43cf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3330,6 +3330,29 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
> return false;
> }
>
> +static void page_flip_completed(struct intel_crtc *intel_crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> + struct intel_unpin_work *work = intel_crtc->unpin_work;
> +
> + /* ensure that the unpin work is consistent wrt ->pending. */
> + smp_rmb();
> + intel_crtc->unpin_work = NULL;
> +
> + if (work->event)
> + drm_send_vblank_event(intel_crtc->base.dev,
> + intel_crtc->pipe,
> + work->event);
> +
> + drm_crtc_vblank_put(&intel_crtc->base);
> +
> + wake_up_all(&dev_priv->pending_flip_queue);
> + queue_work(dev_priv->wq, &work->work);
> +
> + trace_i915_flip_complete(intel_crtc->plane,
> + work->pending_flip_obj);
> +}
> +
> void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -8895,7 +8918,6 @@ static void intel_unpin_work_fn(struct work_struct *__work)
> static void do_intel_finish_page_flip(struct drm_device *dev,
> struct drm_crtc *crtc)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_unpin_work *work;
> unsigned long flags;
> @@ -8915,23 +8937,9 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> return;
> }
>
> - /* and that the unpin work is consistent wrt ->pending. */
> - smp_rmb();
> -
> - intel_crtc->unpin_work = NULL;
> -
> - if (work->event)
> - drm_send_vblank_event(dev, intel_crtc->pipe, work->event);
> -
> - drm_crtc_vblank_put(crtc);
> + page_flip_completed(intel_crtc);
>
> spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> - wake_up_all(&dev_priv->pending_flip_queue);
> -
> - queue_work(dev_priv->wq, &work->work);
> -
> - trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
> }
>
> void intel_finish_page_flip(struct drm_device *dev, int pipe)
> @@ -9265,6 +9273,64 @@ 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 drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_unpin_work *work = intel_crtc->unpin_work;
> + u32 addr;
> +
> + if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
> + return true;
> +
> + if (!work->enable_stall_check)
> + return false;
> +
> + if (work->flip_ready_vblank == 0) {
> + if (!i915_seqno_passed(work->ring->get_seqno(work->ring, true),
> + work->flip_queued_seqno))
> + return false;
> +
> + work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> + }
> +
> + if (drm_vblank_count(dev, intel_crtc->pipe) - work->flip_ready_vblank < 3)
> + return false;
> +
> + /* Potential stall - if we see that the flip has happened,
> + * assume a missed interrupt. */
> + if (INTEL_INFO(dev)->gen >= 4)
> + addr = I915_HI_DISPBASE(I915_READ(DSPSURF(intel_crtc->plane)));
> + else
> + addr = I915_READ(DSPADDR(intel_crtc->plane));
> +
> + /* There is a potential issue here with a false positive after a flip
> + * to the same address. We could address this by checking for a
> + * non-incrementing frame counter.
> + */
> + return addr == work->gtt_offset;
> +}
> +
> +void intel_check_page_flip(struct drm_device *dev, int pipe)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + unsigned long flags;
> +
> + if (crtc == NULL)
> + return;
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) {
> + WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n",
> + intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe));
> + page_flip_completed(intel_crtc);
> + }
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> +}
> +
> static int intel_crtc_page_flip(struct drm_crtc *crtc,
> struct drm_framebuffer *fb,
> struct drm_pending_vblank_event *event,
> @@ -9312,12 +9378,20 @@ 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);
> - drm_crtc_vblank_put(crtc);
> + /* Before declaring the flip queue wedged, check if
> + * the hardware completed the operation behind our backs.
> + */
> + if (__intel_pageflip_stall_check(dev, crtc)) {
> + DRM_DEBUG_DRIVER("flip queue: previous flip completed, continuing\n");
> + page_flip_completed(intel_crtc);
> + } else {
> + DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> + spin_unlock_irqrestore(&dev->event_lock, flags);
>
> - DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
> - return -EBUSY;
> + drm_crtc_vblank_put(crtc);
> + kfree(work);
> + return -EBUSY;
> + }
> }
> intel_crtc->unpin_work = work;
> spin_unlock_irqrestore(&dev->event_lock, flags);
> @@ -9337,8 +9411,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>
> work->pending_flip_obj = obj;
>
> - work->enable_stall_check = true;
> -
> atomic_inc(&intel_crtc->unpin_work_count);
> intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
>
> @@ -9366,6 +9438,12 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> if (ret)
> goto cleanup_unpin;
>
> + work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> + work->flip_queued_seqno = intel_ring_get_seqno(ring);
> + work->ring = ring;
> +
> + work->enable_stall_check = true;
> +
> intel_disable_fbc(dev);
> intel_mark_fb_busy(obj, NULL);
> mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 78d4124dba84..10665716eb10 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -612,12 +612,16 @@ 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;
> + struct intel_engine_cs *ring;
> atomic_t pending;
> #define INTEL_FLIP_INACTIVE 0
> #define INTEL_FLIP_PENDING 1
> #define INTEL_FLIP_COMPLETE 2
> u32 flip_count;
> u32 gtt_offset;
> + u32 flip_queued_seqno;
> + int flip_queued_vblank;
> + int flip_ready_vblank;
> bool enable_stall_check;
> };
>
> @@ -763,6 +767,7 @@ __intel_framebuffer_create(struct drm_device *dev,
> void intel_prepare_page_flip(struct drm_device *dev, int plane);
> void intel_finish_page_flip(struct drm_device *dev, int pipe);
> void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> +void intel_check_page_flip(struct drm_device *dev, int pipe);
> struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> void assert_shared_dpll(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll,
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/i915: Check for a stalled page flip after each vblank
2014-06-24 12:44 ` Jani Nikula
@ 2014-06-24 12:46 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2014-06-24 12:46 UTC (permalink / raw)
To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx
On Tue, Jun 24, 2014 at 03:44:19PM +0300, Jani Nikula wrote:
>
> Chris, I'm lost with the state of this series. Patchwork and a mail from
> Daniel claim that some of the patches were merged, but they are nowhere
> to be found. Did Daniel first push and then change his mind?
There was a conflict with the mmio flips that he merged, so he dropped them.
Updated patches are on mailing list waiting for an r-b.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 9+ messages in thread