forcewake v5

forcewake v5

[Ben Widawsky]

Once you're done laughing that I got up to v5 (I think I skipped a v3,
and v1 was the IOCTls, so it's really more like a v3) for patches meant
to simply allow register reads and wites...

Back to struct_mutex since it turned out to be my mistake regarding the
IIR problem. The only issue in the interrupt handler was rps, which has
been moved to a workqueue.

Struct_mutex must be held to forcewake, or relinquish forcwake.

There is still at least one known problem with the hangcheck code where
it reads registers that aren't safe for gen6, and acquiring
struct_mutex. Moving that code to a workqueue became quite involved, and
since it's mostly an error condition, I am not including it in this
series. Hopefully no other code is trying to read or write registers
while the GPU is potentially hanging.

The reference count is now atomic so that the system may possibly almost
work (at least as well as it works now), if callers don't have
struct_mutex.

This series is potentially more dangerous than the previous because of
the way the PM interrupts are handled for rps.

Ben

[PATCH 1/5] drm/i915: proper use of forcewake

[Ben Widawsky]

Moved the macros around to properly do reads and writes for the given
GPU. This is to address special requirements for gen6 (SNB) reads and
writes.

Registers in the range 0-0x40000 on gen6 platforms require special
handling. Instead of relying on the callers to pick the registers
correctly, move the logic into the read and write functions.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |   56 ++++++++++++++----------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    4 +-
 2 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..8981173 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1313,9 +1313,31 @@ extern void intel_display_print_error_state(struct seq_file *m,
 } while (0)
 
 
+
+/* On SNB platform, before reading ring registers forcewake bit
+ * must be set to prevent GT core from power down and stale values being
+ * returned.
+ */
+void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
+void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
+void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
+
+/* We give fast paths for the really cool registers */
+#define NEEDS_FORCE_WAKE(dev_priv, reg) \
+	(((dev_priv)->info->gen >= 6) && \
+	((reg) < 0x40000) && \
+	((reg) != FORCEWAKE) )
+
 #define __i915_read(x, y) \
 static inline u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
-	u##x val = read##y(dev_priv->regs + reg); \
+	u##x val = 0; \
+	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
+		__gen6_gt_force_wake_get(dev_priv); \
+		val = read##y(dev_priv->regs + reg); \
+		__gen6_gt_force_wake_put(dev_priv); \
+	} else { \
+		val = read##y(dev_priv->regs + reg); \
+	} \
 	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
 	return val; \
 }
@@ -1328,6 +1350,9 @@ __i915_read(64, q)
 #define __i915_write(x, y) \
 static inline void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	trace_i915_reg_rw(true, reg, val, sizeof(val)); \
+	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
+		__gen6_gt_wait_for_fifo(dev_priv); \
+	} \
 	write##y(val, dev_priv->regs + reg); \
 }
 __i915_write(8, b)
@@ -1356,33 +1381,4 @@ __i915_write(64, q)
 #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
 
 
-/* On SNB platform, before reading ring registers forcewake bit
- * must be set to prevent GT core from power down and stale values being
- * returned.
- */
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
-void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
-
-static inline u32 i915_gt_read(struct drm_i915_private *dev_priv, u32 reg)
-{
-	u32 val;
-
-	if (dev_priv->info->gen >= 6) {
-		__gen6_gt_force_wake_get(dev_priv);
-		val = I915_READ(reg);
-		__gen6_gt_force_wake_put(dev_priv);
-	} else
-		val = I915_READ(reg);
-
-	return val;
-}
-
-static inline void i915_gt_write(struct drm_i915_private *dev_priv,
-				u32 reg, u32 val)
-{
-	if (dev_priv->info->gen >= 6)
-		__gen6_gt_wait_for_fifo(dev_priv);
-	I915_WRITE(reg, val);
-}
 #endif
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f23cc5f..8e8821f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -14,8 +14,8 @@ struct  intel_hw_status_page {
 	struct		drm_i915_gem_object *obj;
 };
 
-#define I915_RING_READ(reg) i915_gt_read(dev_priv, reg)
-#define I915_RING_WRITE(reg, val) i915_gt_write(dev_priv, reg, val)
+#define I915_RING_READ(reg) I915_READ(reg)
+#define I915_RING_WRITE(reg, val) I915_WRITE(reg, val)
 
 #define I915_READ_TAIL(ring) I915_RING_READ(RING_TAIL((ring)->mmio_base))
 #define I915_WRITE_TAIL(ring, val) I915_RING_WRITE(RING_TAIL((ring)->mmio_base), val)
-- 
1.7.3.4

[PATCH 2/5] drm/i915: reference counted forcewake

[Ben Widawsky]

Provide a reference count to track the forcewake state of the GPU.
Which exports a mechanism to allow userspace to wake the GT. This also
potentially saves a UC read if the GT is known to be awake already.

The reference count is atomic, but the register access and hardware wake
sequence is protected by struct_mutex.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    4 ++--
 drivers/gpu/drm/i915/i915_drv.c      |   21 +++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h      |   18 ++++++++++++++----
 drivers/gpu/drm/i915/i915_irq.c      |    1 -
 drivers/gpu/drm/i915/intel_display.c |    8 ++++----
 5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 87c8e29..3cb0722 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -873,7 +873,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		int max_freq;
 
 		/* RPSTAT1 is in the GT power well */
-		__gen6_gt_force_wake_get(dev_priv);
+		gen6_gt_force_wake_get(dev_priv);
 
 		rpstat = I915_READ(GEN6_RPSTAT1);
 		rpupei = I915_READ(GEN6_RP_CUR_UP_EI);
@@ -918,7 +918,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		seq_printf(m, "Max non-overclocked (RP0) frequency: %dMHz\n",
 			   max_freq * 50);
 
-		__gen6_gt_force_wake_put(dev_priv);
+		gen6_gt_force_wake_put(dev_priv);
 	} else {
 		seq_printf(m, "no P-state info available\n");
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c34a8dd..a4919ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -263,7 +263,7 @@ void intel_detect_pch (struct drm_device *dev)
 	}
 }
 
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
+static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
 	int count;
 
@@ -279,12 +279,29 @@ void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 		udelay(10);
 }
 
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	/* Forcewake is atomic in case we get in here without the lock */
+	if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
+		__gen6_gt_force_wake_get(dev_priv);
+}
+
+static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
 	POSTING_READ(FORCEWAKE);
 }
 
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	if (atomic_dec_and_test(&dev_priv->forcewake_count))
+		__gen6_gt_force_wake_put(dev_priv);
+}
+
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
 	int loop = 500;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8981173..1cd5a76 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,8 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	atomic_t forcewake_count;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
@@ -1318,8 +1320,8 @@ extern void intel_display_print_error_state(struct seq_file *m,
  * must be set to prevent GT core from power down and stale values being
  * returned.
  */
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 
 /* We give fast paths for the really cool registers */
@@ -1332,15 +1334,23 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 static inline u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 	u##x val = 0; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__gen6_gt_force_wake_get(dev_priv); \
+		gen6_gt_force_wake_get(dev_priv); \
 		val = read##y(dev_priv->regs + reg); \
-		__gen6_gt_force_wake_put(dev_priv); \
+		gen6_gt_force_wake_put(dev_priv); \
 	} else { \
 		val = read##y(dev_priv->regs + reg); \
 	} \
 	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
 	return val; \
+} \
+\
+static inline u##x i915_read##x##_awake(struct drm_i915_private *dev_priv, u32 reg) { \
+	u##x val = 0; \
+	val = read##y(dev_priv->regs + reg); \
+	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
+	return val; \
 }
+
 __i915_read(8, b)
 __i915_read(16, w)
 __i915_read(32, l)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 188b497..c6062c3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -396,7 +396,6 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
 			I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
 				   I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
 		}
-
 	}
 
 	gen6_set_rps(dev, new_delay);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e522c70..f6780cf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1828,7 +1828,7 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 	u32 blt_ecoskpd;
 
 	/* Make sure blitter notifies FBC of writes */
-	__gen6_gt_force_wake_get(dev_priv);
+	gen6_gt_force_wake_get(dev_priv);
 	blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
 	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
 		GEN6_BLITTER_LOCK_SHIFT;
@@ -1839,7 +1839,7 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 			 GEN6_BLITTER_LOCK_SHIFT);
 	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
 	POSTING_READ(GEN6_BLITTER_ECOSKPD);
-	__gen6_gt_force_wake_put(dev_priv);
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
@@ -6860,7 +6860,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	 * userspace...
 	 */
 	I915_WRITE(GEN6_RC_STATE, 0);
-	__gen6_gt_force_wake_get(dev_priv);
+	gen6_gt_force_wake_get(dev_priv);
 
 	/* disable the counters and set deterministic thresholds */
 	I915_WRITE(GEN6_RC_CONTROL, 0);
@@ -6958,7 +6958,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	/* enable all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
-	__gen6_gt_force_wake_put(dev_priv);
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 void intel_enable_clock_gating(struct drm_device *dev)
-- 
1.7.3.4

[PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    2 ++
 drivers/gpu/drm/i915/intel_display.c |   12 ++++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3cb0722..94b38f3 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -873,6 +873,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		int max_freq;
 
 		/* RPSTAT1 is in the GT power well */
+		mutex_lock(&dev->struct_mutex);
 		gen6_gt_force_wake_get(dev_priv);
 
 		rpstat = I915_READ(GEN6_RPSTAT1);
@@ -919,6 +920,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 			   max_freq * 50);
 
 		gen6_gt_force_wake_put(dev_priv);
+		mutex_unlock(&dev->struct_mutex);
 	} else {
 		seq_printf(m, "no P-state info available\n");
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f6780cf..51dcb3f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2873,7 +2873,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		ironlake_pch_enable(crtc);
 
 	intel_crtc_load_lut(crtc);
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
+
 	intel_crtc_update_cursor(crtc, true);
 }
 
@@ -2969,8 +2973,11 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc->active = false;
 	intel_update_watermarks(dev);
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	intel_clear_scanline_wait(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_disable_pll(dev_priv, pipe);
 
 	intel_crtc->active = false;
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	intel_update_watermarks(dev);
 	intel_clear_scanline_wait(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -6860,6 +6870,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	 * userspace...
 	 */
 	I915_WRITE(GEN6_RC_STATE, 0);
+	mutex_lock(&dev_priv->dev->struct_mutex);
 	gen6_gt_force_wake_get(dev_priv);
 
 	/* disable the counters and set deterministic thresholds */
@@ -6959,6 +6970,7 @@ 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);
 }
 
 void intel_enable_clock_gating(struct drm_device *dev)
-- 
1.7.3.4

[PATCH 4/5] drm/i915: move gen6 rps handling to workqueue

[Ben Widawsky]

The render P-state handling code requires reading from a GT register.
This means that FORCEWAKE must be written to, a resource which is shared
and should be protected by struct_mutex.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c |    1 +
 drivers/gpu/drm/i915/i915_drv.h |    4 +++
 drivers/gpu/drm/i915/i915_irq.c |   47 +++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_reg.h |    5 +++-
 4 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..855355e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2025,6 +2025,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->error_lock);
+	spin_lock_init(&dev_priv->rps_lock);
 
 	if (IS_MOBILE(dev) || !IS_GEN2(dev))
 		dev_priv->num_pipe = 2;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1cd5a76..4faa7e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -676,6 +676,10 @@ typedef struct drm_i915_private {
 
 	bool mchbar_need_disable;
 
+	struct work_struct rps_work;
+	spinlock_t rps_lock;
+	u32 pm_iir;
+
 	u8 cur_delay;
 	u8 min_delay;
 	u8 max_delay;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c6062c3..2d9f751 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -367,22 +367,30 @@ static void notify_ring(struct drm_device *dev,
 		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
 }
 
-static void gen6_pm_irq_handler(struct drm_device *dev)
+static void gen6_pm_rps_work(struct work_struct *work)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
+						    rps_work);
 	u8 new_delay = dev_priv->cur_delay;
-	u32 pm_iir;
+	u32 pm_iir, pm_imr;
+
+	spin_lock_irq(&dev_priv->rps_lock);
+	pm_iir = dev_priv->pm_iir;
+	dev_priv->pm_iir = 0;
+	pm_imr = I915_READ(GEN6_PMIMR);
+	spin_unlock_irq(&dev_priv->rps_lock);
 
-	pm_iir = I915_READ(GEN6_PMIIR);
 	if (!pm_iir)
 		return;
 
+	mutex_lock(&dev_priv->dev->struct_mutex);
 	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
 		if (dev_priv->cur_delay != dev_priv->max_delay)
 			new_delay = dev_priv->cur_delay + 1;
 		if (new_delay > dev_priv->max_delay)
 			new_delay = dev_priv->max_delay;
 	} else if (pm_iir & (GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT)) {
+		gen6_gt_force_wake_get(dev_priv);
 		if (dev_priv->cur_delay != dev_priv->min_delay)
 			new_delay = dev_priv->cur_delay - 1;
 		if (new_delay < dev_priv->min_delay) {
@@ -396,13 +404,18 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
 			I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
 				   I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
 		}
+		gen6_gt_force_wake_put(dev_priv);
 	}
 
-	gen6_set_rps(dev, new_delay);
+	gen6_set_rps(dev_priv->dev, new_delay);
 	dev_priv->cur_delay = new_delay;
+	mutex_unlock(&dev_priv->dev->struct_mutex);
 
-	I915_WRITE(GEN6_PMIIR, pm_iir);
-}
+	/*
+	 * Lock not held here, because clearing is non-destructive, and
+	 * the interrupt handler is the only other place where it is written.
+	 */
+	I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); }
 
 static void pch_irq_handler(struct drm_device *dev)
 {
@@ -525,13 +538,28 @@ static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
 		i915_handle_rps_change(dev);
 	}
 
-	if (IS_GEN6(dev))
-		gen6_pm_irq_handler(dev);
+	if (IS_GEN6(dev) && pm_iir)  {
+		if (pm_iir & (GEN6_PM_DEFERRED_EVENTS)) {
+			unsigned long flags;
+			/* Make sure no new interrupts come in */
+			spin_lock_irqsave(&dev_priv->rps_lock, flags);
+			I915_WRITE(GEN6_PMIMR, pm_iir);
+
+			/* Add the new ones */
+			BUG_ON(dev_priv->pm_iir & pm_iir);
+			dev_priv->pm_iir |= pm_iir;
+			spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
+
+			/* queue it up */
+			queue_work(dev_priv->wq, &dev_priv->rps_work);
+		}
+	}
 
 	/* should clear PCH hotplug event before clear CPU irq */
 	I915_WRITE(SDEIIR, pch_iir);
 	I915_WRITE(GTIIR, gt_iir);
 	I915_WRITE(DEIIR, de_iir);
+	I915_WRITE(GEN6_PMIIR, pm_iir);
 
 done:
 	I915_WRITE(DEIER, de_ier);
@@ -1658,6 +1686,7 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
 
 	INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
 	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
+	INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
 
 	if (HAS_PCH_SPLIT(dev)) {
 		ironlake_irq_preinstall(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f39ac3a..9774a2e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3386,7 +3386,7 @@
 #define GEN6_PMINTRMSK				0xA168
 
 #define GEN6_PMISR				0x44020
-#define GEN6_PMIMR				0x44024
+#define GEN6_PMIMR				0x44024 /* Protected by rps_lock */
 #define GEN6_PMIIR				0x44028
 #define GEN6_PMIER				0x4402C
 #define  GEN6_PM_MBOX_EVENT			(1<<25)
@@ -3396,6 +3396,9 @@
 #define  GEN6_PM_RP_DOWN_THRESHOLD		(1<<4)
 #define  GEN6_PM_RP_UP_EI_EXPIRED		(1<<2)
 #define  GEN6_PM_RP_DOWN_EI_EXPIRED		(1<<1)
+#define  GEN6_PM_DEFERRED_EVENTS		(GEN6_PM_RP_UP_THRESHOLD | \
+						 GEN6_PM_RP_DOWN_THRESHOLD | \
+						 GEN6_PM_RP_DOWN_TIMEOUT)
 
 #define GEN6_PCODE_MAILBOX			0x138124
 #define   GEN6_PCODE_READY			(1<<31)
-- 
1.7.3.4

[PATCH 5/5] drm/i915: debugfs interface for forcewake reference count

[Ben Widawsky]

forcewake is controlled by the open and close of the debugfs file. This
assures that buggy applications cannot cause the GT to stay on forever.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   80 +++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 94b38f3..5ce8a5b 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1188,6 +1188,18 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	seq_printf(m, "forcewake count = %d\n",
+		   atomic_read(&dev_priv->forcewake_count));
+
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1290,6 +1302,67 @@ static int i915_wedged_create(struct dentry *root, struct drm_minor *minor)
 	return drm_add_fake_info_node(minor, ent, &i915_wedged_fops);
 }
 
+static int i915_forcewake_open(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (!IS_GEN6(dev))
+		return 0;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+	gen6_gt_force_wake_get(dev_priv);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+int i915_forcewake_release(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!IS_GEN6(dev))
+		return 0;
+
+	/*
+	 * It's bad that we can potentially hang userspace if struct_mutex gets
+	 * forever stuck.  However, if we cannot acquire this lock it means that
+	 * almost certainly the driver has hung, is not unload-able. Therefore
+	 * hanging here is probably a minor inconvenience not to be seen my
+	 * almost every user.
+	 */
+	mutex_lock(&dev->struct_mutex);
+	gen6_gt_force_wake_put(dev_priv);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+static const struct file_operations i915_forcewake_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_forcewake_open,
+	.release = i915_forcewake_release,
+};
+
+static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
+{
+	struct drm_device *dev = minor->dev;
+	struct dentry *ent;
+
+	ent = debugfs_create_file("i915_forcewake_user",
+				  S_IRWXU,
+				  root, dev,
+				  &i915_forcewake_fops);
+	if (IS_ERR(ent))
+		return PTR_ERR(ent);
+
+	return 0;
+}
+
 static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -1326,6 +1399,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915_gen6_forcewake_count", i915_gen6_forcewake_count_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
@@ -1337,6 +1411,10 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	ret = i915_forcewake_create(minor->debugfs_root, minor);
+	if (ret)
+		return ret;
+
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
@@ -1346,6 +1424,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 {
 	drm_debugfs_remove_files(i915_debugfs_list,
 				 I915_DEBUGFS_ENTRIES, minor);
+	drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
+				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_wedged_fops,
 				 1, minor);
 }
-- 
1.7.3.4

Re: [PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Chris Wilson]

On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Just to annoy you, this needs to be split up into the various categories
of fixes. Because...

>  static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
> @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	intel_disable_pll(dev_priv, pipe);
>  
>  	intel_crtc->active = false;
> +
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
>  	intel_update_watermarks(dev);
>  	intel_clear_scanline_wait(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  }

This is overly correct. You can put a comment here to say that we will
never attempt to use FORCEWAKE here and that these registers are protected
by the mode_config lock. Except for intel_clear_scanline_wait, but that
itself is is longing to be killed now. If we haven't fixed the underlying
bug that we were working around by now, we have been too lax.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: move gen6 rps handling to workqueue

[Chris Wilson]

On Wed, 20 Apr 2011 16:53:18 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The render P-state handling code requires reading from a GT register.
> This means that FORCEWAKE must be written to, a resource which is shared
> and should be protected by struct_mutex.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    1 +
>  drivers/gpu/drm/i915/i915_drv.h |    4 +++
>  drivers/gpu/drm/i915/i915_irq.c |   47 +++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_reg.h |    5 +++-
>  4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..855355e 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2025,6 +2025,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	spin_lock_init(&dev_priv->irq_lock);
>  	spin_lock_init(&dev_priv->error_lock);
> +	spin_lock_init(&dev_priv->rps_lock);
>  
>  	if (IS_MOBILE(dev) || !IS_GEN2(dev))
>  		dev_priv->num_pipe = 2;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1cd5a76..4faa7e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -676,6 +676,10 @@ typedef struct drm_i915_private {
>  
>  	bool mchbar_need_disable;
>  
> +	struct work_struct rps_work;
> +	spinlock_t rps_lock;
> +	u32 pm_iir;
> +
>  	u8 cur_delay;
>  	u8 min_delay;
>  	u8 max_delay;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c6062c3..2d9f751 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -367,22 +367,30 @@ static void notify_ring(struct drm_device *dev,
>  		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
>  }
>  
> -static void gen6_pm_irq_handler(struct drm_device *dev)
> +static void gen6_pm_rps_work(struct work_struct *work)
>  {
> -	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> +						    rps_work);
>  	u8 new_delay = dev_priv->cur_delay;
> -	u32 pm_iir;
> +	u32 pm_iir, pm_imr;
> +
> +	spin_lock_irq(&dev_priv->rps_lock);
> +	pm_iir = dev_priv->pm_iir;
> +	dev_priv->pm_iir = 0;
> +	pm_imr = I915_READ(GEN6_PMIMR);
> +	spin_unlock_irq(&dev_priv->rps_lock);
>  
> -	pm_iir = I915_READ(GEN6_PMIIR);
>  	if (!pm_iir)
>  		return;
>  
> +	mutex_lock(&dev_priv->dev->struct_mutex);
>  	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
>  		if (dev_priv->cur_delay != dev_priv->max_delay)
>  			new_delay = dev_priv->cur_delay + 1;
>  		if (new_delay > dev_priv->max_delay)
>  			new_delay = dev_priv->max_delay;
>  	} else if (pm_iir & (GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT)) {
> +		gen6_gt_force_wake_get(dev_priv);
>  		if (dev_priv->cur_delay != dev_priv->min_delay)
>  			new_delay = dev_priv->cur_delay - 1;
>  		if (new_delay < dev_priv->min_delay) {
> @@ -396,13 +404,18 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
>  			I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
>  				   I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
>  		}
> +		gen6_gt_force_wake_put(dev_priv);
>  	}
>  
> -	gen6_set_rps(dev, new_delay);
> +	gen6_set_rps(dev_priv->dev, new_delay);
>  	dev_priv->cur_delay = new_delay;
> +	mutex_unlock(&dev_priv->dev->struct_mutex);
>  
> -	I915_WRITE(GEN6_PMIIR, pm_iir);
> -}
> +	/*
> +	 * Lock not held here, because clearing is non-destructive, and
> +	 * the interrupt handler is the only other place where it is written.
> +	 */
> +	I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); }

But does this register use __gen6_gt_wait_for_fifo? *That* requires a
lock. I'm starting to see a need for I915_READ_IRQ, I915_WRITE_IRQ, so
that the circumstances are explicit and we acknowledge them when modifying
the read/write routines.

>  static void pch_irq_handler(struct drm_device *dev)
>  {
> @@ -525,13 +538,28 @@ static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
>  		i915_handle_rps_change(dev);
>  	}
>  
> -	if (IS_GEN6(dev))
> -		gen6_pm_irq_handler(dev);
> +	if (IS_GEN6(dev) && pm_iir)  {
> +		if (pm_iir & (GEN6_PM_DEFERRED_EVENTS)) {

	if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS) {

roll the two ifs into one to remove a surplus level of nesting and kill
the redundant brackets!

> +			unsigned long flags;
> +			/* Make sure no new interrupts come in */
> +			spin_lock_irqsave(&dev_priv->rps_lock, flags);
> +			I915_WRITE(GEN6_PMIMR, pm_iir);
> +
> +			/* Add the new ones */
> +			BUG_ON(dev_priv->pm_iir & pm_iir);

Bah. The comments are absolutely useless. Really. What you need to
describe is how the use of IMR and IIR is split between the interrupt
handler and the tasklet.

Or maybe they did their job, because I had to go back and read much more
carefully what you were doing...

> +			dev_priv->pm_iir |= pm_iir;
> +			spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
> +
> +			/* queue it up */
> +			queue_work(dev_priv->wq, &dev_priv->rps_work);
> +		}
> +	}
>  
>  	/* should clear PCH hotplug event before clear CPU irq */
>  	I915_WRITE(SDEIIR, pch_iir);
>  	I915_WRITE(GTIIR, gt_iir);
>  	I915_WRITE(DEIIR, de_iir);
> +	I915_WRITE(GEN6_PMIIR, pm_iir);
>  
>  done:
>  	I915_WRITE(DEIER, de_ier);
> @@ -1658,6 +1686,7 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
>  
>  	INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
>  	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
> +	INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
>  	if (HAS_PCH_SPLIT(dev)) {
>  		ironlake_irq_preinstall(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f39ac3a..9774a2e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3386,7 +3386,7 @@
>  #define GEN6_PMINTRMSK				0xA168
>  
>  #define GEN6_PMISR				0x44020
> -#define GEN6_PMIMR				0x44024
> +#define GEN6_PMIMR				0x44024 /* Protected by rps_lock */
>  #define GEN6_PMIIR				0x44028
>  #define GEN6_PMIER				0x4402C
>  #define  GEN6_PM_MBOX_EVENT			(1<<25)
> @@ -3396,6 +3396,9 @@
>  #define  GEN6_PM_RP_DOWN_THRESHOLD		(1<<4)
>  #define  GEN6_PM_RP_UP_EI_EXPIRED		(1<<2)
>  #define  GEN6_PM_RP_DOWN_EI_EXPIRED		(1<<1)
> +#define  GEN6_PM_DEFERRED_EVENTS		(GEN6_PM_RP_UP_THRESHOLD | \
> +						 GEN6_PM_RP_DOWN_THRESHOLD | \
> +						 GEN6_PM_RP_DOWN_TIMEOUT)
>  
>  #define GEN6_PCODE_MAILBOX			0x138124
>  #define   GEN6_PCODE_READY			(1<<31)
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: forcewake v5

[Chris Wilson]

On Wed, 20 Apr 2011 16:53:14 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> Once you're done laughing that I got up to v5 (I think I skipped a v3,
> and v1 was the IOCTls, so it's really more like a v3) for patches meant
> to simply allow register reads and wites...
> 
> Back to struct_mutex since it turned out to be my mistake regarding the
> IIR problem. The only issue in the interrupt handler was rps, which has
> been moved to a workqueue.
> 
> Struct_mutex must be held to forcewake, or relinquish forcwake.

Ok, I got to the end. I feel really squeamish that our register read/write
routines are getting seriously overcomplicated for interrupt work and
would like a cut down version for IRQ context.

But other than that and some silly code that needs to be improved in light
of your review, it's looking good.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: move gen6 rps handling to workqueue

[Ben Widawsky]

On Thu, Apr 21, 2011 at 07:34:08AM +0100, Chris Wilson wrote:

> > +	/*
> > +	 * Lock not held here, because clearing is non-destructive, and
> > +	 * the interrupt handler is the only other place where it is written.
> > +	 */
> > +	I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); }
> 
> But does this register use __gen6_gt_wait_for_fifo? *That* requires a
> lock. I'm starting to see a need for I915_READ_IRQ, I915_WRITE_IRQ, so
> that the circumstances are explicit and we acknowledge them when modifying
> the read/write routines.

GEN6_PMIMR is over 0x40000, so it should be safe. The write to it in the
interrupt handler would also not be safe otherwise.

> 
> > +			unsigned long flags;
> > +			/* Make sure no new interrupts come in */
> > +			spin_lock_irqsave(&dev_priv->rps_lock, flags);
> > +			I915_WRITE(GEN6_PMIMR, pm_iir);
> > +
> > +			/* Add the new ones */
> > +			BUG_ON(dev_priv->pm_iir & pm_iir);
> 
> Bah. The comments are absolutely useless. Really. What you need to
> describe is how the use of IMR and IIR is split between the interrupt
> handler and the tasklet.
> 
> Or maybe they did their job, because I had to go back and read much more
> carefully what you were doing...
> 

Coming up...

Ben

Re: [PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Ben Widawsky]

On Thu, Apr 21, 2011 at 07:18:24AM +0100, Chris Wilson wrote:
> On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Just to annoy you, this needs to be split up into the various categories
> of fixes. Because...
> 
> >  static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
> > @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> >  	intel_disable_pll(dev_priv, pipe);
> >  
> >  	intel_crtc->active = false;
> > +
> > +	mutex_lock(&dev->struct_mutex);
> >  	intel_update_fbc(dev);
> >  	intel_update_watermarks(dev);
> >  	intel_clear_scanline_wait(dev);
> > +	mutex_unlock(&dev->struct_mutex);
> >  }
> 
> This is overly correct. You can put a comment here to say that we will
> never attempt to use FORCEWAKE here and that these registers are protected
> by the mode_config lock. Except for intel_clear_scanline_wait, but that
> itself is is longing to be killed now. If we haven't fixed the underlying
> bug that we were working around by now, we have been too lax.
> -Chris

I don't understand what you're asking for. I'm pretty convinced I need
the mutex protected intel_update_fbc, because the call trace could be:

intel_update_fbc()
intel_enable_fbc()
ironlake_enable_fbc()
sandybridge_blit_fbc_update()
gen6_gt_force_wake_get()


Could you elaborate?

Ben

Re: [PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Ben Widawsky]

On Fri, Apr 22, 2011 at 09:20:17AM -0700, Ben Widawsky wrote:
> On Thu, Apr 21, 2011 at 07:18:24AM +0100, Chris Wilson wrote:
> > On Wed, 20 Apr 2011 16:53:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > Just to annoy you, this needs to be split up into the various categories
> > of fixes. Because...
> > 
> > >  static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
> > > @@ -3067,9 +3074,12 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
> > >  	intel_disable_pll(dev_priv, pipe);
> > >  
> > >  	intel_crtc->active = false;
> > > +
> > > +	mutex_lock(&dev->struct_mutex);
> > >  	intel_update_fbc(dev);
> > >  	intel_update_watermarks(dev);
> > >  	intel_clear_scanline_wait(dev);
> > > +	mutex_unlock(&dev->struct_mutex);
> > >  }
> > 
> > This is overly correct. You can put a comment here to say that we will
> > never attempt to use FORCEWAKE here and that these registers are protected
> > by the mode_config lock. Except for intel_clear_scanline_wait, but that
> > itself is is longing to be killed now. If we haven't fixed the underlying
> > bug that we were working around by now, we have been too lax.
> > -Chris
> 
> I don't understand what you're asking for. I'm pretty convinced I need
> the mutex protected intel_update_fbc, because the call trace could be:
> 
> intel_update_fbc()
> intel_enable_fbc()
> ironlake_enable_fbc()
> sandybridge_blit_fbc_update()
> gen6_gt_force_wake_get()
> 
> 
> Could you elaborate?
> 
> Ben

Crap. I wasn't paying attention. You're right, I did this to be
symmetric with the other code. I can remove it if you prefer.

Re: [PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Chris Wilson]

On Fri, 22 Apr 2011 09:20:17 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I don't understand what you're asking for. I'm pretty convinced I need
> the mutex protected intel_update_fbc, because the call trace could be:
> 
> intel_update_fbc()
> intel_enable_fbc()
> ironlake_enable_fbc()
> sandybridge_blit_fbc_update()
> gen6_gt_force_wake_get()
> 
> 
> Could you elaborate?

It's a pre-ILK code path, so no magic GT handling required.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: forcewake v5

[Ben Widawsky]

On Thu, Apr 21, 2011 at 07:39:27AM +0100, Chris Wilson wrote:
> On Wed, 20 Apr 2011 16:53:14 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > Once you're done laughing that I got up to v5 (I think I skipped a v3,
> > and v1 was the IOCTls, so it's really more like a v3) for patches meant
> > to simply allow register reads and wites...
> > 
> > Back to struct_mutex since it turned out to be my mistake regarding the
> > IIR problem. The only issue in the interrupt handler was rps, which has
> > been moved to a workqueue.
> > 
> > Struct_mutex must be held to forcewake, or relinquish forcwake.
> 
> Ok, I got to the end. I feel really squeamish that our register read/write
> routines are getting seriously overcomplicated for interrupt work and
> would like a cut down version for IRQ context.
> 
> But other than that and some silly code that needs to be improved in light
> of your review, it's looking good.
> -Chris

To save starting a new thread, I'm uploading the fixes to this thread. I
will start a new thread if the fixes are accepted.

Ben

[PATCH 1/5] drm/i915: proper use of forcewake

[Ben Widawsky]

Moved the macros around to properly do reads and writes for the given
GPU. This is to address special requirements for gen6 (SNB) reads and
writes.

Registers in the range 0-0x40000 on gen6 platforms require special
handling. Instead of relying on the callers to pick the registers
correctly, move the logic into the read and write functions.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |   54 ++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |   29 +++++++---------
 2 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5004724..2e57f7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1312,10 +1312,30 @@ extern void intel_display_print_error_state(struct seq_file *m,
 		LOCK_TEST_WITH_RETURN(dev, file);			\
 } while (0)
 
+/* On SNB platform, before reading ring registers forcewake bit
+ * must be set to prevent GT core from power down and stale values being
+ * returned.
+ */
+void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
+void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
+void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
+
+/* We give fast paths for the really cool registers */
+#define NEEDS_FORCE_WAKE(dev_priv, reg) \
+	(((dev_priv)->info->gen >= 6) && \
+	((reg) < 0x40000) && \
+	((reg) != FORCEWAKE))
 
 #define __i915_read(x, y) \
 static inline u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
-	u##x val = read##y(dev_priv->regs + reg); \
+	u##x val = 0; \
+	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
+		__gen6_gt_force_wake_get(dev_priv); \
+		val = read##y(dev_priv->regs + reg); \
+		__gen6_gt_force_wake_put(dev_priv); \
+	} else { \
+		val = read##y(dev_priv->regs + reg); \
+	} \
 	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
 	return val; \
 }
@@ -1328,6 +1348,9 @@ __i915_read(64, q)
 #define __i915_write(x, y) \
 static inline void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	trace_i915_reg_rw(true, reg, val, sizeof(val)); \
+	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
+		__gen6_gt_wait_for_fifo(dev_priv); \
+	} \
 	write##y(val, dev_priv->regs + reg); \
 }
 __i915_write(8, b)
@@ -1356,33 +1379,4 @@ __i915_write(64, q)
 #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
 
 
-/* On SNB platform, before reading ring registers forcewake bit
- * must be set to prevent GT core from power down and stale values being
- * returned.
- */
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
-void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
-
-static inline u32 i915_gt_read(struct drm_i915_private *dev_priv, u32 reg)
-{
-	u32 val;
-
-	if (dev_priv->info->gen >= 6) {
-		__gen6_gt_force_wake_get(dev_priv);
-		val = I915_READ(reg);
-		__gen6_gt_force_wake_put(dev_priv);
-	} else
-		val = I915_READ(reg);
-
-	return val;
-}
-
-static inline void i915_gt_write(struct drm_i915_private *dev_priv,
-				u32 reg, u32 val)
-{
-	if (dev_priv->info->gen >= 6)
-		__gen6_gt_wait_for_fifo(dev_priv);
-	I915_WRITE(reg, val);
-}
 #endif
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f23cc5f..edbaf4c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -14,27 +14,24 @@ struct  intel_hw_status_page {
 	struct		drm_i915_gem_object *obj;
 };
 
-#define I915_RING_READ(reg) i915_gt_read(dev_priv, reg)
-#define I915_RING_WRITE(reg, val) i915_gt_write(dev_priv, reg, val)
+#define I915_READ_TAIL(ring) I915_READ(RING_TAIL((ring)->mmio_base))
+#define I915_WRITE_TAIL(ring, val) I915_WRITE(RING_TAIL((ring)->mmio_base), val)
 
-#define I915_READ_TAIL(ring) I915_RING_READ(RING_TAIL((ring)->mmio_base))
-#define I915_WRITE_TAIL(ring, val) I915_RING_WRITE(RING_TAIL((ring)->mmio_base), val)
+#define I915_READ_START(ring) I915_READ(RING_START((ring)->mmio_base))
+#define I915_WRITE_START(ring, val) I915_WRITE(RING_START((ring)->mmio_base), val)
 
-#define I915_READ_START(ring) I915_RING_READ(RING_START((ring)->mmio_base))
-#define I915_WRITE_START(ring, val) I915_RING_WRITE(RING_START((ring)->mmio_base), val)
+#define I915_READ_HEAD(ring)  I915_READ(RING_HEAD((ring)->mmio_base))
+#define I915_WRITE_HEAD(ring, val) I915_WRITE(RING_HEAD((ring)->mmio_base), val)
 
-#define I915_READ_HEAD(ring)  I915_RING_READ(RING_HEAD((ring)->mmio_base))
-#define I915_WRITE_HEAD(ring, val) I915_RING_WRITE(RING_HEAD((ring)->mmio_base), val)
+#define I915_READ_CTL(ring) I915_READ(RING_CTL((ring)->mmio_base))
+#define I915_WRITE_CTL(ring, val) I915_WRITE(RING_CTL((ring)->mmio_base), val)
 
-#define I915_READ_CTL(ring) I915_RING_READ(RING_CTL((ring)->mmio_base))
-#define I915_WRITE_CTL(ring, val) I915_RING_WRITE(RING_CTL((ring)->mmio_base), val)
+#define I915_READ_IMR(ring) I915_READ(RING_IMR((ring)->mmio_base))
+#define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
 
-#define I915_READ_IMR(ring) I915_RING_READ(RING_IMR((ring)->mmio_base))
-#define I915_WRITE_IMR(ring, val) I915_RING_WRITE(RING_IMR((ring)->mmio_base), val)
-
-#define I915_READ_NOPID(ring) I915_RING_READ(RING_NOPID((ring)->mmio_base))
-#define I915_READ_SYNC_0(ring) I915_RING_READ(RING_SYNC_0((ring)->mmio_base))
-#define I915_READ_SYNC_1(ring) I915_RING_READ(RING_SYNC_1((ring)->mmio_base))
+#define I915_READ_NOPID(ring) I915_READ(RING_NOPID((ring)->mmio_base))
+#define I915_READ_SYNC_0(ring) I915_READ(RING_SYNC_0((ring)->mmio_base))
+#define I915_READ_SYNC_1(ring) I915_READ(RING_SYNC_1((ring)->mmio_base))
 
 struct  intel_ring_buffer {
 	const char	*name;
-- 
1.7.3.4

[PATCH 2/5] drm/i915: reference counted forcewake

[Ben Widawsky]

Provide a reference count to track the forcewake state of the GPU and
give a mechanism for userspace to wake the GT. This also potentially
saves a UC read if the GT is known to be awake already.

The reference count is atomic, but the register access and hardware wake
sequence is protected by struct_mutex.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    4 ++--
 drivers/gpu/drm/i915/i915_drv.c      |   30 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_drv.h      |   11 +++++++----
 drivers/gpu/drm/i915/i915_irq.c      |    1 -
 drivers/gpu/drm/i915/intel_display.c |    8 ++++----
 5 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 87c8e29..3cb0722 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -873,7 +873,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		int max_freq;
 
 		/* RPSTAT1 is in the GT power well */
-		__gen6_gt_force_wake_get(dev_priv);
+		gen6_gt_force_wake_get(dev_priv);
 
 		rpstat = I915_READ(GEN6_RPSTAT1);
 		rpupei = I915_READ(GEN6_RP_CUR_UP_EI);
@@ -918,7 +918,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		seq_printf(m, "Max non-overclocked (RP0) frequency: %dMHz\n",
 			   max_freq * 50);
 
-		__gen6_gt_force_wake_put(dev_priv);
+		gen6_gt_force_wake_put(dev_priv);
 	} else {
 		seq_printf(m, "no P-state info available\n");
 	}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c34a8dd..52e52ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -263,7 +263,7 @@ void intel_detect_pch (struct drm_device *dev)
 	}
 }
 
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
+static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
 	int count;
 
@@ -279,12 +279,38 @@ void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 		udelay(10);
 }
 
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
+/*
+ * Generally this is called implicitly by the register read function. However,
+ * if some sequence requires the GT to not power down then this function should
+ * be called at the beginning of the sequence followed by a call to
+ * gen6_gt_force_wake_put() at the end of the sequence.
+ */
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	/* Forcewake is atomic in case we get in here without the lock */
+	if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
+		__gen6_gt_force_wake_get(dev_priv);
+}
+
+static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
 	POSTING_READ(FORCEWAKE);
 }
 
+/*
+ * see gen6_gt_force_wake_get()
+ */
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
+{
+	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+
+	if (atomic_dec_and_test(&dev_priv->forcewake_count))
+		__gen6_gt_force_wake_put(dev_priv);
+}
+
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
 	int loop = 500;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2e57f7f..140a381 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -703,6 +703,8 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+
+	atomic_t forcewake_count;
 } drm_i915_private_t;
 
 struct drm_i915_gem_object {
@@ -1316,8 +1318,8 @@ extern void intel_display_print_error_state(struct seq_file *m,
  * must be set to prevent GT core from power down and stale values being
  * returned.
  */
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 
 /* We give fast paths for the really cool registers */
@@ -1330,15 +1332,16 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
 static inline u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 	u##x val = 0; \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		__gen6_gt_force_wake_get(dev_priv); \
+		gen6_gt_force_wake_get(dev_priv); \
 		val = read##y(dev_priv->regs + reg); \
-		__gen6_gt_force_wake_put(dev_priv); \
+		gen6_gt_force_wake_put(dev_priv); \
 	} else { \
 		val = read##y(dev_priv->regs + reg); \
 	} \
 	trace_i915_reg_rw(false, reg, val, sizeof(val)); \
 	return val; \
 }
+
 __i915_read(8, b)
 __i915_read(16, w)
 __i915_read(32, l)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 188b497..c6062c3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -396,7 +396,6 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
 			I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
 				   I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
 		}
-
 	}
 
 	gen6_set_rps(dev, new_delay);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e522c70..f6780cf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1828,7 +1828,7 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 	u32 blt_ecoskpd;
 
 	/* Make sure blitter notifies FBC of writes */
-	__gen6_gt_force_wake_get(dev_priv);
+	gen6_gt_force_wake_get(dev_priv);
 	blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
 	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
 		GEN6_BLITTER_LOCK_SHIFT;
@@ -1839,7 +1839,7 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 			 GEN6_BLITTER_LOCK_SHIFT);
 	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
 	POSTING_READ(GEN6_BLITTER_ECOSKPD);
-	__gen6_gt_force_wake_put(dev_priv);
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
@@ -6860,7 +6860,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	 * userspace...
 	 */
 	I915_WRITE(GEN6_RC_STATE, 0);
-	__gen6_gt_force_wake_get(dev_priv);
+	gen6_gt_force_wake_get(dev_priv);
 
 	/* disable the counters and set deterministic thresholds */
 	I915_WRITE(GEN6_RC_CONTROL, 0);
@@ -6958,7 +6958,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	/* enable all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
-	__gen6_gt_force_wake_put(dev_priv);
+	gen6_gt_force_wake_put(dev_priv);
 }
 
 void intel_enable_clock_gating(struct drm_device *dev)
-- 
1.7.3.4

[PATCH 3/5] drm/i915: forcewake struct mutex locking fixes

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    5 +++++
 drivers/gpu/drm/i915/intel_display.c |    9 +++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3cb0722..30bfb2a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -852,6 +852,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	int ret;
 
 	if (IS_GEN5(dev)) {
 		u16 rgvswctl = I915_READ16(MEMSWCTL);
@@ -873,6 +874,9 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 		int max_freq;
 
 		/* RPSTAT1 is in the GT power well */
+		ret = mutex_lock_interruptible(&dev->struct_mutex);
+		if (ret)
+			return ret;
 		gen6_gt_force_wake_get(dev_priv);
 
 		rpstat = I915_READ(GEN6_RPSTAT1);
@@ -919,6 +923,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused)
 			   max_freq * 50);
 
 		gen6_gt_force_wake_put(dev_priv);
+		mutex_unlock(&dev->struct_mutex);
 	} else {
 		seq_printf(m, "no P-state info available\n");
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f6780cf..973aa46 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2873,7 +2873,11 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		ironlake_pch_enable(crtc);
 
 	intel_crtc_load_lut(crtc);
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
+
 	intel_crtc_update_cursor(crtc, true);
 }
 
@@ -2969,8 +2973,11 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc->active = false;
 	intel_update_watermarks(dev);
+
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
 	intel_clear_scanline_wait(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void ironlake_crtc_dpms(struct drm_crtc *crtc, int mode)
@@ -6860,6 +6867,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	 * userspace...
 	 */
 	I915_WRITE(GEN6_RC_STATE, 0);
+	mutex_lock(&dev_priv->dev->struct_mutex);
 	gen6_gt_force_wake_get(dev_priv);
 
 	/* disable the counters and set deterministic thresholds */
@@ -6959,6 +6967,7 @@ 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);
 }
 
 void intel_enable_clock_gating(struct drm_device *dev)
-- 
1.7.3.4

[PATCH 4/5] drm/i915: move gen6 rps handling to workqueue

[Ben Widawsky]

The render P-state handling code requires reading from a GT register.
This means that FORCEWAKE must be written to, a resource which is shared
and should be protected by struct_mutex.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c      |    1 +
 drivers/gpu/drm/i915/i915_drv.h      |    4 +++
 drivers/gpu/drm/i915/i915_irq.c      |   49 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_reg.h      |    5 +++-
 drivers/gpu/drm/i915/intel_display.c |    8 +++++
 5 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7273037..855355e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2025,6 +2025,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->error_lock);
+	spin_lock_init(&dev_priv->rps_lock);
 
 	if (IS_MOBILE(dev) || !IS_GEN2(dev))
 		dev_priv->num_pipe = 2;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 140a381..7b35f45 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -676,6 +676,10 @@ typedef struct drm_i915_private {
 
 	bool mchbar_need_disable;
 
+	struct work_struct rps_work;
+	spinlock_t rps_lock;
+	u32 pm_iir;
+
 	u8 cur_delay;
 	u8 min_delay;
 	u8 max_delay;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c6062c3..5dfef48 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -367,22 +367,30 @@ static void notify_ring(struct drm_device *dev,
 		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
 }
 
-static void gen6_pm_irq_handler(struct drm_device *dev)
+static void gen6_pm_rps_work(struct work_struct *work)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
+						    rps_work);
 	u8 new_delay = dev_priv->cur_delay;
-	u32 pm_iir;
+	u32 pm_iir, pm_imr;
+
+	spin_lock_irq(&dev_priv->rps_lock);
+	pm_iir = dev_priv->pm_iir;
+	dev_priv->pm_iir = 0;
+	pm_imr = I915_READ(GEN6_PMIMR);
+	spin_unlock_irq(&dev_priv->rps_lock);
 
-	pm_iir = I915_READ(GEN6_PMIIR);
 	if (!pm_iir)
 		return;
 
+	mutex_lock(&dev_priv->dev->struct_mutex);
 	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
 		if (dev_priv->cur_delay != dev_priv->max_delay)
 			new_delay = dev_priv->cur_delay + 1;
 		if (new_delay > dev_priv->max_delay)
 			new_delay = dev_priv->max_delay;
 	} else if (pm_iir & (GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT)) {
+		gen6_gt_force_wake_get(dev_priv);
 		if (dev_priv->cur_delay != dev_priv->min_delay)
 			new_delay = dev_priv->cur_delay - 1;
 		if (new_delay < dev_priv->min_delay) {
@@ -396,12 +404,19 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
 			I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
 				   I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
 		}
+		gen6_gt_force_wake_put(dev_priv);
 	}
 
-	gen6_set_rps(dev, new_delay);
+	gen6_set_rps(dev_priv->dev, new_delay);
 	dev_priv->cur_delay = new_delay;
 
-	I915_WRITE(GEN6_PMIIR, pm_iir);
+	/*
+	 * rps_lock not held here because clearing is non-destructive. There is
+	 * an *extremely* unlikely race with gen6_rps_enable() that is prevented
+	 * by holding struct_mutex for the duration of the write.
+	 */
+	I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir);
+	mutex_unlock(&dev_priv->dev->struct_mutex);
 }
 
 static void pch_irq_handler(struct drm_device *dev)
@@ -525,13 +540,30 @@ static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
 		i915_handle_rps_change(dev);
 	}
 
-	if (IS_GEN6(dev))
-		gen6_pm_irq_handler(dev);
+	if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS) {
+		/*
+		 * IIR bits should never already be set because IMR should
+		 * prevent an interrupt from being shown in IIR. The warning
+		 * displays a case where we've unsafely cleared
+		 * dev_priv->pm_iir. Although missing an interrupt of the same
+		 * type is not a problem, it displays a problem in the logic.
+		 *
+		 * The mask bit in IMR is cleared by rps_work.
+		 */
+		unsigned long flags;
+		spin_lock_irqsave(&dev_priv->rps_lock, flags);
+		WARN(dev_priv->pm_iir & pm_iir, "Missed a PM interrupt\n");
+		I915_WRITE(GEN6_PMIMR, pm_iir);
+		dev_priv->pm_iir |= pm_iir;
+		spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
+		queue_work(dev_priv->wq, &dev_priv->rps_work);
+	}
 
 	/* should clear PCH hotplug event before clear CPU irq */
 	I915_WRITE(SDEIIR, pch_iir);
 	I915_WRITE(GTIIR, gt_iir);
 	I915_WRITE(DEIIR, de_iir);
+	I915_WRITE(GEN6_PMIIR, pm_iir);
 
 done:
 	I915_WRITE(DEIER, de_ier);
@@ -1658,6 +1690,7 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
 
 	INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
 	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
+	INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
 
 	if (HAS_PCH_SPLIT(dev)) {
 		ironlake_irq_preinstall(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f39ac3a..289adaa 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3386,7 +3386,7 @@
 #define GEN6_PMINTRMSK				0xA168
 
 #define GEN6_PMISR				0x44020
-#define GEN6_PMIMR				0x44024
+#define GEN6_PMIMR				0x44024 /* rps_lock */
 #define GEN6_PMIIR				0x44028
 #define GEN6_PMIER				0x4402C
 #define  GEN6_PM_MBOX_EVENT			(1<<25)
@@ -3396,6 +3396,9 @@
 #define  GEN6_PM_RP_DOWN_THRESHOLD		(1<<4)
 #define  GEN6_PM_RP_UP_EI_EXPIRED		(1<<2)
 #define  GEN6_PM_RP_DOWN_EI_EXPIRED		(1<<1)
+#define  GEN6_PM_DEFERRED_EVENTS		(GEN6_PM_RP_UP_THRESHOLD | \
+						 GEN6_PM_RP_DOWN_THRESHOLD | \
+						 GEN6_PM_RP_DOWN_TIMEOUT)
 
 #define GEN6_PCODE_MAILBOX			0x138124
 #define   GEN6_PCODE_READY			(1<<31)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 973aa46..bf8006d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6763,6 +6763,11 @@ void gen6_disable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
 	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
 	I915_WRITE(GEN6_PMIER, 0);
+
+	spin_lock_irq(&dev_priv->rps_lock);
+	dev_priv->pm_iir = 0;
+	spin_unlock_irq(&dev_priv->rps_lock);
+
 	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
 }
 
@@ -6962,7 +6967,10 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 		   GEN6_PM_RP_DOWN_THRESHOLD |
 		   GEN6_PM_RP_UP_EI_EXPIRED |
 		   GEN6_PM_RP_DOWN_EI_EXPIRED);
+	spin_lock_irq(&dev_priv->rps_lock);
+	WARN_ON(dev_priv->pm_iir != 0);
 	I915_WRITE(GEN6_PMIMR, 0);
+	spin_unlock_irq(&dev_priv->rps_lock);
 	/* enable all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
-- 
1.7.3.4

[PATCH 5/5] drm/i915: debugfs interface for forcewake reference count

[Ben Widawsky]

forcewake is controlled by the open and close of the debugfs file. This
assures that buggy applications cannot cause the GT to stay on forever.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   80 +++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 30bfb2a..c8f40ce 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1191,6 +1191,18 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	seq_printf(m, "forcewake count = %d\n",
+		   atomic_read(&dev_priv->forcewake_count));
+
+	return 0;
+}
+
 static int
 i915_wedged_open(struct inode *inode,
 		 struct file *filp)
@@ -1293,6 +1305,67 @@ static int i915_wedged_create(struct dentry *root, struct drm_minor *minor)
 	return drm_add_fake_info_node(minor, ent, &i915_wedged_fops);
 }
 
+static int i915_forcewake_open(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
+
+	if (!IS_GEN6(dev))
+		return 0;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+	gen6_gt_force_wake_get(dev_priv);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+int i915_forcewake_release(struct inode *inode, struct file *file)
+{
+	struct drm_device *dev = inode->i_private;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!IS_GEN6(dev))
+		return 0;
+
+	/*
+	 * It's bad that we can potentially hang userspace if struct_mutex gets
+	 * forever stuck.  However, if we cannot acquire this lock it means that
+	 * almost certainly the driver has hung, is not unload-able. Therefore
+	 * hanging here is probably a minor inconvenience not to be seen my
+	 * almost every user.
+	 */
+	mutex_lock(&dev->struct_mutex);
+	gen6_gt_force_wake_put(dev_priv);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
+static const struct file_operations i915_forcewake_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_forcewake_open,
+	.release = i915_forcewake_release,
+};
+
+static int i915_forcewake_create(struct dentry *root, struct drm_minor *minor)
+{
+	struct drm_device *dev = minor->dev;
+	struct dentry *ent;
+
+	ent = debugfs_create_file("i915_forcewake_user",
+				  S_IRWXU,
+				  root, dev,
+				  &i915_forcewake_fops);
+	if (IS_ERR(ent))
+		return PTR_ERR(ent);
+
+	return 0;
+}
+
 static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
@@ -1329,6 +1402,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_sr_status", i915_sr_status, 0},
 	{"i915_opregion", i915_opregion, 0},
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
+	{"i915_gen6_forcewake_count", i915_gen6_forcewake_count_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
@@ -1340,6 +1414,10 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	ret = i915_forcewake_create(minor->debugfs_root, minor);
+	if (ret)
+		return ret;
+
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
@@ -1349,6 +1427,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 {
 	drm_debugfs_remove_files(i915_debugfs_list,
 				 I915_DEBUGFS_ENTRIES, minor);
+	drm_debugfs_remove_files((struct drm_info_list *) &i915_forcewake_fops,
+				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_wedged_fops,
 				 1, minor);
 }
-- 
1.7.3.4

Re: [PATCH 5/5] drm/i915: debugfs interface for forcewake reference count

[Chris Wilson]

On Mon, 25 Apr 2011 11:25:56 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> forcewake is controlled by the open and close of the debugfs file. This
> assures that buggy applications cannot cause the GT to stay on forever.

I pushed them out to drm-intel-staging for wider testing. I'm not sure
that this last past merits being applied to the stable kernel (as this
does little to prevent root from breaking his machine anyway and so will
probably just suggest applying to -next instead)  but the earlier patches
lead up to the important bug fixes where we were not holding the required
mutex.

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: debugfs interface for forcewake reference count

[Ben Widawsky]

On Mon, Apr 25, 2011 at 08:28:50PM +0100, Chris Wilson wrote:
> On Mon, 25 Apr 2011 11:25:56 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > forcewake is controlled by the open and close of the debugfs file. This
> > assures that buggy applications cannot cause the GT to stay on forever.
> 
> I pushed them out to drm-intel-staging for wider testing. I'm not sure
> that this last past merits being applied to the stable kernel (as this
> does little to prevent root from breaking his machine anyway and so will
> probably just suggest applying to -next instead)  but the earlier patches
> lead up to the important bug fixes where we were not holding the required
> mutex.
> 
> Thanks,
> -Chris

I hope you caught the changes in the updates, there was one potentially
important one with synchronizing gen6_[en|dis]able_rps().

Also, I've pushed my updated tools (which is just read and write for
now) to here:
git://anongit.freedesktop.org/~bwidawsk/intel-gpu-tools
branch: good_reg

When/if you do the announce for staging, I will reply there as well with
this repo, and push it once the kernel changes stick.

Thanks.
Ben

Re: [PATCH 5/5] drm/i915: debugfs interface for forcewake reference count

[Chris Wilson]

On Mon, 25 Apr 2011 14:46:14 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, Apr 25, 2011 at 08:28:50PM +0100, Chris Wilson wrote:
> When/if you do the announce for staging, I will reply there as well with
> this repo, and push it once the kernel changes stick.

-staging is a constant flux of patches heading for -fixes waiting for
review and testing. So the closest it gets to an announcement is the list
of pending patches for -fixes. So beware.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre