[PATCH 0/6] drm/i915: Avoid stuck page flip waiters on GPU reset

[PATCH 0/6] drm/i915: Avoid stuck page flip waiters on GPU reset

[ville.syrjala]

Someone mentioned on irc that intel_crtc_wait_for_pending_flips() was
getting stuck in some cases. This rang a bell since I was poking around
that stuff last year.

The issue that I'm trying to fix here is processes getting stuck in D
state when a GPU reset happens while page flips have been scheduled.

Testing is easy 1) fire up 'glxgears -fullscreen', run 'gem_hang 0',
try to VT switch. Without this series X and some kworker soon get stuck
in D state and you're left with a useless box. With the patch set, you
wait a while, the GPU hangcheck kicks in, and you get your console back.

The irc discussion was apparently about [1], but since the dmesg there 
doesn't show a GPU hang, I don't see this patch set fixing it. Frankly,
I have no idea what's happening there.

Additional work after this would involve sending out pending page flip
events. Currently if you don't do the VT switch after a hang, glxgears
remains stuck because the X server didn't get the page flip event from
the kernel. Also we should probably do an explicit intel_pipe_set_base()
with the current fb, to make sure we show the correct fb after the hang.
But I'm not going to touch these right now. Actually I'm hoping someone
else will volunteer for these tasks ;)

[1] https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1097315

[PATCH 1/6] drm/i915: Kill obj->pending_flip

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The pending flip mask no longer set anywhere, so trying to wait for
while it's non-zero is a no-op. Remove it completely.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 7 -------
 drivers/gpu/drm/i915/intel_display.c | 6 ------
 2 files changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4a4d8bd..3a08a3a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1208,13 +1208,6 @@ struct drm_i915_gem_object {
 
 	/** for phy allocated objects */
 	struct drm_i915_gem_phys_object *phys_obj;
-
-	/**
-	 * Number of crtcs where this object is currently the fb, but
-	 * will be page flipped away on the next vblank.  When it
-	 * reaches 0, dev_priv->pending_flip_queue will be woken up.
-	 */
-	atomic_t pending_flip;
 };
 #define to_gem_object(obj) (&((struct drm_i915_gem_object *)(obj))->base)
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 796090e..4097118 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2220,12 +2220,6 @@ intel_finish_fb(struct drm_framebuffer *old_fb)
 	bool was_interruptible = dev_priv->mm.interruptible;
 	int ret;
 
-	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
-
-	wait_event(dev_priv->pending_flip_queue,
-		   i915_reset_in_progress(&dev_priv->gpu_error) ||
-		   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
-- 
1.7.12.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If a GPU reset occurs while a page flip has been submitted to the ring,
the flip will never complete once the ring has been reset.

The GPU reset can be detected by sampling the reset_counter before the
flip is submitted, and then while waiting for the flip, the sampled
counter is compared with the current reset_counter value.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4097118..e348a68 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2862,10 +2862,12 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	unsigned long flags;
 	bool pending;
 
-	if (i915_reset_in_progress(&dev_priv->gpu_error))
+	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
+	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
 		return false;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
@@ -6912,6 +6914,8 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err_unpin;
 
+	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+
 	/* Can't queue multiple flips, so wait for the previous
 	 * one to finish before executing the next.
 	 */
@@ -6956,6 +6960,8 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err_unpin;
 
+	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+
 	if (intel_crtc->plane)
 		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
 	else
@@ -6997,6 +7003,8 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err_unpin;
 
+	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+
 	/* i965+ uses the linear or tiled offsets from the
 	 * Display Registers (which do not change across a page-flip)
 	 * so we need only reprogram the base address.
@@ -7045,6 +7053,8 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err_unpin;
 
+	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+
 	intel_ring_emit(ring, MI_DISPLAY_FLIP |
 			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 	intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
@@ -7111,6 +7121,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	if (ret)
 		goto err_unpin;
 
+	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
+
 	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
 	intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
 	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fcdfe42..a5521d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -235,6 +235,9 @@ struct intel_crtc {
 	/* We can share PLLs across outputs if the timings match */
 	struct intel_pch_pll *pch_pll;
 	uint32_t ddi_pll_sel;
+
+	/* reset counter value when the last flip was submitted */
+	unsigned int reset_counter;
 };
 
 struct intel_plane {
-- 
1.7.12.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915: Wake up pending_flip_queue as part of reset handling

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Someone may be waiting for a flip that will never complete due to a GPU
reset. Wake up all such waiters when the hang is first detected, and
after the reset processing has finished.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 13bb8d3..8b1146b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -915,6 +915,8 @@ static void i915_error_work_func(struct work_struct *work)
 		for_each_ring(ring, dev_priv, i)
 			wake_up_all(&ring->irq_queue);
 
+		wake_up_all(&dev_priv->pending_flip_queue);
+
 		wake_up_all(&dev_priv->gpu_error.reset_queue);
 	}
 }
@@ -1540,6 +1542,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
 		 */
 		for_each_ring(ring, dev_priv, i)
 			wake_up_all(&ring->irq_queue);
+
+		wake_up_all(&dev_priv->pending_flip_queue);
 	}
 
 	queue_work(dev_priv->wq, &dev_priv->gpu_error.work);
-- 
1.7.12.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/i915: Move intel_crtc_has_pending_flip() earlier

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Just shuffle the code around. No functional changes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e348a68..6c21985 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2262,6 +2262,25 @@ static void intel_crtc_update_sarea_pos(struct drm_crtc *crtc, int x, int y)
 	}
 }
 
+static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long flags;
+	bool pending;
+
+	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
+	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
+		return false;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	pending = to_intel_crtc(crtc)->unpin_work != NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return pending;
+}
+
 static int
 intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		    struct drm_framebuffer *fb)
@@ -2858,25 +2877,6 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
 	udelay(100);
 }
 
-static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	unsigned long flags;
-	bool pending;
-
-	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
-	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
-		return false;
-
-	spin_lock_irqsave(&dev->event_lock, flags);
-	pending = to_intel_crtc(crtc)->unpin_work != NULL;
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
-	return pending;
-}
-
 static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-- 
1.7.12.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm/i915: Add intel_crtc_wait_for_pending_flips_locked()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the functionality of intel_crtc_wait_for_pending_flips()
into intel_crtc_wait_for_pending_flips_locked().

intel_crtc_wait_for_pending_flips() is now just a wrapper that
grab struct_mutex and calls intel_crtc_wait_for_pending_flips_locked().

This changes the behaviour of intel_crtc_wait_for_pending_flips()
so that it now holds struct_mutex while waiting for pending flips.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6c21985..a2e04f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2281,6 +2281,22 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 	return pending;
 }
 
+static void intel_crtc_wait_for_pending_flips_locked(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (crtc->fb == NULL)
+		return;
+
+	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
+
+	wait_event(dev_priv->pending_flip_queue,
+		   !intel_crtc_has_pending_flip(crtc));
+
+	intel_finish_fb(crtc->fb);
+}
+
 static int
 intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		    struct drm_framebuffer *fb)
@@ -2880,18 +2896,12 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
 static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (crtc->fb == NULL)
 		return;
 
-	WARN_ON(waitqueue_active(&dev_priv->pending_flip_queue));
-
-	wait_event(dev_priv->pending_flip_queue,
-		   !intel_crtc_has_pending_flip(crtc));
-
 	mutex_lock(&dev->struct_mutex);
-	intel_finish_fb(crtc->fb);
+	intel_crtc_wait_for_pending_flips_locked(crtc);
 	mutex_unlock(&dev->struct_mutex);
 }
 
-- 
1.7.12.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since obj->pending_flips was never set, intel_pipe_set_base() never
actually waited for pending page flips to complete.

We really do want to wait for the pending flips, because otherwise the
mmio surface base address update could overtake the flip, and you
could end up with an old frame on the screen once the flip really
completes.

Just call intel_crtc_wait_pending_flips_locked() instead of
intel_finish_fb() from intel_pipe_set_base() to achieve the
desired result.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a2e04f7..7e047c1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2330,8 +2330,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return ret;
 	}
 
-	if (crtc->fb)
-		intel_finish_fb(crtc->fb);
+	intel_crtc_wait_for_pending_flips_locked(crtc);
 
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
-- 
1.7.12.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/6] drm/i915: Avoid stuck page flip waiters on GPU reset

[Daniel Vetter]

On Tue, Jan 29, 2013 at 06:13:32PM +0200, ville.syrjala@linux.intel.com wrote:
> Someone mentioned on irc that intel_crtc_wait_for_pending_flips() was
> getting stuck in some cases. This rang a bell since I was poking around
> that stuff last year.
> 
> The issue that I'm trying to fix here is processes getting stuck in D
> state when a GPU reset happens while page flips have been scheduled.
> 
> Testing is easy 1) fire up 'glxgears -fullscreen', run 'gem_hang 0',
> try to VT switch. Without this series X and some kworker soon get stuck
> in D state and you're left with a useless box. With the patch set, you
> wait a while, the GPU hangcheck kicks in, and you get your console back.

Broken record maintainer request: Can you please bake that into an i-g-t?
I think (hope) that running one of the delayed flip tests vs. the hangman
(gem_hang is a bit evil since it can kill boxes for real) should do the
trick. Then maybe also run one of the wf-vblank tests vs. hangman to check
that we cancel those correctly, too.

> The irc discussion was apparently about [1], but since the dmesg there 
> doesn't show a GPU hang, I don't see this patch set fixing it. Frankly,
> I have no idea what's happening there.
> 
> Additional work after this would involve sending out pending page flip
> events. Currently if you don't do the VT switch after a hang, glxgears
> remains stuck because the X server didn't get the page flip event from
> the kernel. Also we should probably do an explicit intel_pipe_set_base()
> with the current fb, to make sure we show the correct fb after the hang.
> But I'm not going to touch these right now. Actually I'm hoping someone
> else will volunteer for these tasks ;)

Hm ... I guess we need to walk the file_priv event list somewhere an fire
them all off, indicating somehow that things went kaboom. I think we
should aim for a drm generic way to singal those even.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/6] drm/i915: Avoid stuck page flip waiters on GPU reset

[Daniel Vetter]

On Tue, Jan 29, 2013 at 05:39:46PM +0100, Daniel Vetter wrote:
> On Tue, Jan 29, 2013 at 06:13:32PM +0200, ville.syrjala@linux.intel.com wrote:
> > Someone mentioned on irc that intel_crtc_wait_for_pending_flips() was
> > getting stuck in some cases. This rang a bell since I was poking around
> > that stuff last year.
> > 
> > The issue that I'm trying to fix here is processes getting stuck in D
> > state when a GPU reset happens while page flips have been scheduled.
> > 
> > Testing is easy 1) fire up 'glxgears -fullscreen', run 'gem_hang 0',
> > try to VT switch. Without this series X and some kworker soon get stuck
> > in D state and you're left with a useless box. With the patch set, you
> > wait a while, the GPU hangcheck kicks in, and you get your console back.
> 
> Broken record maintainer request: Can you please bake that into an i-g-t?
> I think (hope) that running one of the delayed flip tests vs. the hangman
> (gem_hang is a bit evil since it can kill boxes for real) should do the
> trick. Then maybe also run one of the wf-vblank tests vs. hangman to check
> that we cancel those correctly, too.

Actually for the case you're fixing here we probably need a delayed flip
vs. modeset (without flip event checks) against a simulated gpu hang.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/6] drm/i915: Kill obj->pending_flip

[Damien Lespiau]

On Tue, Jan 29, 2013 at 06:13:33PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The pending flip mask no longer set anywhere, so trying to wait for
> while it's non-zero is a no-op. Remove it completely.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[Damien Lespiau]

On Tue, Jan 29, 2013 at 06:13:34PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If a GPU reset occurs while a page flip has been submitted to the ring,
> the flip will never complete once the ring has been reset.
> 
> The GPU reset can be detected by sampling the reset_counter before the
> flip is submitted, and then while waiting for the flip, the sampled
> counter is compared with the current reset_counter value.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

You might want to rename reset_counter to flip_reset_counter to indicate
this field is specific to the flipping code. Other parts of the code
might need something similar as well?

-- 
Damien

Re: [PATCH 3/6] drm/i915: Wake up pending_flip_queue as part of reset handling

[Damien Lespiau]

On Tue, Jan 29, 2013 at 06:13:35PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Someone may be waiting for a flip that will never complete due to a GPU
> reset. Wake up all such waiters when the hang is first detected, and
> after the reset processing has finished.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 4/6] drm/i915: Move intel_crtc_has_pending_flip() earlier

[Damien Lespiau]

On Tue, Jan 29, 2013 at 06:13:36PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Just shuffle the code around. No functional changes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Mastering the art of small orthognal commits :)

-- 
Damien

Re: [PATCH 5/6] drm/i915: Add intel_crtc_wait_for_pending_flips_locked()

[Damien Lespiau]

On Tue, Jan 29, 2013 at 06:13:37PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Move the functionality of intel_crtc_wait_for_pending_flips()
> into intel_crtc_wait_for_pending_flips_locked().
> 
> intel_crtc_wait_for_pending_flips() is now just a wrapper that
> grab struct_mutex and calls intel_crtc_wait_for_pending_flips_locked().
> 
> This changes the behaviour of intel_crtc_wait_for_pending_flips()
> so that it now holds struct_mutex while waiting for pending flips.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
 
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Damien Lespiau]

On Tue, Jan 29, 2013 at 06:13:38PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since obj->pending_flips was never set, intel_pipe_set_base() never
> actually waited for pending page flips to complete.
> 
> We really do want to wait for the pending flips, because otherwise the
> mmio surface base address update could overtake the flip, and you
> could end up with an old frame on the screen once the flip really
> completes.
> 
> Just call intel_crtc_wait_pending_flips_locked() instead of
> intel_finish_fb() from intel_pipe_set_base() to achieve the
> desired result.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[Ville Syrjälä]

On Wed, Feb 13, 2013 at 10:23:28AM +0000, Damien Lespiau wrote:
> On Tue, Jan 29, 2013 at 06:13:34PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If a GPU reset occurs while a page flip has been submitted to the ring,
> > the flip will never complete once the ring has been reset.
> > 
> > The GPU reset can be detected by sampling the reset_counter before the
> > flip is submitted, and then while waiting for the flip, the sampled
> > counter is compared with the current reset_counter value.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> You might want to rename reset_counter to flip_reset_counter to indicate
> this field is specific to the flipping code. Other parts of the code
> might need something similar as well?

IIRC I used flip_reset_counter initially but then I decided it's too
long and dropped the flip_ part. I can change it back if that's what
people prefer.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[Daniel Vetter]

On Wed, Feb 13, 2013 at 12:51:33PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 13, 2013 at 10:23:28AM +0000, Damien Lespiau wrote:
> > On Tue, Jan 29, 2013 at 06:13:34PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > If a GPU reset occurs while a page flip has been submitted to the ring,
> > > the flip will never complete once the ring has been reset.
> > > 
> > > The GPU reset can be detected by sampling the reset_counter before the
> > > flip is submitted, and then while waiting for the flip, the sampled
> > > counter is compared with the current reset_counter value.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > 
> > You might want to rename reset_counter to flip_reset_counter to indicate
> > this field is specific to the flipping code. Other parts of the code
> > might need something similar as well?
> 
> IIRC I used flip_reset_counter initially but then I decided it's too
> long and dropped the flip_ part. I can change it back if that's what
> people prefer.

Imo the generic reset_counter name is ok. At least as long as we don't
need to keep track of more than one of these per crtc for different things
(or if we ever start to add more fine-grained reset domains).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[Daniel Vetter]

On Tue, Jan 29, 2013 at 06:13:34PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If a GPU reset occurs while a page flip has been submitted to the ring,
> the flip will never complete once the ring has been reset.
> 
> The GPU reset can be detected by sampling the reset_counter before the
> flip is submitted, and then while waiting for the flip, the sampled
> counter is compared with the current reset_counter value.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4097118..e348a68 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2862,10 +2862,12 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	unsigned long flags;
>  	bool pending;
>  
> -	if (i915_reset_in_progress(&dev_priv->gpu_error))
> +	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> +	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
>  		return false;
>  
>  	spin_lock_irqsave(&dev->event_lock, flags);
> @@ -6912,6 +6914,8 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err_unpin;
>  
> +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);

Ok no bikeshed about ->reset_counter, but a different one: Why does this
need to be in the per-gen callback? Can't we just grab this before we call
down into these callbacks? Imo races wrt the reset/hang code don't matter
that much as long as we don't wait forever for a pageflip which won't
happen. Hanging the gpu concurrently to pageflipping is undefined anyway
right now ...
-Daniel

> +
>  	/* Can't queue multiple flips, so wait for the previous
>  	 * one to finish before executing the next.
>  	 */
> @@ -6956,6 +6960,8 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err_unpin;
>  
> +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> +
>  	if (intel_crtc->plane)
>  		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
>  	else
> @@ -6997,6 +7003,8 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err_unpin;
>  
> +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> +
>  	/* i965+ uses the linear or tiled offsets from the
>  	 * Display Registers (which do not change across a page-flip)
>  	 * so we need only reprogram the base address.
> @@ -7045,6 +7053,8 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err_unpin;
>  
> +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> +
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP |
>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>  	intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
> @@ -7111,6 +7121,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  	if (ret)
>  		goto err_unpin;
>  
> +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> +
>  	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
>  	intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
>  	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fcdfe42..a5521d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -235,6 +235,9 @@ struct intel_crtc {
>  	/* We can share PLLs across outputs if the timings match */
>  	struct intel_pch_pll *pch_pll;
>  	uint32_t ddi_pll_sel;
> +
> +	/* reset counter value when the last flip was submitted */
> +	unsigned int reset_counter;
>  };
>  
>  struct intel_plane {
> -- 
> 1.7.12.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm/i915: Wake up pending_flip_queue as part of reset handling

[Daniel Vetter]

On Tue, Jan 29, 2013 at 06:13:35PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Someone may be waiting for a flip that will never complete due to a GPU
> reset. Wake up all such waiters when the hang is first detected, and
> after the reset processing has finished.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 13bb8d3..8b1146b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -915,6 +915,8 @@ static void i915_error_work_func(struct work_struct *work)
>  		for_each_ring(ring, dev_priv, i)
>  			wake_up_all(&ring->irq_queue);
>  
> +		wake_up_all(&dev_priv->pending_flip_queue);
> +
>  		wake_up_all(&dev_priv->gpu_error.reset_queue);
>  	}
>  }
> @@ -1540,6 +1542,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
>  		 */
>  		for_each_ring(ring, dev_priv, i)
>  			wake_up_all(&ring->irq_queue);
> +
> +		wake_up_all(&dev_priv->pending_flip_queue);

I don't quite follow why we need this one here. The wake up for the ring
irq queues is to get people off the dev->struct_mutex lock, which the
reset handler needs to acquire to do its job. But process stalling for
pageflips to complete already can't hold dev->struct_mutex, since
otherwise they'd block out the pageflip completion works and so would
deadlock already.

Is there another reason why we need to kick waiters before the reset has
completed that I'm missing here? One potential issue would be clarifying
what exactly happens when the gpu hangs while pageflipping, i.e. whether
the kernel should go out of it's way and re-issue the pageflip if it died
meanwhile. But even that could be scheduled from a workqueue I think. And
even if that'd require us to kick waiters of the pending flip queue I'd
prefer to only add it once we really need it (plus a big comment
explaining the tricks).
-Daniel

>  	}
>  
>  	queue_work(dev_priv->wq, &dev_priv->gpu_error.work);
> -- 
> 1.7.12.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Daniel Vetter]

On Tue, Jan 29, 2013 at 06:13:38PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since obj->pending_flips was never set, intel_pipe_set_base() never
> actually waited for pending page flips to complete.
> 
> We really do want to wait for the pending flips, because otherwise the
> mmio surface base address update could overtake the flip, and you
> could end up with an old frame on the screen once the flip really
> completes.
> 
> Just call intel_crtc_wait_pending_flips_locked() instead of
> intel_finish_fb() from intel_pipe_set_base() to achieve the
> desired result.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a2e04f7..7e047c1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2330,8 +2330,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		return ret;
>  	}
>  
> -	if (crtc->fb)
> -		intel_finish_fb(crtc->fb);
> +	intel_crtc_wait_for_pending_flips_locked(crtc);

Ah, I see now why you grab dev->struct_mutex and need to kick waiters from
the pending flip queue. I think grabbing the mutex twice isn't a major
offense, since both the crtc disable code and set_base are slowpaths used
rarely. So what about simply calling wait_for_pending_flips before
grabbing the mutex in intel_pipe_set_base? We could then also inline
finish_fb into it's only callsite ...
-Daniel

>  
>  	ret = dev_priv->display.update_plane(crtc, fb, x, y);
>  	if (ret) {
> -- 
> 1.7.12.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/6] drm/i915: Kill obj->pending_flip

[Daniel Vetter]

On Wed, Feb 13, 2013 at 10:16:27AM +0000, Damien Lespiau wrote:
> On Tue, Jan 29, 2013 at 06:13:33PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The pending flip mask no longer set anywhere, so trying to wait for
> > while it's non-zero is a no-op. Remove it completely.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[Ville Syrjälä]

On Wed, Feb 13, 2013 at 04:23:27PM +0100, Daniel Vetter wrote:
> On Tue, Jan 29, 2013 at 06:13:34PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If a GPU reset occurs while a page flip has been submitted to the ring,
> > the flip will never complete once the ring has been reset.
> > 
> > The GPU reset can be detected by sampling the reset_counter before the
> > flip is submitted, and then while waiting for the flip, the sampled
> > counter is compared with the current reset_counter value.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4097118..e348a68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2862,10 +2862,12 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	unsigned long flags;
> >  	bool pending;
> >  
> > -	if (i915_reset_in_progress(&dev_priv->gpu_error))
> > +	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> > +	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> >  		return false;
> >  
> >  	spin_lock_irqsave(&dev->event_lock, flags);
> > @@ -6912,6 +6914,8 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> >  	if (ret)
> >  		goto err_unpin;
> >  
> > +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> 
> Ok no bikeshed about ->reset_counter, but a different one: Why does this
> need to be in the per-gen callback? Can't we just grab this before we call
> down into these callbacks? Imo races wrt the reset/hang code don't matter
> that much as long as we don't wait forever for a pageflip which won't
> happen. Hanging the gpu concurrently to pageflipping is undefined anyway
> right now ...

Yeah. It could be moved to happen a little earlier, and thus avoid the
duplication.

> -Daniel
> 
> > +
> >  	/* Can't queue multiple flips, so wait for the previous
> >  	 * one to finish before executing the next.
> >  	 */
> > @@ -6956,6 +6960,8 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
> >  	if (ret)
> >  		goto err_unpin;
> >  
> > +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> > +
> >  	if (intel_crtc->plane)
> >  		flip_mask = MI_WAIT_FOR_PLANE_B_FLIP;
> >  	else
> > @@ -6997,6 +7003,8 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> >  	if (ret)
> >  		goto err_unpin;
> >  
> > +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> > +
> >  	/* i965+ uses the linear or tiled offsets from the
> >  	 * Display Registers (which do not change across a page-flip)
> >  	 * so we need only reprogram the base address.
> > @@ -7045,6 +7053,8 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> >  	if (ret)
> >  		goto err_unpin;
> >  
> > +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> > +
> >  	intel_ring_emit(ring, MI_DISPLAY_FLIP |
> >  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
> >  	intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
> > @@ -7111,6 +7121,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  	if (ret)
> >  		goto err_unpin;
> >  
> > +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> > +
> >  	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> >  	intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
> >  	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index fcdfe42..a5521d9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -235,6 +235,9 @@ struct intel_crtc {
> >  	/* We can share PLLs across outputs if the timings match */
> >  	struct intel_pch_pll *pch_pll;
> >  	uint32_t ddi_pll_sel;
> > +
> > +	/* reset counter value when the last flip was submitted */
> > +	unsigned int reset_counter;
> >  };
> >  
> >  struct intel_plane {
> > -- 
> > 1.7.12.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Ville Syrjälä]

On Wed, Feb 13, 2013 at 04:49:35PM +0100, Daniel Vetter wrote:
> On Tue, Jan 29, 2013 at 06:13:38PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Since obj->pending_flips was never set, intel_pipe_set_base() never
> > actually waited for pending page flips to complete.
> > 
> > We really do want to wait for the pending flips, because otherwise the
> > mmio surface base address update could overtake the flip, and you
> > could end up with an old frame on the screen once the flip really
> > completes.
> > 
> > Just call intel_crtc_wait_pending_flips_locked() instead of
> > intel_finish_fb() from intel_pipe_set_base() to achieve the
> > desired result.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a2e04f7..7e047c1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2330,8 +2330,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >  		return ret;
> >  	}
> >  
> > -	if (crtc->fb)
> > -		intel_finish_fb(crtc->fb);
> > +	intel_crtc_wait_for_pending_flips_locked(crtc);
> 
> Ah, I see now why you grab dev->struct_mutex and need to kick waiters from
> the pending flip queue. I think grabbing the mutex twice isn't a major
> offense, since both the crtc disable code and set_base are slowpaths used
> rarely. So what about simply calling wait_for_pending_flips before
> grabbing the mutex in intel_pipe_set_base? We could then also inline
> finish_fb into it's only callsite ...

I didn't want to slow down intel_pipe_set_base() too much. If we wait
for pending flips before pinning the new fb, we can never achieve any
parallelism there. But if no-one cares about that, we can reorder.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/6] drm/i915: Don't wait for page flips if there was GPU reset

[Daniel Vetter]

On Wed, Feb 13, 2013 at 06:52:29PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 13, 2013 at 04:23:27PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 29, 2013 at 06:13:34PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > If a GPU reset occurs while a page flip has been submitted to the ring,
> > > the flip will never complete once the ring has been reset.
> > > 
> > > The GPU reset can be detected by sampling the reset_counter before the
> > > flip is submitted, and then while waiting for the flip, the sampled
> > > counter is compared with the current reset_counter value.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 14 +++++++++++++-
> > >  drivers/gpu/drm/i915/intel_drv.h     |  3 +++
> > >  2 files changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 4097118..e348a68 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2862,10 +2862,12 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
> > >  {
> > >  	struct drm_device *dev = crtc->dev;
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >  	unsigned long flags;
> > >  	bool pending;
> > >  
> > > -	if (i915_reset_in_progress(&dev_priv->gpu_error))
> > > +	if (i915_reset_in_progress(&dev_priv->gpu_error) ||
> > > +	    intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter))
> > >  		return false;
> > >  
> > >  	spin_lock_irqsave(&dev->event_lock, flags);
> > > @@ -6912,6 +6914,8 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> > >  	if (ret)
> > >  		goto err_unpin;
> > >  
> > > +	intel_crtc->reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> > 
> > Ok no bikeshed about ->reset_counter, but a different one: Why does this
> > need to be in the per-gen callback? Can't we just grab this before we call
> > down into these callbacks? Imo races wrt the reset/hang code don't matter
> > that much as long as we don't wait forever for a pageflip which won't
> > happen. Hanging the gpu concurrently to pageflipping is undefined anyway
> > right now ...
> 
> Yeah. It could be moved to happen a little earlier, and thus avoid the
> duplication.

Applied the patch and moved the assignment around a bit, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Daniel Vetter]

On Wed, Feb 13, 2013 at 6:06 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index a2e04f7..7e047c1 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -2330,8 +2330,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>> >             return ret;
>> >     }
>> >
>> > -   if (crtc->fb)
>> > -           intel_finish_fb(crtc->fb);
>> > +   intel_crtc_wait_for_pending_flips_locked(crtc);
>>
>> Ah, I see now why you grab dev->struct_mutex and need to kick waiters from
>> the pending flip queue. I think grabbing the mutex twice isn't a major
>> offense, since both the crtc disable code and set_base are slowpaths used
>> rarely. So what about simply calling wait_for_pending_flips before
>> grabbing the mutex in intel_pipe_set_base? We could then also inline
>> finish_fb into it's only callsite ...
>
> I didn't want to slow down intel_pipe_set_base() too much. If we wait
> for pending flips before pinning the new fb, we can never achieve any
> parallelism there. But if no-one cares about that, we can reorder.

I'm confused here - where can we extract parallelism in set_base
between waiting for pending flips and the pinning? And imo set_base
isn't really critical: It's officially a synchronous thing (we have a
vblank wait in there), and if we want to fix that imo the nuclear
pageflip should be the answer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Ville Syrjälä]

On Wed, Feb 13, 2013 at 06:11:00PM +0100, Daniel Vetter wrote:
> On Wed, Feb 13, 2013 at 6:06 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index a2e04f7..7e047c1 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -2330,8 +2330,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >> >             return ret;
> >> >     }
> >> >
> >> > -   if (crtc->fb)
> >> > -           intel_finish_fb(crtc->fb);
> >> > +   intel_crtc_wait_for_pending_flips_locked(crtc);
> >>
> >> Ah, I see now why you grab dev->struct_mutex and need to kick waiters from
> >> the pending flip queue. I think grabbing the mutex twice isn't a major
> >> offense, since both the crtc disable code and set_base are slowpaths used
> >> rarely. So what about simply calling wait_for_pending_flips before
> >> grabbing the mutex in intel_pipe_set_base? We could then also inline
> >> finish_fb into it's only callsite ...
> >
> > I didn't want to slow down intel_pipe_set_base() too much. If we wait
> > for pending flips before pinning the new fb, we can never achieve any
> > parallelism there. But if no-one cares about that, we can reorder.
> 
> I'm confused here - where can we extract parallelism in set_base
> between waiting for pending flips and the pinning? And imo set_base
> isn't really critical: It's officially a synchronous thing (we have a
> vblank wait in there), and if we want to fix that imo the nuclear
> pageflip should be the answer.

The flip is making progress on the GPU side, and at the same time the
CPU side can make some progress with the pin operation. At least that
was my theory.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Daniel Vetter]

On Wed, Feb 13, 2013 at 6:26 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> The flip is making progress on the GPU side, and at the same time the
> CPU side can make some progress with the pin operation. At least that
> was my theory.

The pin should be a no-op when moving a given framebuffer around. It's
only really expensive when we need to clflush (and lesser so when we
need to rewrite ptes), which should only happen on fresh framebuffers.
As long as userspace does it's job and caches scanout buffers we
should be fine. The other thing is waiting for outstanding rendering
(to avoid hanging on an MI_WAIT_SCANLINE cmd), which just blocks. So
doesn't really matter imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm/i915: Really wait for pending flips in intel_pipe_set_base()

[Chris Wilson]

On Wed, Feb 13, 2013 at 06:11:00PM +0100, Daniel Vetter wrote:
> I'm confused here - where can we extract parallelism in set_base
> between waiting for pending flips and the pinning? And imo set_base
> isn't really critical: It's officially a synchronous thing (we have a
> vblank wait in there), and if we want to fix that imo the nuclear
> pageflip should be the answer.

Speaking of which I've had a patch to remove that extra synchronous
wait for over a year...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre