[PATCH 0/5] gpu reset improvements

[PATCH 0/5] gpu reset improvements

[Daniel Vetter]

Hi all,

The first two patches are actually unrelated, I've stumbled over that while
chasing down a dead alley.

The later patches fix up some issues I've found around the gpu reset handling.
Without these running the hangman in a loop eventually fails, with these it
didn't fail on my ilk, snb & ivb while running for half a day.

Note that intel_ring_begin now unconditionally returns -EIO if the gpu died,
even when the reset code has not yet died. In pratice you'll only hit this when
the ringbuffer is completely full when the gpu died _and_ the gpu's death has
been declared while an execbuf ioctl call was in progress (specifically already
grabbed the mutex but hasn't yet emitted all the MI_ commands).

Still, with pipelined fencing gone and the flushing list's demise almost
certain I think we should push down the non-interruptible handling of
intel_ring_begin in the callchain and try to do the right thing in the execbuf
code at least. But that's future patch-fodder!

Happy bikeshedding!

Cheers, Daniel

Daniel Vetter (5):
  drm/i915: wrap up gt powersave enabling functions
  drm/i915: make enable/disable_gt_powersave locking consistent
  drm/i915: don't return a spurious -EIO from intel_ring_begin
  drm/i915: don't trylock in the gpu reset code
  drm/i915: non-interruptible sleeps can't handle -EGAIN

 drivers/gpu/drm/i915/i915_drv.c         |    3 +-
 drivers/gpu/drm/i915/i915_drv.h         |    1 +
 drivers/gpu/drm/i915/i915_gem.c         |   19 ++++++++---
 drivers/gpu/drm/i915/i915_suspend.c     |    5 +--
 drivers/gpu/drm/i915/intel_display.c    |   23 +++----------
 drivers/gpu/drm/i915/intel_drv.h        |    9 ++---
 drivers/gpu/drm/i915/intel_pm.c         |   57 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.c |   10 +++---
 8 files changed, 65 insertions(+), 62 deletions(-)

-- 
1.7.10

[PATCH 1/5] drm/i915: wrap up gt powersave enabling functions

[Daniel Vetter]

... instead of calling each one for each generation indiviudally.

Notice that we've already managed to be inconsistent, the resume path
is missing an IS_VLV check. As a nice benefit we can mark all the
platform specific enable/disable functions as static and hide them in
intel_pm.c

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c  |    5 +----
 drivers/gpu/drm/i915/intel_display.c |   21 ++-----------------
 drivers/gpu/drm/i915/intel_drv.h     |    9 ++-------
 drivers/gpu/drm/i915/intel_pm.c      |   37 +++++++++++++++++++++++++++-------
 4 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 0ede02a..740c076 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -825,10 +825,7 @@ int i915_save_state(struct drm_device *dev)
 		dev_priv->saveIMR = I915_READ(IMR);
 	}
 
-	if (IS_IRONLAKE_M(dev))
-		ironlake_disable_drps(dev);
-	if (INTEL_INFO(dev)->gen >= 6)
-		gen6_disable_rps(dev);
+	intel_disable_gt_powersave(dev);
 
 	/* Cache mode state */
 	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b3052ef..fdca5b9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7172,20 +7172,9 @@ static void ivb_pch_pwm_override(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	intel_init_clock_gating(dev);
 
-	if (IS_IRONLAKE_M(dev)) {
-		ironlake_enable_drps(dev);
-		ironlake_enable_rc6(dev);
-		intel_init_emon(dev);
-	}
-
-	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
-		gen6_enable_rps(dev_priv);
-		gen6_update_ring_freq(dev_priv);
-	}
+	intel_enable_gt_powersave(dev);
 
 	if (IS_IVYBRIDGE(dev))
 		ivb_pch_pwm_override(dev);
@@ -7277,13 +7266,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_disable_fbc(dev);
 
-	if (IS_IRONLAKE_M(dev))
-		ironlake_disable_drps(dev);
-	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev))
-		gen6_disable_rps(dev);
-
-	if (IS_IRONLAKE_M(dev))
-		ironlake_disable_rc6(dev);
+	intel_disable_gt_powersave(dev);
 
 	if (IS_VALLEYVIEW(dev))
 		vlv_init_dpio(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5290e9d..cc1573b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -425,9 +425,6 @@ extern void intel_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
 extern void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
 				    u16 *blue, int regno);
 extern void intel_enable_clock_gating(struct drm_device *dev);
-extern void ironlake_disable_rc6(struct drm_device *dev);
-extern void ironlake_enable_drps(struct drm_device *dev);
-extern void ironlake_disable_drps(struct drm_device *dev);
 
 extern int intel_pin_and_fence_fb_obj(struct drm_device *dev,
 				      struct drm_i915_gem_object *obj,
@@ -494,10 +491,8 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
-extern void gen6_enable_rps(struct drm_i915_private *dev_priv);
-extern void gen6_update_ring_freq(struct drm_i915_private *dev_priv);
-extern void gen6_disable_rps(struct drm_device *dev);
-extern void intel_init_emon(struct drm_device *dev);
+extern void intel_enable_gt_powersave(struct drm_device *dev);
+extern void intel_disable_gt_powersave(struct drm_device *dev);
 
 extern void intel_ddi_dpms(struct drm_encoder *encoder, int mode);
 extern void intel_ddi_mode_set(struct drm_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7504fbc..2baba10 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2184,7 +2184,7 @@ bool ironlake_set_drps(struct drm_device *dev, u8 val)
 	return true;
 }
 
-void ironlake_enable_drps(struct drm_device *dev)
+static void ironlake_enable_drps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 rgvmodectl = I915_READ(MEMMODECTL);
@@ -2248,7 +2248,7 @@ void ironlake_enable_drps(struct drm_device *dev)
 	getrawmonotonic(&dev_priv->last_time2);
 }
 
-void ironlake_disable_drps(struct drm_device *dev)
+static void ironlake_disable_drps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 rgvswctl = I915_READ16(MEMSWCTL);
@@ -2301,7 +2301,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	dev_priv->cur_delay = val;
 }
 
-void gen6_disable_rps(struct drm_device *dev)
+static void gen6_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -2349,7 +2349,7 @@ int intel_enable_rc6(const struct drm_device *dev)
 	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
-void gen6_enable_rps(struct drm_i915_private *dev_priv)
+static void gen6_enable_rps(struct drm_i915_private *dev_priv)
 {
 	struct intel_ring_buffer *ring;
 	u32 rp_state_cap;
@@ -2494,7 +2494,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
 
-void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
+static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
 {
 	int min_freq = 15;
 	int gpu_freq, ia_freq, max_ia_freq;
@@ -3156,8 +3156,7 @@ void intel_gpu_ips_teardown(void)
 	i915_mch_dev = NULL;
 	spin_unlock(&mchdev_lock);
 }
-
-void intel_init_emon(struct drm_device *dev)
+static void intel_init_emon(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 lcfuse;
@@ -3228,6 +3227,30 @@ void intel_init_emon(struct drm_device *dev)
 	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
+void intel_disable_gt_powersave(struct drm_device *dev)
+{
+	if (IS_IRONLAKE_M(dev))
+		ironlake_disable_drps(dev);
+	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
+		gen6_disable_rps(dev);
+}
+
+void intel_enable_gt_powersave(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_IRONLAKE_M(dev)) {
+		ironlake_enable_drps(dev);
+		ironlake_enable_rc6(dev);
+		intel_init_emon(dev);
+	}
+
+	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
+		gen6_enable_rps(dev_priv);
+		gen6_update_ring_freq(dev_priv);
+	}
+}
+
 static void ironlake_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.10

[PATCH 2/5] drm/i915: make enable/disable_gt_powersave locking consistent

[Daniel Vetter]

The enable functions grabbed dev->struct_mutex themselves, whereas
the disable functions expected dev->struct_mutex to be held by the
caller. Move the locking out to the (currently only) callsite of
intel_enable_gt_powersave to make this more consistent.

Originally this was prep work for future patches, but I've chased down
a totally wrong alley. Still, I think this is a sensible
clarification.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 drivers/gpu/drm/i915/intel_pm.c      |   32 +++++++++++++-------------------
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fdca5b9..3fbc802 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7174,7 +7174,9 @@ void intel_modeset_init_hw(struct drm_device *dev)
 {
 	intel_init_clock_gating(dev);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_enable_gt_powersave(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	if (IS_IVYBRIDGE(dev))
 		ivb_pch_pwm_override(dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2baba10..99bc1f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2349,8 +2349,9 @@ int intel_enable_rc6(const struct drm_device *dev)
 	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
-static void gen6_enable_rps(struct drm_i915_private *dev_priv)
+static void gen6_enable_rps(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *ring;
 	u32 rp_state_cap;
 	u32 gt_perf_status;
@@ -2359,6 +2360,8 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	int rc6_mode;
 	int i;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	/* Here begins a magic sequence of register writes to enable
 	 * auto-downclocking.
 	 *
@@ -2366,7 +2369,6 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	 * userspace...
 	 */
 	I915_WRITE(GEN6_RC_STATE, 0);
-	mutex_lock(&dev_priv->dev->struct_mutex);
 
 	/* Clear the DBG now so we don't confuse earlier errors */
 	if ((gtfifodbg = I915_READ(GTFIFODBG))) {
@@ -2491,15 +2493,17 @@ static void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
 	gen6_gt_force_wake_put(dev_priv);
-	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
 
-static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
+static void gen6_update_ring_freq(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int min_freq = 15;
 	int gpu_freq, ia_freq, max_ia_freq;
 	int scaling_factor = 180;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	max_ia_freq = cpufreq_quick_get_max(0);
 	/*
 	 * Default to measured freq if none found, PCU will ensure we don't go
@@ -2511,8 +2515,6 @@ static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
 	/* Convert from kHz to MHz */
 	max_ia_freq /= 1000;
 
-	mutex_lock(&dev_priv->dev->struct_mutex);
-
 	/*
 	 * For each potential GPU frequency, load a ring frequency we'd like
 	 * to use for memory access.  We do this by specifying the IA frequency
@@ -2543,8 +2545,6 @@ static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
 			continue;
 		}
 	}
-
-	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
 
 static void ironlake_teardown_rc6(struct drm_device *dev)
@@ -2615,12 +2615,11 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	if (!intel_enable_rc6(dev))
 		return;
 
-	mutex_lock(&dev->struct_mutex);
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	ret = ironlake_setup_rc6(dev);
-	if (ret) {
-		mutex_unlock(&dev->struct_mutex);
+	if (ret)
 		return;
-	}
 
 	/*
 	 * GPU can automatically power down the render unit if given a page
@@ -2629,7 +2628,6 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	ret = intel_ring_begin(ring, 6);
 	if (ret) {
 		ironlake_teardown_rc6(dev);
-		mutex_unlock(&dev->struct_mutex);
 		return;
 	}
 
@@ -2654,13 +2652,11 @@ void ironlake_enable_rc6(struct drm_device *dev)
 	if (ret) {
 		DRM_ERROR("failed to enable ironlake power power savings\n");
 		ironlake_teardown_rc6(dev);
-		mutex_unlock(&dev->struct_mutex);
 		return;
 	}
 
 	I915_WRITE(PWRCTXA, dev_priv->pwrctx->gtt_offset | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 static unsigned long intel_pxfreq(u32 vidfreq)
@@ -3237,8 +3233,6 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 
 void intel_enable_gt_powersave(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_enable_drps(dev);
 		ironlake_enable_rc6(dev);
@@ -3246,8 +3240,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 	}
 
 	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
-		gen6_enable_rps(dev_priv);
-		gen6_update_ring_freq(dev_priv);
+		gen6_enable_rps(dev);
+		gen6_update_ring_freq(dev);
 	}
 }
 
-- 
1.7.10

[PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Daniel Vetter]

We have a careful logic in i915_gem_check_wedge already to asses whether
the gpu reset has been tried already and hence whether to return -EIO
or -EAGAIN.

Having this early check in intel_ring_begin doesn't buy us anything,
since we'll be calling into wait_request in the usual case already
anyway. In the corner case of not waiting for free space using the
last_retired_head we simply need to do the same check, too.

With these changes we'll only ever get an -EIO from intel_ring_begin
if the gpu has truely been declared dead.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h         |    1 +
 drivers/gpu/drm/i915/i915_gem.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |   10 ++++------
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0c15ab..86ac9ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1330,6 +1330,7 @@ i915_gem_object_unpin_fence(struct drm_i915_gem_object *obj)
 
 void i915_gem_retire_requests(struct drm_device *dev);
 void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
+int __must_check i915_gem_check_wedge(struct drm_i915_private *dev_priv);
 
 void i915_gem_reset(struct drm_device *dev);
 void i915_gem_clflush_object(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6a98c06..1214850 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1863,7 +1863,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-static int
+int
 i915_gem_check_wedge(struct drm_i915_private *dev_priv)
 {
 	BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index f30a53a..1e86894 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1220,8 +1220,10 @@ int intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n)
 		}
 
 		msleep(1);
-		if (atomic_read(&dev_priv->mm.wedged))
-			return -EAGAIN;
+
+		ret = i915_gem_check_wedge(dev_priv);
+		if (ret)
+			return ret;
 	} while (!time_after(jiffies, end));
 	trace_i915_ring_wait_end(ring);
 	return -EBUSY;
@@ -1230,13 +1232,9 @@ int intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n)
 int intel_ring_begin(struct intel_ring_buffer *ring,
 		     int num_dwords)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	int n = 4*num_dwords;
 	int ret;
 
-	if (unlikely(atomic_read(&dev_priv->mm.wedged)))
-		return -EIO;
-
 	if (unlikely(ring->tail + n > ring->effective_size)) {
 		ret = intel_wrap_ring_buffer(ring);
 		if (unlikely(ret))
-- 
1.7.10

[PATCH 4/5] drm/i915: don't trylock in the gpu reset code

[Daniel Vetter]

Simply failing to reset the gpu because someone else might still hold
the mutex isn't a great idea - I see reliable silent reset failures.
And gpu reset simply needs to be reliable and Just Work.

"But ... the deadlocks!"

We already kick all processes waiting for the gpu before launching the
reset work item. New waiters need to check the wedging state anyway
and then bail out. If we have places that can deadlock, we simply need
to fix them.

"But ... testing!"

We have the gpu hangman, and if the current gpu load gem_exec_nop
isn't good enough to hit a specific case, we can add a new one.

"But ...  don't we return -EAGAIN for non-interruptible calls to
wait_seqno now?"

Yep, but this problem already exists in the current code. A follow up
patch will remedy this by returning -EIO for non-interruptible sleeps
if the gpu died and the low-level wait bails out with -EAGAIN.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a378c08..11f21a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -866,8 +866,7 @@ int i915_reset(struct drm_device *dev)
 	if (!i915_try_reset)
 		return 0;
 
-	if (!mutex_trylock(&dev->struct_mutex))
-		return -EBUSY;
+	mutex_lock(&dev->struct_mutex);
 
 	i915_gem_reset(dev);
 
-- 
1.7.10

[PATCH 5/5] drm/i915: non-interruptible sleeps can't handle -EGAIN

[Daniel Vetter]

So don't return -EAGAIN, even in the case of a gpu hang. Remap it to -EIO
instead.

This is a bit ugly because intel_ring_begin is all non-interruptible
and hence only returns -EIO. But as the comment in there says,
auditing all the callsites would be a pain.

To avoid duplicating code, reuse i915_gem_check_wedge in __wait_seqno.
Use the opportunity to clarify the different cases a bit with
comments.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1214850..32b4db3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1878,7 +1878,16 @@ i915_gem_check_wedge(struct drm_i915_private *dev_priv)
 		recovery_complete = x->done > 0;
 		spin_unlock_irqrestore(&x->wait.lock, flags);
 
-		return recovery_complete ? -EIO : -EAGAIN;
+		/* Non-interruptible callers can't handle -EAGAIN, hence return
+		 * -EIO unconditionally for these. */
+		if (!dev_priv->mm.interruptible)
+			return -EIO;
+
+		/* Recovery complete, but still wedged means reset failure. */
+		if (recovery_complete)
+			return -EIO;
+
+		return -EAGAIN;
 	}
 
 	return 0;
@@ -1932,6 +1941,7 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	unsigned long timeout_jiffies;
 	long end;
 	bool wait_forever = true;
+	int ret;
 
 	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
 		return 0;
@@ -1963,8 +1973,9 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 			end = wait_event_timeout(ring->irq_queue, EXIT_COND,
 						 timeout_jiffies);
 
-		if (atomic_read(&dev_priv->mm.wedged))
-			end = -EAGAIN;
+		ret = i915_gem_check_wedge(dev_priv);
+		if (ret)
+			end = ret;
 	} while (end == 0 && wait_forever);
 
 	getrawmonotonic(&now);
-- 
1.7.10

Re: [PATCH 1/5] drm/i915: wrap up gt powersave enabling functions

[Eugeni Dodonov]

On 06/24/2012 11:42 AM, Daniel Vetter wrote:
> ... instead of calling each one for each generation indiviudally.
> 
> Notice that we've already managed to be inconsistent, the resume path
> is missing an IS_VLV check. As a nice benefit we can mark all the
> platform specific enable/disable functions as static and hide them in
> intel_pm.c
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I have very similar patch for this as well (for HSW patches to come
somewhat later this week), you beat me on sending it by a few days. So:

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Just one (actually, two) bikesheds below.

> +void intel_disable_gt_powersave(struct drm_device *dev)
> +{
> +	if (IS_IRONLAKE_M(dev))
> +		ironlake_disable_drps(dev);
> +	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> +		gen6_disable_rps(dev);
> +}

Just a minor bikeshed on those if loops. Wouldn't it be cleaner to
transform the 2nd if into 'else if'?

> +
> +void intel_enable_gt_powersave(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (IS_IRONLAKE_M(dev)) {
> +		ironlake_enable_drps(dev);
> +		ironlake_enable_rc6(dev);
> +		intel_init_emon(dev);
> +	}
> +
> +	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> +		gen6_enable_rps(dev_priv);
> +		gen6_update_ring_freq(dev_priv);
> +	}
> +}

...and here as well.

Eugeni

Re: [PATCH 2/5] drm/i915: make enable/disable_gt_powersave locking consistent

[Eugeni Dodonov]

On 06/24/2012 11:42 AM, Daniel Vetter wrote:
> The enable functions grabbed dev->struct_mutex themselves, whereas
> the disable functions expected dev->struct_mutex to be held by the
> caller. Move the locking out to the (currently only) callsite of
> intel_enable_gt_powersave to make this more consistent.
> 
> Originally this was prep work for future patches, but I've chased down
> a totally wrong alley. Still, I think this is a sensible
> clarification.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Eugeni

Re: [PATCH 1/5] drm/i915: wrap up gt powersave enabling functions

[Daniel Vetter]

On Sun, Jun 24, 2012 at 08:03:08PM -0300, Eugeni Dodonov wrote:
> On 06/24/2012 11:42 AM, Daniel Vetter wrote:
> > ... instead of calling each one for each generation indiviudally.
> > 
> > Notice that we've already managed to be inconsistent, the resume path
> > is missing an IS_VLV check. As a nice benefit we can mark all the
> > platform specific enable/disable functions as static and hide them in
> > intel_pm.c
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I have very similar patch for this as well (for HSW patches to come
> somewhat later this week), you beat me on sending it by a few days. So:
> 
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> 
> Just one (actually, two) bikesheds below.
> 
> > +void intel_disable_gt_powersave(struct drm_device *dev)
> > +{
> > +	if (IS_IRONLAKE_M(dev))
> > +		ironlake_disable_drps(dev);
> > +	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> > +		gen6_disable_rps(dev);
> > +}
> 
> Just a minor bikeshed on those if loops. Wouldn't it be cleaner to
> transform the 2nd if into 'else if'?

I guess I'll leave that for your hsw patches ;-)
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/5] drm/i915: make enable/disable_gt_powersave locking consistent

[Daniel Vetter]

On Sun, Jun 24, 2012 at 08:03:37PM -0300, Eugeni Dodonov wrote:
> On 06/24/2012 11:42 AM, Daniel Vetter wrote:
> > The enable functions grabbed dev->struct_mutex themselves, whereas
> > the disable functions expected dev->struct_mutex to be held by the
> > caller. Move the locking out to the (currently only) callsite of
> > intel_enable_gt_powersave to make this more consistent.
> > 
> > Originally this was prep work for future patches, but I've chased down
> > a totally wrong alley. Still, I think this is a sensible
> > clarification.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Merged the first two patches of this series to dinq, thanks for the
review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 4/5] drm/i915: don't trylock in the gpu reset code

[Chris Wilson]

On Sun, 24 Jun 2012 16:42:35 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Simply failing to reset the gpu because someone else might still hold
> the mutex isn't a great idea - I see reliable silent reset failures.
> And gpu reset simply needs to be reliable and Just Work.

GPU reset needs to just work, and if it can't then we need to avoid
locking up the machine...

Consider hitting an OOPS whlist holding the struct mutex and then
getting a hang, we end up with a struct worker thread. Not as bad as it
once was, but it can still make rebooting tricky at times.

How about a compromise,
while(!timeout) {if (trylock()) break; msleep(1); } ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: non-interruptible sleeps can't handle -EGAIN

[Chris Wilson]

On Sun, 24 Jun 2012 16:42:36 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> So don't return -EAGAIN, even in the case of a gpu hang. Remap it to -EIO
> instead.
> 
> This is a bit ugly because intel_ring_begin is all non-interruptible
> and hence only returns -EIO. But as the comment in there says,
> auditing all the callsites would be a pain.
> 
> To avoid duplicating code, reuse i915_gem_check_wedge in __wait_seqno.
> Use the opportunity to clarify the different cases a bit with
> comments.

Two separate patches. The first chunk sounds reasonable,
non-interruptible is a little bit of misnomer here, we mostly use it to
mean NOFAIL, and none of the callers are setup to handle EAGAIN. So
reporting EIO seems reasonable.

The rationale for if (wedged) return -EGAIN, was that originally it was
called under the mutex and all paths prior to that point sould have
checked the wedged status upon acquiring the mutex, and so to encounter
a wedged at that point implied that reset had not yet been run and so the
right error code was always EGAIN. However now it is lockless, so
check_wedge is more than just a cleanup but a useful early test for EIO.

(Aside from the BUG_ON!)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: non-interruptible sleeps can't handle -EGAIN

[Daniel Vetter]

So don't return -EAGAIN, even in the case of a gpu hang. Remap it to -EIO
instead.

This is a bit ugly because intel_ring_begin is all non-interruptible
and hence only returns -EIO. But as the comment in there says,
auditing all the callsites would be a pain.

To avoid duplicating code, reuse i915_gem_check_wedge in __wait_seqno.
Use the opportunity to clarify the different cases a bit with
comments.

v2: Don't access dev_priv->mm.interruptible from check_wedge - we
might not hold dev->struct_mutex, making this racy. Instead pass
interruptible in as a parameter. I've noticed this because I've hit a
BUG_ON(!mutex_is_locked) at the top of check_wedge. This has been
added in

commit b4aca0106c466b5a0329318203f65bac2d91b682
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Wed Apr 25 20:50:12 2012 -0700

    drm/i915: extract some common olr+wedge code

although that commit is missing any justification for this it. I guess
it's just copy&paste, because the same commit add the same BUG_ON
check to check_olr, where it indeed makes sense.

But in check_wedge everything we access is protected by other means,
so this is superflous. And because it now gets in the way (we add a
new caller in __wait_seqno, which can be called without
dev->struct_mutext) let's just remove it.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h         |    3 ++-
 drivers/gpu/drm/i915/i915_gem.c         |   24 +++++++++++++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 86ac9ff..ab9ade0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1330,7 +1330,8 @@ i915_gem_object_unpin_fence(struct drm_i915_gem_object *obj)
 
 void i915_gem_retire_requests(struct drm_device *dev);
 void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
-int __must_check i915_gem_check_wedge(struct drm_i915_private *dev_priv);
+int __must_check i915_gem_check_wedge(struct drm_i915_private *dev_priv,
+				      bool interruptible);
 
 void i915_gem_reset(struct drm_device *dev);
 void i915_gem_clflush_object(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1214850..af6a510 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1864,10 +1864,9 @@ i915_gem_retire_work_handler(struct work_struct *work)
 }
 
 int
-i915_gem_check_wedge(struct drm_i915_private *dev_priv)
+i915_gem_check_wedge(struct drm_i915_private *dev_priv,
+		     bool interruptible)
 {
-	BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
-
 	if (atomic_read(&dev_priv->mm.wedged)) {
 		struct completion *x = &dev_priv->error_completion;
 		bool recovery_complete;
@@ -1878,7 +1877,16 @@ i915_gem_check_wedge(struct drm_i915_private *dev_priv)
 		recovery_complete = x->done > 0;
 		spin_unlock_irqrestore(&x->wait.lock, flags);
 
-		return recovery_complete ? -EIO : -EAGAIN;
+		/* Non-interruptible callers can't handle -EAGAIN, hence return
+		 * -EIO unconditionally for these. */
+		if (!interruptible)
+			return -EIO;
+
+		/* Recovery complete, but still wedged means reset failure. */
+		if (recovery_complete)
+			return -EIO;
+
+		return -EAGAIN;
 	}
 
 	return 0;
@@ -1932,6 +1940,7 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	unsigned long timeout_jiffies;
 	long end;
 	bool wait_forever = true;
+	int ret;
 
 	if (i915_seqno_passed(ring->get_seqno(ring), seqno))
 		return 0;
@@ -1963,8 +1972,9 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 			end = wait_event_timeout(ring->irq_queue, EXIT_COND,
 						 timeout_jiffies);
 
-		if (atomic_read(&dev_priv->mm.wedged))
-			end = -EAGAIN;
+		ret = i915_gem_check_wedge(dev_priv, interruptible);
+		if (ret)
+			end = ret;
 	} while (end == 0 && wait_forever);
 
 	getrawmonotonic(&now);
@@ -2004,7 +2014,7 @@ i915_wait_seqno(struct intel_ring_buffer *ring, uint32_t seqno)
 
 	BUG_ON(seqno == 0);
 
-	ret = i915_gem_check_wedge(dev_priv);
+	ret = i915_gem_check_wedge(dev_priv, dev_priv->mm.interruptible);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1e86894..6c024d4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1221,7 +1221,7 @@ int intel_wait_ring_buffer(struct intel_ring_buffer *ring, int n)
 
 		msleep(1);
 
-		ret = i915_gem_check_wedge(dev_priv);
+		ret = i915_gem_check_wedge(dev_priv, dev_priv->mm.interruptible);
 		if (ret)
 			return ret;
 	} while (!time_after(jiffies, end));
-- 
1.7.10

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Chris Wilson]

On Sun, 24 Jun 2012 16:42:34 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We have a careful logic in i915_gem_check_wedge already to asses whether
> the gpu reset has been tried already and hence whether to return -EIO
> or -EAGAIN.
> 
> Having this early check in intel_ring_begin doesn't buy us anything,
> since we'll be calling into wait_request in the usual case already
> anyway. In the corner case of not waiting for free space using the
> last_retired_head we simply need to do the same check, too.
> 
> With these changes we'll only ever get an -EIO from intel_ring_begin
> if the gpu has truely been declared dead.

The intention here was that only operations that queued a command to a
wedged GPU would trigger the EIO. That has been superseded by the error
checking during mutex acquisition. The patch looks sane, but I'd like
it split into two, one to remove the wedged check before
intel_ring_begin (with some of the rationale about it now being
superfluous) and a separate patch to reuse check_wedge().

It looks like the patch to reuse check_wedge() should be first as it is
the common theme in the series.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: non-interruptible sleeps can't handle -EGAIN

[Daniel Vetter]

On Mon, Jun 25, 2012 at 09:23:38PM +0100, Chris Wilson wrote:
> On Sun, 24 Jun 2012 16:42:36 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > So don't return -EAGAIN, even in the case of a gpu hang. Remap it to -EIO
> > instead.
> > 
> > This is a bit ugly because intel_ring_begin is all non-interruptible
> > and hence only returns -EIO. But as the comment in there says,
> > auditing all the callsites would be a pain.
> > 
> > To avoid duplicating code, reuse i915_gem_check_wedge in __wait_seqno.
> > Use the opportunity to clarify the different cases a bit with
> > comments.
> 
> Two separate patches. The first chunk sounds reasonable,
> non-interruptible is a little bit of misnomer here, we mostly use it to
> mean NOFAIL, and none of the callers are setup to handle EAGAIN. So
> reporting EIO seems reasonable.

This started as a patch to avoid returning -EAGAIN from intel_ring_begin
if mm.interruptible == false. After having added the same return -EIO
logic at a few places, I've noticed that I might as well shove it into
check_wedge and also call check_wedge from __wait_seqno (the older patch
check the retvalues at a few places instead).

Splitting this up would leave is in a state where we avoid returning
-EAGAIN at a few places within the functions called by intel_ring_begin,
but not all of them. Hence I think this should be all in one patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Daniel Vetter]

On Mon, Jun 25, 2012 at 09:32:23PM +0100, Chris Wilson wrote:
> On Sun, 24 Jun 2012 16:42:34 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We have a careful logic in i915_gem_check_wedge already to asses whether
> > the gpu reset has been tried already and hence whether to return -EIO
> > or -EAGAIN.
> > 
> > Having this early check in intel_ring_begin doesn't buy us anything,
> > since we'll be calling into wait_request in the usual case already
> > anyway. In the corner case of not waiting for free space using the
> > last_retired_head we simply need to do the same check, too.
> > 
> > With these changes we'll only ever get an -EIO from intel_ring_begin
> > if the gpu has truely been declared dead.
> 
> The intention here was that only operations that queued a command to a
> wedged GPU would trigger the EIO. That has been superseded by the error
> checking during mutex acquisition. The patch looks sane, but I'd like
> it split into two, one to remove the wedged check before
> intel_ring_begin (with some of the rationale about it now being
> superfluous) and a separate patch to reuse check_wedge().
> 
> It looks like the patch to reuse check_wedge() should be first as it is
> the common theme in the series.

Hm, actually I think I'll smash the check_wedge into the last patch. With
that change, this patch would solely be about not returning spurious -EIO,
whereas the last patch would be solely about not returning -EAGAIN in
cases we can't handle. Does that make some sense?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Chris Wilson]

On Mon, 25 Jun 2012 22:49:03 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jun 25, 2012 at 09:32:23PM +0100, Chris Wilson wrote:
> > It looks like the patch to reuse check_wedge() should be first as it is
> > the common theme in the series.
> 
> Hm, actually I think I'll smash the check_wedge into the last patch. With
> that change, this patch would solely be about not returning spurious -EIO,
> whereas the last patch would be solely about not returning -EAGAIN in
> cases we can't handle. Does that make some sense?

The split sounds reasonable, grouping the patch in that manner should
give a better story. My only holdout is that I don't want to lose the
papering in i915_reset().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Daniel Vetter]

On Mon, Jun 25, 2012 at 11:06 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, 25 Jun 2012 22:49:03 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Jun 25, 2012 at 09:32:23PM +0100, Chris Wilson wrote:
>> > It looks like the patch to reuse check_wedge() should be first as it is
>> > the common theme in the series.
>>
>> Hm, actually I think I'll smash the check_wedge into the last patch. With
>> that change, this patch would solely be about not returning spurious -EIO,
>> whereas the last patch would be solely about not returning -EAGAIN in
>> cases we can't handle. Does that make some sense?
>
> The split sounds reasonable, grouping the patch in that manner should
> give a better story. My only holdout is that I don't want to lose the
> papering in i915_reset().

Hm, I'm not yet convinced on the quality of that ductape. I've went
with the unconditional mutex_lock, deadlocks be damned, approach
because:
- QA has a machine that seemingly _always_ hits this problem. Here it
depends upon the machine and ranges from "fails after a few hundred
reset cycles" to "fails after 5 runs at most".
- Eric complained that when developing new userspace driver code the
gpu gets wedged every once in a while resetting his gpu pretty much
after every compile&run cycle.
So to make life easier for QA and userspace driver devs I've opted to
make it succed (or deadlock), and presuming the kernel code actually
works. Now kernel devs obviously prefer to blows things up with an
OOPS or two, so I see that we should have some balance. But even there
my thinking is that waiting for the stuck process backtrace is better
than trying to paper over severe issues when the systems has clearly
lost its mind already.

So essentially I still fail to see the upside of your proposed ductape
... In either case I guess a walk to the reset button is inevitable
every once in a while ;-)

Yours, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Chris Wilson]

On Mon, 25 Jun 2012 23:48:01 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> So essentially I still fail to see the upside of your proposed ductape
> ... In either case I guess a walk to the reset button is inevitable
> every once in a while ;-)

A false positive for declaring a GPU wedged in a situation that should
have never occurred in the first place is a recoverable and minor
inconvenience compared to locking the display and possibly the system up.

An alternative is to incorporate the deadlock detection into
i915_mutex_lock_interruptible() and make it report -EIO if it waits
longer than 10s, f.e., for the reset to complete. Then the only danger
are the few paths that do not perform the error checking lock.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Daniel Vetter]

On Tue, Jun 26, 2012 at 12:52 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, 25 Jun 2012 23:48:01 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
>> So essentially I still fail to see the upside of your proposed ductape
>> ... In either case I guess a walk to the reset button is inevitable
>> every once in a while ;-)
>
> A false positive for declaring a GPU wedged in a situation that should
> have never occurred in the first place is a recoverable and minor
> inconvenience compared to locking the display and possibly the system up.
>
> An alternative is to incorporate the deadlock detection into
> i915_mutex_lock_interruptible() and make it report -EIO if it waits
> longer than 10s, f.e., for the reset to complete. Then the only danger
> are the few paths that do not perform the error checking lock.

I kinda like this idea - all unconditional mutex_lockers would
deadlock in the same way as i915_reset, but if we've managed to
sprinkle our special reset aware trylock code at all the right places,
at least userspace should get to the -EIO eventually and do something
sensible. I guess if someone is indeed hogging dev->struct_mutex
somehow (which /should/ be the only thing preventing i915_reset from
doing its job) there's not much userspace could actually do - it would
inevitably die on the next gtt pagefault. But I guess we can etch out
a bit more survivability in corner cases.

I'll see what this looks like in actual code tomorrow.

Thanks, Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm/i915: don't return a spurious -EIO from intel_ring_begin

[Chris Wilson]

On Tue, 26 Jun 2012 01:05:24 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> it would
> inevitably die on the next gtt pagefault. But I guess we can etch out
> a bit more survivability in corner cases.

Yeah, we need to do something about that pagefault-of-doom after EIO,
regardless. :(

I believe to do so we need a lot of the buffer management to be
resilient to EIO. But for proposed change to
i915_mutex_lock_interruptible(), I suggest we do something like
 int i915_mutex_lock_wedged()
 {
   ret = i915_mutex_lock_interruptible(dev);
   if (ret) {
     if (ret == -EIO) {
       if (mutex_trylock(&dev->struct_mutex))
         ret = 0;
       else
         ret = -EAGAIN;
     }       
   }
   return ret;
 }
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: wrap up gt powersave enabling functions

[Ben Widawsky]

On Sun, 24 Jun 2012 16:42:32 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> ... instead of calling each one for each generation indiviudally.
> 
> Notice that we've already managed to be inconsistent, the resume path
> is missing an IS_VLV check. As a nice benefit we can mark all the
> platform specific enable/disable functions as static and hide them in
> intel_pm.c
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I like what you've done here. Some comments inline, still.

> ---
>  drivers/gpu/drm/i915/i915_suspend.c  |    5 +----
>  drivers/gpu/drm/i915/intel_display.c |   21 ++-----------------
>  drivers/gpu/drm/i915/intel_drv.h     |    9 ++-------
>  drivers/gpu/drm/i915/intel_pm.c      |   37 +++++++++++++++++++++++++++-------
>  4 files changed, 35 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 0ede02a..740c076 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -825,10 +825,7 @@ int i915_save_state(struct drm_device *dev)
>  		dev_priv->saveIMR = I915_READ(IMR);
>  	}
>  
> -	if (IS_IRONLAKE_M(dev))
> -		ironlake_disable_drps(dev);
> -	if (INTEL_INFO(dev)->gen >= 6)
> -		gen6_disable_rps(dev);
> +	intel_disable_gt_powersave(dev);
>  
>  	/* Cache mode state */
>  	dev_priv->saveCACHE_MODE_0 = I915_READ(CACHE_MODE_0);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b3052ef..fdca5b9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7172,20 +7172,9 @@ static void ivb_pch_pwm_override(struct drm_device *dev)
>  
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
>  	intel_init_clock_gating(dev);
>  
> -	if (IS_IRONLAKE_M(dev)) {
> -		ironlake_enable_drps(dev);
> -		ironlake_enable_rc6(dev);
> -		intel_init_emon(dev);
> -	}
> -
> -	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> -		gen6_enable_rps(dev_priv);
> -		gen6_update_ring_freq(dev_priv);
> -	}
> +	intel_enable_gt_powersave(dev);
>  
>  	if (IS_IVYBRIDGE(dev))
>  		ivb_pch_pwm_override(dev);
> @@ -7277,13 +7266,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_disable_fbc(dev);
>  
> -	if (IS_IRONLAKE_M(dev))
> -		ironlake_disable_drps(dev);
> -	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev))
> -		gen6_disable_rps(dev);
> -
> -	if (IS_IRONLAKE_M(dev))
> -		ironlake_disable_rc6(dev);
> +	intel_disable_gt_powersave(dev);
>  
>  	if (IS_VALLEYVIEW(dev))
>  		vlv_init_dpio(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5290e9d..cc1573b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -425,9 +425,6 @@ extern void intel_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
>  extern void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
>  				    u16 *blue, int regno);
>  extern void intel_enable_clock_gating(struct drm_device *dev);
> -extern void ironlake_disable_rc6(struct drm_device *dev);
> -extern void ironlake_enable_drps(struct drm_device *dev);
> -extern void ironlake_disable_drps(struct drm_device *dev);
>  
>  extern int intel_pin_and_fence_fb_obj(struct drm_device *dev,
>  				      struct drm_i915_gem_object *obj,
> @@ -494,10 +491,8 @@ extern void intel_update_fbc(struct drm_device *dev);
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
> -extern void gen6_enable_rps(struct drm_i915_private *dev_priv);
> -extern void gen6_update_ring_freq(struct drm_i915_private *dev_priv);
> -extern void gen6_disable_rps(struct drm_device *dev);
> -extern void intel_init_emon(struct drm_device *dev);
> +extern void intel_enable_gt_powersave(struct drm_device *dev);
> +extern void intel_disable_gt_powersave(struct drm_device *dev);
>  
>  extern void intel_ddi_dpms(struct drm_encoder *encoder, int mode);
>  extern void intel_ddi_mode_set(struct drm_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7504fbc..2baba10 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2184,7 +2184,7 @@ bool ironlake_set_drps(struct drm_device *dev, u8 val)
>  	return true;
>  }
>  
> -void ironlake_enable_drps(struct drm_device *dev)
> +static void ironlake_enable_drps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 rgvmodectl = I915_READ(MEMMODECTL);
> @@ -2248,7 +2248,7 @@ void ironlake_enable_drps(struct drm_device *dev)
>  	getrawmonotonic(&dev_priv->last_time2);
>  }
>  
> -void ironlake_disable_drps(struct drm_device *dev)
> +static void ironlake_disable_drps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u16 rgvswctl = I915_READ16(MEMSWCTL);
> @@ -2301,7 +2301,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	dev_priv->cur_delay = val;
>  }
>  
> -void gen6_disable_rps(struct drm_device *dev)
> +static void gen6_disable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -2349,7 +2349,7 @@ int intel_enable_rc6(const struct drm_device *dev)
>  	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
>  }
>  
> -void gen6_enable_rps(struct drm_i915_private *dev_priv)
> +static void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_ring_buffer *ring;
>  	u32 rp_state_cap;
> @@ -2494,7 +2494,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
>  }
>  
> -void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
> +static void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
>  {
>  	int min_freq = 15;
>  	int gpu_freq, ia_freq, max_ia_freq;
> @@ -3156,8 +3156,7 @@ void intel_gpu_ips_teardown(void)
>  	i915_mch_dev = NULL;
>  	spin_unlock(&mchdev_lock);
>  }
> -
> -void intel_init_emon(struct drm_device *dev)
> +static void intel_init_emon(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 lcfuse;
> @@ -3228,6 +3227,30 @@ void intel_init_emon(struct drm_device *dev)
>  	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
>  }
>  
> +void intel_disable_gt_powersave(struct drm_device *dev)
> +{
> +	if (IS_IRONLAKE_M(dev))
> +		ironlake_disable_drps(dev);
> +	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> +		gen6_disable_rps(dev);
> +}

What happened to ironlake_disable_rc6?

> +
> +void intel_enable_gt_powersave(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (IS_IRONLAKE_M(dev)) {
> +		ironlake_enable_drps(dev);
> +		ironlake_enable_rc6(dev);
> +		intel_init_emon(dev);
> +	}
> +
> +	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> +		gen6_enable_rps(dev_priv);
> +		gen6_update_ring_freq(dev_priv);
> +	}
> +}
> +
>  static void ironlake_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;


It'd be nice if there was some explanation of why disable doesn't do the
reverse of enable.


-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/5] drm/i915: wrap up gt powersave enabling functions

[Daniel Vetter]

On Fri, Jun 29, 2012 at 11:51:35AM -0700, Ben Widawsky wrote:
> On Sun, 24 Jun 2012 16:42:32 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +void intel_disable_gt_powersave(struct drm_device *dev)
> > +{
> > +	if (IS_IRONLAKE_M(dev))
> > +		ironlake_disable_drps(dev);
> > +	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> > +		gen6_disable_rps(dev);
> > +}
> 
> What happened to ironlake_disable_rc6?

Lost in refactoring. I've noticed this while playing around with a few
things on my ilk myself, but somehow forgot to send out the fixup. Shame
on me :(
> 
> > +
> > +void intel_enable_gt_powersave(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	if (IS_IRONLAKE_M(dev)) {
> > +		ironlake_enable_drps(dev);
> > +		ironlake_enable_rc6(dev);
> > +		intel_init_emon(dev);
> > +	}
> > +
> > +	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> > +		gen6_enable_rps(dev_priv);
> > +		gen6_update_ring_freq(dev_priv);
> > +	}
> > +}
> > +
> >  static void ironlake_init_clock_gating(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> 
> 
> It'd be nice if there was some explanation of why disable doesn't do the
> reverse of enable.

Well, the enable_rc6 and init_emon are a bit at the wrong place and might
need to be split-up and moved around (i.e. hw frobbing should be here,
object alloc and other such setup stuff moved out). It's not the only
place where we suck in this way, though ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48