[PATCH 0/9] rps locking fixes

[PATCH 0/9] rps locking fixes

[Daniel Vetter]

Hi all

While reviewing Chris' simplified mark_busy/idle code I've noticed that we
seriously lack some locking. This series here fixes the first part around the
gpu power handling, the pll up/downclocking might still be rather broken :(

Since this clearly fixes some issues with the code, also try another stab at
enabling rc6 on ilk by default.

Review, flames and comments highly welcome!

Cheers, Daniel

Daniel Vetter (9):
  drm/i915: ensure rps state is properly lock-protected
  drm/i915: properly guard ilk ips state
  drm/i915: fixup up debugfs rps state handling
  drm/i915: move all rps state into dev_priv->rps
  drm/i915: kill dev_priv->mchdev_lock
  drm/i915: DE_PCU_EVENT irq is ilk-only
  drm/i915: fix up ilk drps/ips locking
  drm/ips: move drps/ips/ilk related variables into dev_priv->ips
  drm/i915: enable rc6 on ilk again

 drivers/gpu/drm/i915/i915_debugfs.c  |   38 ++++-
 drivers/gpu/drm/i915/i915_dma.c      |    2 +-
 drivers/gpu/drm/i915/i915_drv.h      |   55 +++++---
 drivers/gpu/drm/i915/i915_irq.c      |   72 ++++++----
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_pm.c      |  259 ++++++++++++++++++----------------
 6 files changed, 251 insertions(+), 177 deletions(-)

-- 
1.7.10.4

[PATCH 1/9] drm/i915: ensure rps state is properly lock-protected

[Daniel Vetter]

Pure paranoia-induced patch as part of a large work to fix up the
locking around intel_mark_busy/idle. At least for the gen6+ rps state,
locking seems to be sane and the hw/sw state is protected by
dev->struct_mutex.

The only thing missing is a paranoid WARN_ON in sanitize_pm.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 85d1b1c..b6e8fbf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3745,6 +3745,8 @@ static void gen6_sanitize_pm(struct drm_device *dev)
 
 	gen6_gt_force_wake_get(dev_priv);
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	old = limits = I915_READ(GEN6_RP_INTERRUPT_LIMITS);
 	/* Make sure we continue to get interrupts
 	 * until we hit the minimum or maximum frequencies.
-- 
1.7.10.4

[PATCH 2/9] drm/i915: properly guard ilk ips state

[Daniel Vetter]

The update_gfx_val function called from mark_busy wasn't taking the
mchdev_lock, as it should have. Also sprinkle a few spinlock asserts
over the code to document things better.

Things are still rather confusing, especially since a few variables
in dev_priv are used by both the gen6+ rps code and the ilk ips code.
But protected by totally different locks. Follow-on patches will clean
that up.

v2: Don't add a deadlock ... hence split up update_gfx_val into a
wrapper that grabs the lock and an internal __ variant for callsites
within intel_pm.c that already have taken the lock.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c |   50 ++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b6e8fbf..21a0088 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2693,6 +2693,21 @@ static const struct cparams {
 	{ 0, 800, 231, 23784 },
 };
 
+/**
+ * Lock protecting IPS related data structures
+ *   - i915_mch_dev
+ *   - dev_priv->max_delay
+ *   - dev_priv->min_delay
+ *   - dev_priv->fmax
+ *   - dev_priv->gpu_busy
+ *   - dev_priv->gfx_power
+ */
+static DEFINE_SPINLOCK(mchdev_lock);
+
+/* Global for IPS driver to get at the current i915 device. Protected by
+ * mchdev_lock. */
+static struct drm_i915_private *i915_mch_dev;
+
 unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 {
 	u64 total_count, diff, ret;
@@ -2700,6 +2715,8 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	unsigned long now = jiffies_to_msecs(jiffies), diff1;
 	int i;
 
+	assert_spin_locked(&mchdev_lock);
+
 	diff1 = now - dev_priv->last_time1;
 
 	/* Prevent division-by-zero if we are asking too fast.
@@ -2901,15 +2918,14 @@ static u16 pvid_to_extvid(struct drm_i915_private *dev_priv, u8 pxvid)
 		return v_table[pxvid].vd;
 }
 
-void i915_update_gfx_val(struct drm_i915_private *dev_priv)
+void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 {
 	struct timespec now, diff1;
 	u64 diff;
 	unsigned long diffms;
 	u32 count;
 
-	if (dev_priv->info->gen != 5)
-		return;
+	assert_spin_locked(&mchdev_lock);
 
 	getrawmonotonic(&now);
 	diff1 = timespec_sub(now, dev_priv->last_time2);
@@ -2937,11 +2953,25 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
 	dev_priv->gfx_power = diff;
 }
 
+void i915_update_gfx_val(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->info->gen != 5)
+		return;
+
+	spin_lock(&mchdev_lock);
+
+	__i915_update_gfx_val(dev_priv);
+
+	spin_unlock(&mchdev_lock);
+}
+
 unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 {
 	unsigned long t, corr, state1, corr2, state2;
 	u32 pxvid, ext_v;
 
+	assert_spin_locked(&mchdev_lock);
+
 	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
 	pxvid = (pxvid >> 24) & 0x7f;
 	ext_v = pvid_to_extvid(dev_priv, pxvid);
@@ -2967,23 +2997,11 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 	state2 = (corr2 * state1) / 10000;
 	state2 /= 100; /* convert to mW */
 
-	i915_update_gfx_val(dev_priv);
+	__i915_update_gfx_val(dev_priv);
 
 	return dev_priv->gfx_power + state2;
 }
 
-/* Global for IPS driver to get at the current i915 device */
-static struct drm_i915_private *i915_mch_dev;
-/*
- * Lock protecting IPS related data structures
- *   - i915_mch_dev
- *   - dev_priv->max_delay
- *   - dev_priv->min_delay
- *   - dev_priv->fmax
- *   - dev_priv->gpu_busy
- */
-static DEFINE_SPINLOCK(mchdev_lock);
-
 /**
  * i915_read_mch_val - return value for IPS use
  *
-- 
1.7.10.4

[PATCH 3/9] drm/i915: fixup up debugfs rps state handling

[Daniel Vetter]

- Take the dev->struct_mutex around access the corresponding state
  (and adjusting the rps hw state).
- Add an assert to gen6_set_rps to ensure we don't forget about this
  in the future.
- Don't set up the min/max_freq files if it doesn't apply to the hw.
  And do the same for the gen6+ cache sharing file while at it.

v2: Move the gen6+ checks into the read/write callbacks. Thanks to the
awesome drm midlayer we can't check this when registering the debugfs
files, because the driver is not yet fully set up, specifically the
->load callback hasn't run yet.

Oh how I despise & loathe this disaster ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c     |    2 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1312b79..2499610 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1710,8 +1710,13 @@ i915_max_freq_read(struct file *filp,
 	char buf[80];
 	int len;
 
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
+	mutex_lock(&dev->struct_mutex);
 	len = snprintf(buf, sizeof(buf),
 		       "max freq: %d\n", dev_priv->max_delay * 50);
+	mutex_unlock(&dev->struct_mutex);
 
 	if (len > sizeof(buf))
 		len = sizeof(buf);
@@ -1730,6 +1735,9 @@ i915_max_freq_write(struct file *filp,
 	char buf[20];
 	int val = 1;
 
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
 	if (cnt > 0) {
 		if (cnt > sizeof(buf) - 1)
 			return -EINVAL;
@@ -1743,12 +1751,14 @@ i915_max_freq_write(struct file *filp,
 
 	DRM_DEBUG_DRIVER("Manually setting max freq to %d\n", val);
 
+	mutex_lock(&dev->struct_mutex);
 	/*
 	 * Turbo will still be enabled, but won't go above the set value.
 	 */
 	dev_priv->max_delay = val / 50;
 
 	gen6_set_rps(dev, val / 50);
+	mutex_unlock(&dev->struct_mutex);
 
 	return cnt;
 }
@@ -1770,8 +1780,13 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
 	char buf[80];
 	int len;
 
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
+	mutex_lock(&dev->struct_mutex);
 	len = snprintf(buf, sizeof(buf),
 		       "min freq: %d\n", dev_priv->min_delay * 50);
+	mutex_unlock(&dev->struct_mutex);
 
 	if (len > sizeof(buf))
 		len = sizeof(buf);
@@ -1788,6 +1803,9 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	char buf[20];
 	int val = 1;
 
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
 	if (cnt > 0) {
 		if (cnt > sizeof(buf) - 1)
 			return -EINVAL;
@@ -1801,12 +1819,14 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
 
 	DRM_DEBUG_DRIVER("Manually setting min freq to %d\n", val);
 
+	mutex_lock(&dev->struct_mutex);
 	/*
 	 * Turbo will still be enabled, but won't go below the set value.
 	 */
 	dev_priv->min_delay = val / 50;
 
 	gen6_set_rps(dev, val / 50);
+	mutex_unlock(&dev->struct_mutex);
 
 	return cnt;
 }
@@ -1831,6 +1851,9 @@ i915_cache_sharing_read(struct file *filp,
 	u32 snpcr;
 	int len;
 
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
 	mutex_lock(&dev_priv->dev->struct_mutex);
 	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 	mutex_unlock(&dev_priv->dev->struct_mutex);
@@ -1857,6 +1880,9 @@ i915_cache_sharing_write(struct file *filp,
 	u32 snpcr;
 	int val = 1;
 
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
 	if (cnt > 0) {
 		if (cnt > sizeof(buf) - 1)
 			return -EINVAL;
@@ -2061,6 +2087,7 @@ int i915_debugfs_init(struct drm_minor *minor)
 				  &i915_cache_sharing_fops);
 	if (ret)
 		return ret;
+
 	ret = i915_debugfs_create(minor->debugfs_root, minor,
 				  "i915_ring_stop",
 				  &i915_ring_stop_fops);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 21a0088..d5af41b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2272,6 +2272,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 limits;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	limits = 0;
 	if (val >= dev_priv->max_delay)
 		val = dev_priv->max_delay;
-- 
1.7.10.4

[PATCH 4/9] drm/i915: move all rps state into dev_priv->rps

[Daniel Vetter]

This way it's easier so see what belongs together, and what is used
by the ilk ips code. Also add some comments that explain the locking.

v2: Missed one place that the dev_priv->ips change caught ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |   11 +++----
 drivers/gpu/drm/i915/i915_dma.c      |    2 +-
 drivers/gpu/drm/i915/i915_drv.h      |   18 ++++++++++--
 drivers/gpu/drm/i915/i915_irq.c      |   32 ++++++++++-----------
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_pm.c      |   52 +++++++++++++++++-----------------
 6 files changed, 65 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2499610..05087fa 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1287,7 +1287,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
 
 	seq_printf(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\n");
 
-	for (gpu_freq = dev_priv->min_delay; gpu_freq <= dev_priv->max_delay;
+	for (gpu_freq = dev_priv->rps.min_delay;
+	     gpu_freq <= dev_priv->rps.max_delay;
 	     gpu_freq++) {
 		I915_WRITE(GEN6_PCODE_DATA, gpu_freq);
 		I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY |
@@ -1715,7 +1716,7 @@ i915_max_freq_read(struct file *filp,
 
 	mutex_lock(&dev->struct_mutex);
 	len = snprintf(buf, sizeof(buf),
-		       "max freq: %d\n", dev_priv->max_delay * 50);
+		       "max freq: %d\n", dev_priv->rps.max_delay * 50);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (len > sizeof(buf))
@@ -1755,7 +1756,7 @@ i915_max_freq_write(struct file *filp,
 	/*
 	 * Turbo will still be enabled, but won't go above the set value.
 	 */
-	dev_priv->max_delay = val / 50;
+	dev_priv->rps.max_delay = val / 50;
 
 	gen6_set_rps(dev, val / 50);
 	mutex_unlock(&dev->struct_mutex);
@@ -1785,7 +1786,7 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
 
 	mutex_lock(&dev->struct_mutex);
 	len = snprintf(buf, sizeof(buf),
-		       "min freq: %d\n", dev_priv->min_delay * 50);
+		       "min freq: %d\n", dev_priv->rps.min_delay * 50);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (len > sizeof(buf))
@@ -1823,7 +1824,7 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	/*
 	 * Turbo will still be enabled, but won't go below the set value.
 	 */
-	dev_priv->min_delay = val / 50;
+	dev_priv->rps.min_delay = val / 50;
 
 	gen6_set_rps(dev, val / 50);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e20f11..5f63638 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1586,7 +1586,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);
+	spin_lock_init(&dev_priv->rps.lock);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 		dev_priv->num_pipe = 3;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f176589..2496d60 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -793,9 +793,21 @@ typedef struct drm_i915_private {
 
 	bool mchbar_need_disable;
 
-	struct work_struct rps_work;
-	spinlock_t rps_lock;
-	u32 pm_iir;
+	/* gen6+ rps state */
+	struct {
+		struct work_struct work;
+		u32 pm_iir;
+		/* lock - irqsave spinlock that protectects the work_struct and
+		 * pm_iir. */
+		spinlock_t lock;
+
+		/* The below variables an all the rps hw state are protected by
+		 * dev->struct mutext. */
+		u8 cur_delay;
+		u8 min_delay;
+		u8 max_delay;
+	} rps;
+
 
 	u8 cur_delay;
 	u8 min_delay;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 41ed41d..6e3f43c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -349,16 +349,16 @@ static void notify_ring(struct drm_device *dev,
 static void gen6_pm_rps_work(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
-						    rps_work);
+						    rps.work);
 	u32 pm_iir, pm_imr;
 	u8 new_delay;
 
-	spin_lock_irq(&dev_priv->rps_lock);
-	pm_iir = dev_priv->pm_iir;
-	dev_priv->pm_iir = 0;
+	spin_lock_irq(&dev_priv->rps.lock);
+	pm_iir = dev_priv->rps.pm_iir;
+	dev_priv->rps.pm_iir = 0;
 	pm_imr = I915_READ(GEN6_PMIMR);
 	I915_WRITE(GEN6_PMIMR, 0);
-	spin_unlock_irq(&dev_priv->rps_lock);
+	spin_unlock_irq(&dev_priv->rps.lock);
 
 	if ((pm_iir & GEN6_PM_DEFERRED_EVENTS) == 0)
 		return;
@@ -366,9 +366,9 @@ static void gen6_pm_rps_work(struct work_struct *work)
 	mutex_lock(&dev_priv->dev->struct_mutex);
 
 	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD)
-		new_delay = dev_priv->cur_delay + 1;
+		new_delay = dev_priv->rps.cur_delay + 1;
 	else
-		new_delay = dev_priv->cur_delay - 1;
+		new_delay = dev_priv->rps.cur_delay - 1;
 
 	gen6_set_rps(dev_priv->dev, new_delay);
 
@@ -488,20 +488,20 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
 	 * 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
+	 * dev_priv->rps.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.
+	 * The mask bit in IMR is cleared by dev_priv->rps.work.
 	 */
 
-	spin_lock_irqsave(&dev_priv->rps_lock, flags);
-	WARN(dev_priv->pm_iir & pm_iir, "Missed a PM interrupt\n");
-	dev_priv->pm_iir |= pm_iir;
-	I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir);
+	spin_lock_irqsave(&dev_priv->rps.lock, flags);
+	WARN(dev_priv->rps.pm_iir & pm_iir, "Missed a PM interrupt\n");
+	dev_priv->rps.pm_iir |= pm_iir;
+	I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
 	POSTING_READ(GEN6_PMIMR);
-	spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
+	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
 
-	queue_work(dev_priv->wq, &dev_priv->rps_work);
+	queue_work(dev_priv->wq, &dev_priv->rps.work);
 }
 
 static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
@@ -2649,7 +2649,7 @@ void intel_irq_init(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);
+	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
 	INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
 
 	dev->driver->get_vblank_counter = i915_get_vblank_counter;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index be45b92..00a90e1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7220,7 +7220,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	 * enqueue unpin/hotplug work. */
 	drm_irq_uninstall(dev);
 	cancel_work_sync(&dev_priv->hotplug_work);
-	cancel_work_sync(&dev_priv->rps_work);
+	cancel_work_sync(&dev_priv->rps.work);
 
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d5af41b..6c9925d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2275,17 +2275,17 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	limits = 0;
-	if (val >= dev_priv->max_delay)
-		val = dev_priv->max_delay;
+	if (val >= dev_priv->rps.max_delay)
+		val = dev_priv->rps.max_delay;
 	else
-		limits |= dev_priv->max_delay << 24;
+		limits |= dev_priv->rps.max_delay << 24;
 
-	if (val <= dev_priv->min_delay)
-		val = dev_priv->min_delay;
+	if (val <= dev_priv->rps.min_delay)
+		val = dev_priv->rps.min_delay;
 	else
-		limits |= dev_priv->min_delay << 16;
+		limits |= dev_priv->rps.min_delay << 16;
 
-	if (val == dev_priv->cur_delay)
+	if (val == dev_priv->rps.cur_delay)
 		return;
 
 	I915_WRITE(GEN6_RPNSWREQ,
@@ -2298,7 +2298,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	 */
 	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
 
-	dev_priv->cur_delay = val;
+	dev_priv->rps.cur_delay = val;
 }
 
 static void gen6_disable_rps(struct drm_device *dev)
@@ -2314,9 +2314,9 @@ static void gen6_disable_rps(struct drm_device *dev)
 	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
 	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
 
-	spin_lock_irq(&dev_priv->rps_lock);
-	dev_priv->pm_iir = 0;
-	spin_unlock_irq(&dev_priv->rps_lock);
+	spin_lock_irq(&dev_priv->rps.lock);
+	dev_priv->rps.pm_iir = 0;
+	spin_unlock_irq(&dev_priv->rps.lock);
 
 	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
 }
@@ -2385,9 +2385,9 @@ static void gen6_enable_rps(struct drm_device *dev)
 	gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 
 	/* In units of 100MHz */
-	dev_priv->max_delay = rp_state_cap & 0xff;
-	dev_priv->min_delay = (rp_state_cap & 0xff0000) >> 16;
-	dev_priv->cur_delay = 0;
+	dev_priv->rps.max_delay = rp_state_cap & 0xff;
+	dev_priv->rps.min_delay = (rp_state_cap & 0xff0000) >> 16;
+	dev_priv->rps.cur_delay = 0;
 
 	/* disable the counters and set deterministic thresholds */
 	I915_WRITE(GEN6_RC_CONTROL, 0);
@@ -2440,8 +2440,8 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1000000);
 	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
-		   dev_priv->max_delay << 24 |
-		   dev_priv->min_delay << 16);
+		   dev_priv->rps.max_delay << 24 |
+		   dev_priv->rps.min_delay << 16);
 
 	if (IS_HASWELL(dev)) {
 		I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
@@ -2486,7 +2486,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 		     500))
 		DRM_ERROR("timeout waiting for pcode mailbox to finish\n");
 	if (pcu_mbox & (1<<31)) { /* OC supported */
-		dev_priv->max_delay = pcu_mbox & 0xff;
+		dev_priv->rps.max_delay = pcu_mbox & 0xff;
 		DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
 	}
 
@@ -2494,10 +2494,10 @@ static void gen6_enable_rps(struct drm_device *dev)
 
 	/* requires MSI enabled */
 	I915_WRITE(GEN6_PMIER, GEN6_PM_DEFERRED_EVENTS);
-	spin_lock_irq(&dev_priv->rps_lock);
-	WARN_ON(dev_priv->pm_iir != 0);
+	spin_lock_irq(&dev_priv->rps.lock);
+	WARN_ON(dev_priv->rps.pm_iir != 0);
 	I915_WRITE(GEN6_PMIMR, 0);
-	spin_unlock_irq(&dev_priv->rps_lock);
+	spin_unlock_irq(&dev_priv->rps.lock);
 	/* enable all PM interrupts */
 	I915_WRITE(GEN6_PMINTRMSK, 0);
 
@@ -2529,9 +2529,9 @@ static void gen6_update_ring_freq(struct drm_device *dev)
 	 * to use for memory access.  We do this by specifying the IA frequency
 	 * the PCU should use as a reference to determine the ring frequency.
 	 */
-	for (gpu_freq = dev_priv->max_delay; gpu_freq >= dev_priv->min_delay;
+	for (gpu_freq = dev_priv->rps.max_delay; gpu_freq >= dev_priv->rps.min_delay;
 	     gpu_freq--) {
-		int diff = dev_priv->max_delay - gpu_freq;
+		int diff = dev_priv->rps.max_delay - gpu_freq;
 
 		/*
 		 * For GPU frequencies less than 750MHz, just use the lowest
@@ -2974,7 +2974,7 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 
 	assert_spin_locked(&mchdev_lock);
 
-	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
+	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->rps.cur_delay * 4));
 	pxvid = (pxvid >> 24) & 0x7f;
 	ext_v = pvid_to_extvid(dev_priv, pxvid);
 
@@ -3772,11 +3772,11 @@ static void gen6_sanitize_pm(struct drm_device *dev)
 	 * until we hit the minimum or maximum frequencies.
 	 */
 	limits &= ~(0x3f << 16 | 0x3f << 24);
-	delay = dev_priv->cur_delay;
+	delay = dev_priv->rps.cur_delay;
 	if (delay < dev_priv->max_delay)
 		limits |= (dev_priv->max_delay & 0x3f) << 24;
-	if (delay > dev_priv->min_delay)
-		limits |= (dev_priv->min_delay & 0x3f) << 16;
+	if (delay > dev_priv->rps.min_delay)
+		limits |= (dev_priv->rps.min_delay & 0x3f) << 16;
 
 	if (old != limits) {
 		/* Note that the known failure case is to read back 0. */
-- 
1.7.10.4

[PATCH 5/9] drm/i915: kill dev_priv->mchdev_lock

[Daniel Vetter]

It's only ever a pointer to the global mchdev_lock, and we don't use
it at all.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 -
 drivers/gpu/drm/i915/intel_pm.c |    1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2496d60..b104b7f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -824,7 +824,6 @@ typedef struct drm_i915_private {
 	int c_m;
 	int r_t;
 	u8 corr;
-	spinlock_t *mchdev_lock;
 
 	enum no_fbc_reason no_fbc_reason;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6c9925d..b5a08b0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3168,7 +3168,6 @@ void intel_gpu_ips_init(struct drm_i915_private *dev_priv)
 {
 	spin_lock(&mchdev_lock);
 	i915_mch_dev = dev_priv;
-	dev_priv->mchdev_lock = &mchdev_lock;
 	spin_unlock(&mchdev_lock);
 
 	ips_ping_for_i915_load();
-- 
1.7.10.4

[PATCH 6/9] drm/i915: DE_PCU_EVENT irq is ilk-only

[Daniel Vetter]

Like all the other drps/ips stuff. Hence add the corresponding check,
give the function a preciser prefix and move the single reg clearing into
the rps handling function, too.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6e3f43c..f46fa6c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -296,12 +296,14 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	drm_helper_hpd_irq_event(dev);
 }
 
-static void i915_handle_rps_change(struct drm_device *dev)
+static void ironlake_handle_rps_change(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 busy_up, busy_down, max_avg, min_avg;
 	u8 new_delay = dev_priv->cur_delay;
 
+	I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
+
 	I915_WRITE16(MEMINTRSTS, MEMINT_EVAL_CHG);
 	busy_up = I915_READ(RCPREVBSYTUPAVG);
 	busy_down = I915_READ(RCPREVBSYTDNAVG);
@@ -794,10 +796,8 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
 			ibx_irq_handler(dev, pch_iir);
 	}
 
-	if (de_iir & DE_PCU_EVENT) {
-		I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
-		i915_handle_rps_change(dev);
-	}
+	if (IS_GEN5(dev) &&  de_iir & DE_PCU_EVENT)
+		ironlake_handle_rps_change(dev);
 
 	if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS)
 		gen6_queue_rps_work(dev_priv, pm_iir);
-- 
1.7.10.4

[PATCH 7/9] drm/i915: fix up ilk drps/ips locking

[Daniel Vetter]

We change the drps/ips sw/hw state from different callers: Our own irq
handler, the external intel-ips module and from process context. Most
of these callers don't take any lock at all.

Protect everything by making the mchdev_lock irqsave and grabbing it in
all relevant callsites. Note that we have to convert a few sleeps in the
drps enable/disable code to delays, but alas, I'm not volunteering to
restructure the code around a few work items.

For paranoia add a spin_locked assert to ironlake_set_drps, too.

v2: Move one access inside the lock protection. Caught by the
dev_priv->ips mass-rename ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c |   12 +++++-
 drivers/gpu/drm/i915/intel_pm.c |   83 ++++++++++++++++++++++-----------------
 2 files changed, 58 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f46fa6c..75c631d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -296,14 +296,22 @@ static void i915_hotplug_work_func(struct work_struct *work)
 	drm_helper_hpd_irq_event(dev);
 }
 
+/* defined intel_pm.c */
+extern spinlock_t mchdev_lock;
+
 static void ironlake_handle_rps_change(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 busy_up, busy_down, max_avg, min_avg;
-	u8 new_delay = dev_priv->cur_delay;
+	u8 new_delay;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mchdev_lock, flags);
 
 	I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
 
+	new_delay = dev_priv->cur_delay;
+
 	I915_WRITE16(MEMINTRSTS, MEMINT_EVAL_CHG);
 	busy_up = I915_READ(RCPREVBSYTUPAVG);
 	busy_down = I915_READ(RCPREVBSYTDNAVG);
@@ -326,6 +334,8 @@ static void ironlake_handle_rps_change(struct drm_device *dev)
 	if (ironlake_set_drps(dev, new_delay))
 		dev_priv->cur_delay = new_delay;
 
+	spin_unlock_irqrestore(&mchdev_lock, flags);
+
 	return;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b5a08b0..e582b50 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2160,11 +2160,28 @@ err_unref:
 	return NULL;
 }
 
+/**
+ * Lock protecting IPS related data structures
+ *   - i915_mch_dev
+ *   - dev_priv->max_delay
+ *   - dev_priv->min_delay
+ *   - dev_priv->fmax
+ *   - dev_priv->gpu_busy
+ *   - dev_priv->gfx_power
+ */
+DEFINE_SPINLOCK(mchdev_lock);
+
+/* Global for IPS driver to get at the current i915 device. Protected by
+ * mchdev_lock. */
+static struct drm_i915_private *i915_mch_dev;
+
 bool ironlake_set_drps(struct drm_device *dev, u8 val)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 rgvswctl;
 
+	assert_spin_locked(&mchdev_lock);
+
 	rgvswctl = I915_READ16(MEMSWCTL);
 	if (rgvswctl & MEMCTL_CMD_STS) {
 		DRM_DEBUG("gpu busy, RCS change rejected\n");
@@ -2188,6 +2205,8 @@ static void ironlake_enable_drps(struct drm_device *dev)
 	u32 rgvmodectl = I915_READ(MEMMODECTL);
 	u8 fmax, fmin, fstart, vstart;
 
+	spin_lock_irq(&mchdev_lock);
+
 	/* Enable temp reporting */
 	I915_WRITE16(PMMISC, I915_READ(PMMISC) | MCPPCE_EN);
 	I915_WRITE16(TSC1, I915_READ(TSC1) | TSE);
@@ -2233,9 +2252,9 @@ static void ironlake_enable_drps(struct drm_device *dev)
 	rgvmodectl |= MEMMODE_SWMODE_EN;
 	I915_WRITE(MEMMODECTL, rgvmodectl);
 
-	if (wait_for((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
+	if (wait_for_atomic((I915_READ(MEMSWCTL) & MEMCTL_CMD_STS) == 0, 10))
 		DRM_ERROR("stuck trying to change perf mode\n");
-	msleep(1);
+	mdelay(1);
 
 	ironlake_set_drps(dev, fstart);
 
@@ -2244,12 +2263,18 @@ static void ironlake_enable_drps(struct drm_device *dev)
 	dev_priv->last_time1 = jiffies_to_msecs(jiffies);
 	dev_priv->last_count2 = I915_READ(0x112f4);
 	getrawmonotonic(&dev_priv->last_time2);
+
+	spin_unlock_irq(&mchdev_lock);
 }
 
 static void ironlake_disable_drps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u16 rgvswctl = I915_READ16(MEMSWCTL);
+	u16 rgvswctl;
+
+	spin_lock_irq(&mchdev_lock);
+
+	rgvswctl = I915_READ16(MEMSWCTL);
 
 	/* Ack interrupts, disable EFC interrupt */
 	I915_WRITE(MEMINTREN, I915_READ(MEMINTREN) & ~MEMINT_EVAL_CHG_EN);
@@ -2260,11 +2285,12 @@ static void ironlake_disable_drps(struct drm_device *dev)
 
 	/* Go back to the starting frequency */
 	ironlake_set_drps(dev, dev_priv->fstart);
-	msleep(1);
+	mdelay(1);
 	rgvswctl |= MEMCTL_CMD_STS;
 	I915_WRITE(MEMSWCTL, rgvswctl);
-	msleep(1);
+	mdelay(1);
 
+	spin_unlock_irq(&mchdev_lock);
 }
 
 void gen6_set_rps(struct drm_device *dev, u8 val)
@@ -2695,21 +2721,6 @@ static const struct cparams {
 	{ 0, 800, 231, 23784 },
 };
 
-/**
- * Lock protecting IPS related data structures
- *   - i915_mch_dev
- *   - dev_priv->max_delay
- *   - dev_priv->min_delay
- *   - dev_priv->fmax
- *   - dev_priv->gpu_busy
- *   - dev_priv->gfx_power
- */
-static DEFINE_SPINLOCK(mchdev_lock);
-
-/* Global for IPS driver to get at the current i915 device. Protected by
- * mchdev_lock. */
-static struct drm_i915_private *i915_mch_dev;
-
 unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 {
 	u64 total_count, diff, ret;
@@ -2960,11 +2971,11 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
 	if (dev_priv->info->gen != 5)
 		return;
 
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 
 	__i915_update_gfx_val(dev_priv);
 
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 }
 
 unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
@@ -3015,7 +3026,7 @@ unsigned long i915_read_mch_val(void)
 	struct drm_i915_private *dev_priv;
 	unsigned long chipset_val, graphics_val, ret = 0;
 
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
@@ -3026,7 +3037,7 @@ unsigned long i915_read_mch_val(void)
 	ret = chipset_val + graphics_val;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3042,7 +3053,7 @@ bool i915_gpu_raise(void)
 	struct drm_i915_private *dev_priv;
 	bool ret = true;
 
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev) {
 		ret = false;
 		goto out_unlock;
@@ -3053,7 +3064,7 @@ bool i915_gpu_raise(void)
 		dev_priv->max_delay--;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3070,7 +3081,7 @@ bool i915_gpu_lower(void)
 	struct drm_i915_private *dev_priv;
 	bool ret = true;
 
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev) {
 		ret = false;
 		goto out_unlock;
@@ -3081,7 +3092,7 @@ bool i915_gpu_lower(void)
 		dev_priv->max_delay++;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3099,7 +3110,7 @@ bool i915_gpu_busy(void)
 	bool ret = false;
 	int i;
 
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
@@ -3108,7 +3119,7 @@ bool i915_gpu_busy(void)
 		ret |= !list_empty(&ring->request_list);
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3125,7 +3136,7 @@ bool i915_gpu_turbo_disable(void)
 	struct drm_i915_private *dev_priv;
 	bool ret = true;
 
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev) {
 		ret = false;
 		goto out_unlock;
@@ -3138,7 +3149,7 @@ bool i915_gpu_turbo_disable(void)
 		ret = false;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3166,18 +3177,18 @@ ips_ping_for_i915_load(void)
 
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv)
 {
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	i915_mch_dev = dev_priv;
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	ips_ping_for_i915_load();
 }
 
 void intel_gpu_ips_teardown(void)
 {
-	spin_lock(&mchdev_lock);
+	spin_lock_irq(&mchdev_lock);
 	i915_mch_dev = NULL;
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 }
 static void intel_init_emon(struct drm_device *dev)
 {
-- 
1.7.10.4

[PATCH 8/9] drm/ips: move drps/ips/ilk related variables into dev_priv->ips

[Daniel Vetter]

Like with the equivalent change for gen6+ rps state, this helps in
clarifying the code (and in fixing a few places that have fallen through
the cracks in the locking review).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h |   36 +++++++++-------
 drivers/gpu/drm/i915/i915_irq.c |   20 ++++-----
 drivers/gpu/drm/i915/intel_pm.c |   90 ++++++++++++++++++---------------------
 3 files changed, 72 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b104b7f..2a1f861 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -808,22 +808,26 @@ typedef struct drm_i915_private {
 		u8 max_delay;
 	} rps;
 
-
-	u8 cur_delay;
-	u8 min_delay;
-	u8 max_delay;
-	u8 fmax;
-	u8 fstart;
-
-	u64 last_count1;
-	unsigned long last_time1;
-	unsigned long chipset_power;
-	u64 last_count2;
-	struct timespec last_time2;
-	unsigned long gfx_power;
-	int c_m;
-	int r_t;
-	u8 corr;
+	/* ilk-only ips/rps state. Everything in here is protected by the global
+	 * mchdev_lock in intel_pm.c */
+	struct {
+		u8 cur_delay;
+		u8 min_delay;
+		u8 max_delay;
+		u8 fmax;
+		u8 fstart;
+
+		u64 last_count1;
+		unsigned long last_time1;
+		unsigned long chipset_power;
+		u64 last_count2;
+		struct timespec last_time2;
+		unsigned long gfx_power;
+		u8 corr;
+
+		int c_m;
+		int r_t;
+	} ips;
 
 	enum no_fbc_reason no_fbc_reason;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 75c631d..0b75d22 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -310,7 +310,7 @@ static void ironlake_handle_rps_change(struct drm_device *dev)
 
 	I915_WRITE16(MEMINTRSTS, I915_READ(MEMINTRSTS));
 
-	new_delay = dev_priv->cur_delay;
+	new_delay = dev_priv->ips.cur_delay;
 
 	I915_WRITE16(MEMINTRSTS, MEMINT_EVAL_CHG);
 	busy_up = I915_READ(RCPREVBSYTUPAVG);
@@ -320,19 +320,19 @@ static void ironlake_handle_rps_change(struct drm_device *dev)
 
 	/* Handle RCS change request from hw */
 	if (busy_up > max_avg) {
-		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;
+		if (dev_priv->ips.cur_delay != dev_priv->ips.max_delay)
+			new_delay = dev_priv->ips.cur_delay - 1;
+		if (new_delay < dev_priv->ips.max_delay)
+			new_delay = dev_priv->ips.max_delay;
 	} else if (busy_down < min_avg) {
-		if (dev_priv->cur_delay != dev_priv->min_delay)
-			new_delay = dev_priv->cur_delay + 1;
-		if (new_delay > dev_priv->min_delay)
-			new_delay = dev_priv->min_delay;
+		if (dev_priv->ips.cur_delay != dev_priv->ips.min_delay)
+			new_delay = dev_priv->ips.cur_delay + 1;
+		if (new_delay > dev_priv->ips.min_delay)
+			new_delay = dev_priv->ips.min_delay;
 	}
 
 	if (ironlake_set_drps(dev, new_delay))
-		dev_priv->cur_delay = new_delay;
+		dev_priv->ips.cur_delay = new_delay;
 
 	spin_unlock_irqrestore(&mchdev_lock, flags);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e582b50..0056362 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -593,7 +593,7 @@ static void i915_ironlake_get_mem_freq(struct drm_device *dev)
 		break;
 	}
 
-	dev_priv->r_t = dev_priv->mem_freq;
+	dev_priv->ips.r_t = dev_priv->mem_freq;
 
 	switch (csipll & 0x3ff) {
 	case 0x00c:
@@ -625,11 +625,11 @@ static void i915_ironlake_get_mem_freq(struct drm_device *dev)
 	}
 
 	if (dev_priv->fsb_freq == 3200) {
-		dev_priv->c_m = 0;
+		dev_priv->ips.c_m = 0;
 	} else if (dev_priv->fsb_freq > 3200 && dev_priv->fsb_freq <= 4800) {
-		dev_priv->c_m = 1;
+		dev_priv->ips.c_m = 1;
 	} else {
-		dev_priv->c_m = 2;
+		dev_priv->ips.c_m = 2;
 	}
 }
 
@@ -2162,12 +2162,6 @@ err_unref:
 
 /**
  * Lock protecting IPS related data structures
- *   - i915_mch_dev
- *   - dev_priv->max_delay
- *   - dev_priv->min_delay
- *   - dev_priv->fmax
- *   - dev_priv->gpu_busy
- *   - dev_priv->gfx_power
  */
 DEFINE_SPINLOCK(mchdev_lock);
 
@@ -2230,12 +2224,12 @@ static void ironlake_enable_drps(struct drm_device *dev)
 	vstart = (I915_READ(PXVFREQ_BASE + (fstart * 4)) & PXVFREQ_PX_MASK) >>
 		PXVFREQ_PX_SHIFT;
 
-	dev_priv->fmax = fmax; /* IPS callback will increase this */
-	dev_priv->fstart = fstart;
+	dev_priv->ips.fmax = fmax; /* IPS callback will increase this */
+	dev_priv->ips.fstart = fstart;
 
-	dev_priv->max_delay = fstart;
-	dev_priv->min_delay = fmin;
-	dev_priv->cur_delay = fstart;
+	dev_priv->ips.max_delay = fstart;
+	dev_priv->ips.min_delay = fmin;
+	dev_priv->ips.cur_delay = fstart;
 
 	DRM_DEBUG_DRIVER("fmax: %d, fmin: %d, fstart: %d\n",
 			 fmax, fmin, fstart);
@@ -2258,11 +2252,11 @@ static void ironlake_enable_drps(struct drm_device *dev)
 
 	ironlake_set_drps(dev, fstart);
 
-	dev_priv->last_count1 = I915_READ(0x112e4) + I915_READ(0x112e8) +
+	dev_priv->ips.last_count1 = I915_READ(0x112e4) + I915_READ(0x112e8) +
 		I915_READ(0x112e0);
-	dev_priv->last_time1 = jiffies_to_msecs(jiffies);
-	dev_priv->last_count2 = I915_READ(0x112f4);
-	getrawmonotonic(&dev_priv->last_time2);
+	dev_priv->ips.last_time1 = jiffies_to_msecs(jiffies);
+	dev_priv->ips.last_count2 = I915_READ(0x112f4);
+	getrawmonotonic(&dev_priv->ips.last_time2);
 
 	spin_unlock_irq(&mchdev_lock);
 }
@@ -2284,7 +2278,7 @@ static void ironlake_disable_drps(struct drm_device *dev)
 	I915_WRITE(DEIMR, I915_READ(DEIMR) | DE_PCU_EVENT);
 
 	/* Go back to the starting frequency */
-	ironlake_set_drps(dev, dev_priv->fstart);
+	ironlake_set_drps(dev, dev_priv->ips.fstart);
 	mdelay(1);
 	rgvswctl |= MEMCTL_CMD_STS;
 	I915_WRITE(MEMSWCTL, rgvswctl);
@@ -2730,7 +2724,7 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 
 	assert_spin_locked(&mchdev_lock);
 
-	diff1 = now - dev_priv->last_time1;
+	diff1 = now - dev_priv->ips.last_time1;
 
 	/* Prevent division-by-zero if we are asking too fast.
 	 * Also, we don't get interesting results if we are polling
@@ -2738,7 +2732,7 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	 * in such cases.
 	 */
 	if (diff1 <= 10)
-		return dev_priv->chipset_power;
+		return dev_priv->ips.chipset_power;
 
 	count1 = I915_READ(DMIEC);
 	count2 = I915_READ(DDREC);
@@ -2747,16 +2741,16 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	total_count = count1 + count2 + count3;
 
 	/* FIXME: handle per-counter overflow */
-	if (total_count < dev_priv->last_count1) {
-		diff = ~0UL - dev_priv->last_count1;
+	if (total_count < dev_priv->ips.last_count1) {
+		diff = ~0UL - dev_priv->ips.last_count1;
 		diff += total_count;
 	} else {
-		diff = total_count - dev_priv->last_count1;
+		diff = total_count - dev_priv->ips.last_count1;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(cparams); i++) {
-		if (cparams[i].i == dev_priv->c_m &&
-		    cparams[i].t == dev_priv->r_t) {
+		if (cparams[i].i == dev_priv->ips.c_m &&
+		    cparams[i].t == dev_priv->ips.r_t) {
 			m = cparams[i].m;
 			c = cparams[i].c;
 			break;
@@ -2767,10 +2761,10 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	ret = ((m * diff) + c);
 	ret = div_u64(ret, 10);
 
-	dev_priv->last_count1 = total_count;
-	dev_priv->last_time1 = now;
+	dev_priv->ips.last_count1 = total_count;
+	dev_priv->ips.last_time1 = now;
 
-	dev_priv->chipset_power = ret;
+	dev_priv->ips.chipset_power = ret;
 
 	return ret;
 }
@@ -2941,7 +2935,7 @@ void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 	assert_spin_locked(&mchdev_lock);
 
 	getrawmonotonic(&now);
-	diff1 = timespec_sub(now, dev_priv->last_time2);
+	diff1 = timespec_sub(now, dev_priv->ips.last_time2);
 
 	/* Don't divide by 0 */
 	diffms = diff1.tv_sec * 1000 + diff1.tv_nsec / 1000000;
@@ -2950,20 +2944,20 @@ void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 
 	count = I915_READ(GFXEC);
 
-	if (count < dev_priv->last_count2) {
-		diff = ~0UL - dev_priv->last_count2;
+	if (count < dev_priv->ips.last_count2) {
+		diff = ~0UL - dev_priv->ips.last_count2;
 		diff += count;
 	} else {
-		diff = count - dev_priv->last_count2;
+		diff = count - dev_priv->ips.last_count2;
 	}
 
-	dev_priv->last_count2 = count;
-	dev_priv->last_time2 = now;
+	dev_priv->ips.last_count2 = count;
+	dev_priv->ips.last_time2 = now;
 
 	/* More magic constants... */
 	diff = diff * 1181;
 	diff = div_u64(diff, diffms * 10);
-	dev_priv->gfx_power = diff;
+	dev_priv->ips.gfx_power = diff;
 }
 
 void i915_update_gfx_val(struct drm_i915_private *dev_priv)
@@ -3005,14 +2999,14 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 
 	corr = corr * ((150142 * state1) / 10000 - 78642);
 	corr /= 100000;
-	corr2 = (corr * dev_priv->corr);
+	corr2 = (corr * dev_priv->ips.corr);
 
 	state2 = (corr2 * state1) / 10000;
 	state2 /= 100; /* convert to mW */
 
 	__i915_update_gfx_val(dev_priv);
 
-	return dev_priv->gfx_power + state2;
+	return dev_priv->ips.gfx_power + state2;
 }
 
 /**
@@ -3060,8 +3054,8 @@ bool i915_gpu_raise(void)
 	}
 	dev_priv = i915_mch_dev;
 
-	if (dev_priv->max_delay > dev_priv->fmax)
-		dev_priv->max_delay--;
+	if (dev_priv->ips.max_delay > dev_priv->ips.fmax)
+		dev_priv->ips.max_delay--;
 
 out_unlock:
 	spin_unlock_irq(&mchdev_lock);
@@ -3088,8 +3082,8 @@ bool i915_gpu_lower(void)
 	}
 	dev_priv = i915_mch_dev;
 
-	if (dev_priv->max_delay < dev_priv->min_delay)
-		dev_priv->max_delay++;
+	if (dev_priv->ips.max_delay < dev_priv->ips.min_delay)
+		dev_priv->ips.max_delay++;
 
 out_unlock:
 	spin_unlock_irq(&mchdev_lock);
@@ -3143,9 +3137,9 @@ bool i915_gpu_turbo_disable(void)
 	}
 	dev_priv = i915_mch_dev;
 
-	dev_priv->max_delay = dev_priv->fstart;
+	dev_priv->ips.max_delay = dev_priv->ips.fstart;
 
-	if (!ironlake_set_drps(dev_priv->dev, dev_priv->fstart))
+	if (!ironlake_set_drps(dev_priv->dev, dev_priv->ips.fstart))
 		ret = false;
 
 out_unlock:
@@ -3258,7 +3252,7 @@ static void intel_init_emon(struct drm_device *dev)
 
 	lcfuse = I915_READ(LCFUSE02);
 
-	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
+	dev_priv->ips.corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
 void intel_disable_gt_powersave(struct drm_device *dev)
@@ -3783,8 +3777,8 @@ static void gen6_sanitize_pm(struct drm_device *dev)
 	 */
 	limits &= ~(0x3f << 16 | 0x3f << 24);
 	delay = dev_priv->rps.cur_delay;
-	if (delay < dev_priv->max_delay)
-		limits |= (dev_priv->max_delay & 0x3f) << 24;
+	if (delay < dev_priv->ips.max_delay)
+		limits |= (dev_priv->ips.max_delay & 0x3f) << 24;
 	if (delay > dev_priv->rps.min_delay)
 		limits |= (dev_priv->rps.min_delay & 0x3f) << 16;
 
-- 
1.7.10.4

[PATCH 9/9] drm/i915: enable rc6 on ilk again

[Daniel Vetter]

I have the faint hope that the total absence of any locking for the
rps code wasn't too good an idea and could very well have caused some
rc6 related regressions.

Unfortunately we've never managed to reproduce these issues on any of
our own machines, so the only way to go about this is to enable it and
see what happens.

While at it, kill some stale comments and improve the logging.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c |   25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0056362..28db44e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2343,31 +2343,26 @@ static void gen6_disable_rps(struct drm_device *dev)
 
 int intel_enable_rc6(const struct drm_device *dev)
 {
-	/*
-	 * Respect the kernel parameter if it is set
-	 */
+	/* Respect the kernel parameter if it is set */
 	if (i915_enable_rc6 >= 0)
 		return i915_enable_rc6;
 
-	/*
-	 * Disable RC6 on Ironlake
-	 */
-	if (INTEL_INFO(dev)->gen == 5)
-		return 0;
+	if (INTEL_INFO(dev)->gen == 5) {
+		DRM_DEBUG_DRIVER("Ironlake: only RC6 available\n");
+		return INTEL_RC6_ENABLE;
+	}
 
-	/* On Haswell, only RC6 is available. So let's enable it by default to
-	 * provide better testing and coverage since the beginning.
-	 */
-	if (IS_HASWELL(dev))
+	if (IS_HASWELL(dev)) {
+		DRM_DEBUG_DRIVER("Haswell: only RC6 available\n");
 		return INTEL_RC6_ENABLE;
+	}
 
-	/*
-	 * Disable rc6 on Sandybridge
-	 */
+	/* snb/ivb have more than one rc6 state. */
 	if (INTEL_INFO(dev)->gen == 6) {
 		DRM_DEBUG_DRIVER("Sandybridge: deep RC6 disabled\n");
 		return INTEL_RC6_ENABLE;
 	}
+
 	DRM_DEBUG_DRIVER("RC6 and deep RC6 enabled\n");
 	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
-- 
1.7.10.4

Re: [PATCH 2/9] drm/i915: properly guard ilk ips state

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:43 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> The update_gfx_val function called from mark_busy wasn't taking the
> mchdev_lock, as it should have. Also sprinkle a few spinlock asserts
> over the code to document things better.
> 
> Things are still rather confusing, especially since a few variables
> in dev_priv are used by both the gen6+ rps code and the ilk ips code.
> But protected by totally different locks. Follow-on patches will clean
> that up.
> 
> v2: Don't add a deadlock ... hence split up update_gfx_val into a
> wrapper that grabs the lock and an internal __ variant for callsites
> within intel_pm.c that already have taken the lock.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |   50 ++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b6e8fbf..21a0088 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2693,6 +2693,21 @@ static const struct cparams {
>  	{ 0, 800, 231, 23784 },
>  };
>  
> +/**
> + * Lock protecting IPS related data structures
> + *   - i915_mch_dev
> + *   - dev_priv->max_delay
> + *   - dev_priv->min_delay
> + *   - dev_priv->fmax
> + *   - dev_priv->gpu_busy
> + *   - dev_priv->gfx_power
> + */
> +static DEFINE_SPINLOCK(mchdev_lock);
> +
> +/* Global for IPS driver to get at the current i915 device. Protected by
> + * mchdev_lock. */
> +static struct drm_i915_private *i915_mch_dev;
> +
>  unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
>  {
>  	u64 total_count, diff, ret;
> @@ -2700,6 +2715,8 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
>  	unsigned long now = jiffies_to_msecs(jiffies), diff1;
>  	int i;
>  
> +	assert_spin_locked(&mchdev_lock);
> +
>  	diff1 = now - dev_priv->last_time1;
>  
>  	/* Prevent division-by-zero if we are asking too fast.
> @@ -2901,15 +2918,14 @@ static u16 pvid_to_extvid(struct drm_i915_private *dev_priv, u8 pxvid)
>  		return v_table[pxvid].vd;
>  }
>  
> -void i915_update_gfx_val(struct drm_i915_private *dev_priv)
> +void __i915_update_gfx_val(struct drm_i915_private *dev_priv)

If I am understanding what you're trying to do, this should be static.

>  {
>  	struct timespec now, diff1;
>  	u64 diff;
>  	unsigned long diffms;
>  	u32 count;
>  
> -	if (dev_priv->info->gen != 5)
> -		return;
> +	assert_spin_locked(&mchdev_lock);

And if it's static, I think the assertion is unnecessary since all
callers are easily checked within the file. NOTE: you've killed a bunch
of my assertions previously for similar reasons, so I'm just letting off
steam.

>  
>  	getrawmonotonic(&now);
>  	diff1 = timespec_sub(now, dev_priv->last_time2);
> @@ -2937,11 +2953,25 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
>  	dev_priv->gfx_power = diff;
>  }
>  
> +void i915_update_gfx_val(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->info->gen != 5)
> +		return;
> +
> +	spin_lock(&mchdev_lock);
> +
> +	__i915_update_gfx_val(dev_priv);
> +
> +	spin_unlock(&mchdev_lock);
> +}
> +
>  unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
>  {
>  	unsigned long t, corr, state1, corr2, state2;
>  	u32 pxvid, ext_v;
>  
> +	assert_spin_locked(&mchdev_lock);
> +
>  	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
>  	pxvid = (pxvid >> 24) & 0x7f;
>  	ext_v = pvid_to_extvid(dev_priv, pxvid);
> @@ -2967,23 +2997,11 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
>  	state2 = (corr2 * state1) / 10000;
>  	state2 /= 100; /* convert to mW */
>  
> -	i915_update_gfx_val(dev_priv);
> +	__i915_update_gfx_val(dev_priv);
>  
>  	return dev_priv->gfx_power + state2;
>  }
>  
> -/* Global for IPS driver to get at the current i915 device */
> -static struct drm_i915_private *i915_mch_dev;
> -/*
> - * Lock protecting IPS related data structures
> - *   - i915_mch_dev
> - *   - dev_priv->max_delay
> - *   - dev_priv->min_delay
> - *   - dev_priv->fmax
> - *   - dev_priv->gpu_busy
> - */
> -static DEFINE_SPINLOCK(mchdev_lock);
> -
>  /**
>   * i915_read_mch_val - return value for IPS use
>   *



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/9] drm/i915: fixup up debugfs rps state handling

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:44 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> - Take the dev->struct_mutex around access the corresponding state
>   (and adjusting the rps hw state).
> - Add an assert to gen6_set_rps to ensure we don't forget about this
>   in the future.
> - Don't set up the min/max_freq files if it doesn't apply to the hw.
>   And do the same for the gen6+ cache sharing file while at it.
> 
> v2: Move the gen6+ checks into the read/write callbacks. Thanks to the
> awesome drm midlayer we can't check this when registering the debugfs
> files, because the driver is not yet fully set up, specifically the
> ->load callback hasn't run yet.
> 
> Oh how I despise & loathe this disaster ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I think it's wise to use some kind of HAS_RPS() macro, but otherwise:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_pm.c     |    2 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1312b79..2499610 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1710,8 +1710,13 @@ i915_max_freq_read(struct file *filp,
>  	char buf[80];
>  	int len;
>  
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> +		return -ENODEV;
> +
> +	mutex_lock(&dev->struct_mutex);
>  	len = snprintf(buf, sizeof(buf),
>  		       "max freq: %d\n", dev_priv->max_delay * 50);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	if (len > sizeof(buf))
>  		len = sizeof(buf);
> @@ -1730,6 +1735,9 @@ i915_max_freq_write(struct file *filp,
>  	char buf[20];
>  	int val = 1;
>  
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> +		return -ENODEV;
> +
>  	if (cnt > 0) {
>  		if (cnt > sizeof(buf) - 1)
>  			return -EINVAL;
> @@ -1743,12 +1751,14 @@ i915_max_freq_write(struct file *filp,
>  
>  	DRM_DEBUG_DRIVER("Manually setting max freq to %d\n", val);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	/*
>  	 * Turbo will still be enabled, but won't go above the set value.
>  	 */
>  	dev_priv->max_delay = val / 50;
>  
>  	gen6_set_rps(dev, val / 50);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	return cnt;
>  }
> @@ -1770,8 +1780,13 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
>  	char buf[80];
>  	int len;
>  
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> +		return -ENODEV;
> +
> +	mutex_lock(&dev->struct_mutex);
>  	len = snprintf(buf, sizeof(buf),
>  		       "min freq: %d\n", dev_priv->min_delay * 50);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	if (len > sizeof(buf))
>  		len = sizeof(buf);
> @@ -1788,6 +1803,9 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	char buf[20];
>  	int val = 1;
>  
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> +		return -ENODEV;
> +
>  	if (cnt > 0) {
>  		if (cnt > sizeof(buf) - 1)
>  			return -EINVAL;
> @@ -1801,12 +1819,14 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  
>  	DRM_DEBUG_DRIVER("Manually setting min freq to %d\n", val);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	/*
>  	 * Turbo will still be enabled, but won't go below the set value.
>  	 */
>  	dev_priv->min_delay = val / 50;
>  
>  	gen6_set_rps(dev, val / 50);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	return cnt;
>  }
> @@ -1831,6 +1851,9 @@ i915_cache_sharing_read(struct file *filp,
>  	u32 snpcr;
>  	int len;
>  
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> +		return -ENODEV;
> +
>  	mutex_lock(&dev_priv->dev->struct_mutex);
>  	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
> @@ -1857,6 +1880,9 @@ i915_cache_sharing_write(struct file *filp,
>  	u32 snpcr;
>  	int val = 1;
>  
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
> +		return -ENODEV;
> +
>  	if (cnt > 0) {
>  		if (cnt > sizeof(buf) - 1)
>  			return -EINVAL;
> @@ -2061,6 +2087,7 @@ int i915_debugfs_init(struct drm_minor *minor)
>  				  &i915_cache_sharing_fops);
>  	if (ret)
>  		return ret;
> +
>  	ret = i915_debugfs_create(minor->debugfs_root, minor,
>  				  "i915_ring_stop",
>  				  &i915_ring_stop_fops);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 21a0088..d5af41b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2272,6 +2272,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 limits;
>  
> +	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +
>  	limits = 0;
>  	if (val >= dev_priv->max_delay)
>  		val = dev_priv->max_delay;



-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: properly guard ilk ips state

[Daniel Vetter]

The update_gfx_val function called from mark_busy wasn't taking the
mchdev_lock, as it should have. Also sprinkle a few spinlock asserts
over the code to document things better.

Things are still rather confusing, especially since a few variables
in dev_priv are used by both the gen6+ rps code and the ilk ips code.
But protected by totally different locks. Follow-on patches will clean
that up.

v2: Don't add a deadlock ... hence split up update_gfx_val into a
wrapper that grabs the lock and an internal __ variant for callsites
within intel_pm.c that already have taken the lock.

v3: Mark the internal helper as static, noticed by Ben Widawsky.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c |   50 ++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b6e8fbf..65f071e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2693,6 +2693,21 @@ static const struct cparams {
 	{ 0, 800, 231, 23784 },
 };
 
+/**
+ * Lock protecting IPS related data structures
+ *   - i915_mch_dev
+ *   - dev_priv->max_delay
+ *   - dev_priv->min_delay
+ *   - dev_priv->fmax
+ *   - dev_priv->gpu_busy
+ *   - dev_priv->gfx_power
+ */
+static DEFINE_SPINLOCK(mchdev_lock);
+
+/* Global for IPS driver to get at the current i915 device. Protected by
+ * mchdev_lock. */
+static struct drm_i915_private *i915_mch_dev;
+
 unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 {
 	u64 total_count, diff, ret;
@@ -2700,6 +2715,8 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	unsigned long now = jiffies_to_msecs(jiffies), diff1;
 	int i;
 
+	assert_spin_locked(&mchdev_lock);
+
 	diff1 = now - dev_priv->last_time1;
 
 	/* Prevent division-by-zero if we are asking too fast.
@@ -2901,15 +2918,14 @@ static u16 pvid_to_extvid(struct drm_i915_private *dev_priv, u8 pxvid)
 		return v_table[pxvid].vd;
 }
 
-void i915_update_gfx_val(struct drm_i915_private *dev_priv)
+static void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 {
 	struct timespec now, diff1;
 	u64 diff;
 	unsigned long diffms;
 	u32 count;
 
-	if (dev_priv->info->gen != 5)
-		return;
+	assert_spin_locked(&mchdev_lock);
 
 	getrawmonotonic(&now);
 	diff1 = timespec_sub(now, dev_priv->last_time2);
@@ -2937,11 +2953,25 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
 	dev_priv->gfx_power = diff;
 }
 
+void i915_update_gfx_val(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->info->gen != 5)
+		return;
+
+	spin_lock(&mchdev_lock);
+
+	__i915_update_gfx_val(dev_priv);
+
+	spin_unlock(&mchdev_lock);
+}
+
 unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 {
 	unsigned long t, corr, state1, corr2, state2;
 	u32 pxvid, ext_v;
 
+	assert_spin_locked(&mchdev_lock);
+
 	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
 	pxvid = (pxvid >> 24) & 0x7f;
 	ext_v = pvid_to_extvid(dev_priv, pxvid);
@@ -2967,23 +2997,11 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 	state2 = (corr2 * state1) / 10000;
 	state2 /= 100; /* convert to mW */
 
-	i915_update_gfx_val(dev_priv);
+	__i915_update_gfx_val(dev_priv);
 
 	return dev_priv->gfx_power + state2;
 }
 
-/* Global for IPS driver to get at the current i915 device */
-static struct drm_i915_private *i915_mch_dev;
-/*
- * Lock protecting IPS related data structures
- *   - i915_mch_dev
- *   - dev_priv->max_delay
- *   - dev_priv->min_delay
- *   - dev_priv->fmax
- *   - dev_priv->gpu_busy
- */
-static DEFINE_SPINLOCK(mchdev_lock);
-
 /**
  * i915_read_mch_val - return value for IPS use
  *
-- 
1.7.10.4

Re: [PATCH 4/9] drm/i915: move all rps state into dev_priv->rps

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:45 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> This way it's easier so see what belongs together, and what is used
> by the ilk ips code. Also add some comments that explain the locking.
> 
> v2: Missed one place that the dev_priv->ips change caught ...
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
With a few comments below

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |   11 +++----
>  drivers/gpu/drm/i915/i915_dma.c      |    2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |   18 ++++++++++--
>  drivers/gpu/drm/i915/i915_irq.c      |   32 ++++++++++-----------
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  drivers/gpu/drm/i915/intel_pm.c      |   52 +++++++++++++++++-----------------
>  6 files changed, 65 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2499610..05087fa 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1287,7 +1287,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  
>  	seq_printf(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\n");
>  
> -	for (gpu_freq = dev_priv->min_delay; gpu_freq <= dev_priv->max_delay;
> +	for (gpu_freq = dev_priv->rps.min_delay;
> +	     gpu_freq <= dev_priv->rps.max_delay;
>  	     gpu_freq++) {
>  		I915_WRITE(GEN6_PCODE_DATA, gpu_freq);
>  		I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY |
> @@ -1715,7 +1716,7 @@ i915_max_freq_read(struct file *filp,
>  
>  	mutex_lock(&dev->struct_mutex);
>  	len = snprintf(buf, sizeof(buf),
> -		       "max freq: %d\n", dev_priv->max_delay * 50);
> +		       "max freq: %d\n", dev_priv->rps.max_delay * 50);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	if (len > sizeof(buf))
> @@ -1755,7 +1756,7 @@ i915_max_freq_write(struct file *filp,
>  	/*
>  	 * Turbo will still be enabled, but won't go above the set value.
>  	 */
> -	dev_priv->max_delay = val / 50;
> +	dev_priv->rps.max_delay = val / 50;
>  
>  	gen6_set_rps(dev, val / 50);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -1785,7 +1786,7 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
>  
>  	mutex_lock(&dev->struct_mutex);
>  	len = snprintf(buf, sizeof(buf),
> -		       "min freq: %d\n", dev_priv->min_delay * 50);
> +		       "min freq: %d\n", dev_priv->rps.min_delay * 50);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	if (len > sizeof(buf))
> @@ -1823,7 +1824,7 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	/*
>  	 * Turbo will still be enabled, but won't go below the set value.
>  	 */
> -	dev_priv->min_delay = val / 50;
> +	dev_priv->rps.min_delay = val / 50;
>  
>  	gen6_set_rps(dev, val / 50);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5e20f11..5f63638 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1586,7 +1586,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);
> +	spin_lock_init(&dev_priv->rps.lock);
>  
>  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
>  		dev_priv->num_pipe = 3;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f176589..2496d60 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -793,9 +793,21 @@ typedef struct drm_i915_private {
>  
>  	bool mchbar_need_disable;
>  
> -	struct work_struct rps_work;
> -	spinlock_t rps_lock;
> -	u32 pm_iir;
> +	/* gen6+ rps state */
> +	struct {
> +		struct work_struct work;
> +		u32 pm_iir;
> +		/* lock - irqsave spinlock that protectects the work_struct and
> +		 * pm_iir. */
> +		spinlock_t lock;
> +
> +		/* The below variables an all the rps hw state are protected by
> +		 * dev->struct mutext. */
> +		u8 cur_delay;
> +		u8 min_delay;
> +		u8 max_delay;
> +	} rps;
> +
>  
>  	u8 cur_delay;
>  	u8 min_delay;

Could you add the reason for adding new cur/min/max delays to the commit
message? From just this hunk it would seem we'd want to remove the old
cur/min/max.

> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 41ed41d..6e3f43c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -349,16 +349,16 @@ static void notify_ring(struct drm_device *dev,
>  static void gen6_pm_rps_work(struct work_struct *work)
>  {
>  	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> -						    rps_work);
> +						    rps.work);
>  	u32 pm_iir, pm_imr;
>  	u8 new_delay;
>  
> -	spin_lock_irq(&dev_priv->rps_lock);
> -	pm_iir = dev_priv->pm_iir;
> -	dev_priv->pm_iir = 0;
> +	spin_lock_irq(&dev_priv->rps.lock);
> +	pm_iir = dev_priv->rps.pm_iir;
> +	dev_priv->rps.pm_iir = 0;
>  	pm_imr = I915_READ(GEN6_PMIMR);
>  	I915_WRITE(GEN6_PMIMR, 0);
> -	spin_unlock_irq(&dev_priv->rps_lock);
> +	spin_unlock_irq(&dev_priv->rps.lock);
>  
>  	if ((pm_iir & GEN6_PM_DEFERRED_EVENTS) == 0)
>  		return;
> @@ -366,9 +366,9 @@ static void gen6_pm_rps_work(struct work_struct *work)
>  	mutex_lock(&dev_priv->dev->struct_mutex);
>  
>  	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD)
> -		new_delay = dev_priv->cur_delay + 1;
> +		new_delay = dev_priv->rps.cur_delay + 1;
>  	else
> -		new_delay = dev_priv->cur_delay - 1;
> +		new_delay = dev_priv->rps.cur_delay - 1;
>  
>  	gen6_set_rps(dev_priv->dev, new_delay);
>  
> @@ -488,20 +488,20 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
>  	 * 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
> +	 * dev_priv->rps.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.
> +	 * The mask bit in IMR is cleared by dev_priv->rps.work.
>  	 */
>  
> -	spin_lock_irqsave(&dev_priv->rps_lock, flags);
> -	WARN(dev_priv->pm_iir & pm_iir, "Missed a PM interrupt\n");
> -	dev_priv->pm_iir |= pm_iir;
> -	I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir);
> +	spin_lock_irqsave(&dev_priv->rps.lock, flags);
> +	WARN(dev_priv->rps.pm_iir & pm_iir, "Missed a PM interrupt\n");
> +	dev_priv->rps.pm_iir |= pm_iir;
> +	I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
>  	POSTING_READ(GEN6_PMIMR);
> -	spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
> +	spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
>  
> -	queue_work(dev_priv->wq, &dev_priv->rps_work);
> +	queue_work(dev_priv->wq, &dev_priv->rps.work);
>  }
>  
>  static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
> @@ -2649,7 +2649,7 @@ void intel_irq_init(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);
> +	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
>  	INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
>  
>  	dev->driver->get_vblank_counter = i915_get_vblank_counter;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index be45b92..00a90e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7220,7 +7220,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  	 * enqueue unpin/hotplug work. */
>  	drm_irq_uninstall(dev);
>  	cancel_work_sync(&dev_priv->hotplug_work);
> -	cancel_work_sync(&dev_priv->rps_work);
> +	cancel_work_sync(&dev_priv->rps.work);
>  
>  	/* flush any delayed tasks or pending work */
>  	flush_scheduled_work();
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d5af41b..6c9925d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2275,17 +2275,17 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
>  	limits = 0;
> -	if (val >= dev_priv->max_delay)
> -		val = dev_priv->max_delay;
> +	if (val >= dev_priv->rps.max_delay)
> +		val = dev_priv->rps.max_delay;
>  	else
> -		limits |= dev_priv->max_delay << 24;
> +		limits |= dev_priv->rps.max_delay << 24;
>  
> -	if (val <= dev_priv->min_delay)
> -		val = dev_priv->min_delay;
> +	if (val <= dev_priv->rps.min_delay)
> +		val = dev_priv->rps.min_delay;
>  	else
> -		limits |= dev_priv->min_delay << 16;
> +		limits |= dev_priv->rps.min_delay << 16;
>  
> -	if (val == dev_priv->cur_delay)
> +	if (val == dev_priv->rps.cur_delay)
>  		return;
>  
>  	I915_WRITE(GEN6_RPNSWREQ,
> @@ -2298,7 +2298,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
>  	 */
>  	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
>  
> -	dev_priv->cur_delay = val;
> +	dev_priv->rps.cur_delay = val;
>  }
>  
>  static void gen6_disable_rps(struct drm_device *dev)
> @@ -2314,9 +2314,9 @@ static void gen6_disable_rps(struct drm_device *dev)
>  	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
>  	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
>  
> -	spin_lock_irq(&dev_priv->rps_lock);
> -	dev_priv->pm_iir = 0;
> -	spin_unlock_irq(&dev_priv->rps_lock);
> +	spin_lock_irq(&dev_priv->rps.lock);
> +	dev_priv->rps.pm_iir = 0;
> +	spin_unlock_irq(&dev_priv->rps.lock);
>  
>  	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
>  }
> @@ -2385,9 +2385,9 @@ static void gen6_enable_rps(struct drm_device *dev)
>  	gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
>  
>  	/* In units of 100MHz */
> -	dev_priv->max_delay = rp_state_cap & 0xff;
> -	dev_priv->min_delay = (rp_state_cap & 0xff0000) >> 16;
> -	dev_priv->cur_delay = 0;
> +	dev_priv->rps.max_delay = rp_state_cap & 0xff;
> +	dev_priv->rps.min_delay = (rp_state_cap & 0xff0000) >> 16;
> +	dev_priv->rps.cur_delay = 0;
>  
>  	/* disable the counters and set deterministic thresholds */
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
> @@ -2440,8 +2440,8 @@ static void gen6_enable_rps(struct drm_device *dev)
>  
>  	I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1000000);
>  	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> -		   dev_priv->max_delay << 24 |
> -		   dev_priv->min_delay << 16);
> +		   dev_priv->rps.max_delay << 24 |
> +		   dev_priv->rps.min_delay << 16);
>  
>  	if (IS_HASWELL(dev)) {
>  		I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> @@ -2486,7 +2486,7 @@ static void gen6_enable_rps(struct drm_device *dev)
>  		     500))
>  		DRM_ERROR("timeout waiting for pcode mailbox to finish\n");
>  	if (pcu_mbox & (1<<31)) { /* OC supported */
> -		dev_priv->max_delay = pcu_mbox & 0xff;
> +		dev_priv->rps.max_delay = pcu_mbox & 0xff;
>  		DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
>  	}
>  
> @@ -2494,10 +2494,10 @@ static void gen6_enable_rps(struct drm_device *dev)
>  
>  	/* requires MSI enabled */
>  	I915_WRITE(GEN6_PMIER, GEN6_PM_DEFERRED_EVENTS);
> -	spin_lock_irq(&dev_priv->rps_lock);
> -	WARN_ON(dev_priv->pm_iir != 0);
> +	spin_lock_irq(&dev_priv->rps.lock);
> +	WARN_ON(dev_priv->rps.pm_iir != 0);
>  	I915_WRITE(GEN6_PMIMR, 0);
> -	spin_unlock_irq(&dev_priv->rps_lock);
> +	spin_unlock_irq(&dev_priv->rps.lock);
>  	/* enable all PM interrupts */
>  	I915_WRITE(GEN6_PMINTRMSK, 0);
>  
> @@ -2529,9 +2529,9 @@ static void gen6_update_ring_freq(struct drm_device *dev)
>  	 * to use for memory access.  We do this by specifying the IA frequency
>  	 * the PCU should use as a reference to determine the ring frequency.
>  	 */
> -	for (gpu_freq = dev_priv->max_delay; gpu_freq >= dev_priv->min_delay;
> +	for (gpu_freq = dev_priv->rps.max_delay; gpu_freq >= dev_priv->rps.min_delay;
>  	     gpu_freq--) {
> -		int diff = dev_priv->max_delay - gpu_freq;
> +		int diff = dev_priv->rps.max_delay - gpu_freq;
>  
>  		/*
>  		 * For GPU frequencies less than 750MHz, just use the lowest
> @@ -2974,7 +2974,7 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
>  
>  	assert_spin_locked(&mchdev_lock);
>  
> -	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
> +	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->rps.cur_delay * 4));
>  	pxvid = (pxvid >> 24) & 0x7f;
>  	ext_v = pvid_to_extvid(dev_priv, pxvid);
>  
> @@ -3772,11 +3772,11 @@ static void gen6_sanitize_pm(struct drm_device *dev)
>  	 * until we hit the minimum or maximum frequencies.
>  	 */
>  	limits &= ~(0x3f << 16 | 0x3f << 24);
> -	delay = dev_priv->cur_delay;
> +	delay = dev_priv->rps.cur_delay;
>  	if (delay < dev_priv->max_delay)
>  		limits |= (dev_priv->max_delay & 0x3f) << 24;
> -	if (delay > dev_priv->min_delay)
> -		limits |= (dev_priv->min_delay & 0x3f) << 16;
> +	if (delay > dev_priv->rps.min_delay)
> +		limits |= (dev_priv->rps.min_delay & 0x3f) << 16;
>  
>  	if (old != limits) {
>  		/* Note that the known failure case is to read back 0. */



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 6/9] drm/i915: DE_PCU_EVENT irq is ilk-only

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:47 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Like all the other drps/ips stuff. Hence add the corresponding check,
> give the function a preciser prefix and move the single reg clearing into
> the rps handling function, too.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 8/9] drm/ips: move drps/ips/ilk related variables into dev_priv->ips

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:49 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Like with the equivalent change for gen6+ rps state, this helps in
> clarifying the code (and in fixing a few places that have fallen through
> the cracks in the locking review).
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I don't think this patch is necessary, and doesn't belong in the series.
The series was about fixing a locking problem. If you want to submit
this as a separate patch, I'd prefer it.

If you're really determined to keep it, I'd roll it into the earlier
patches that did the rps renaming.

Re: [PATCH 4/9] drm/i915: move all rps state into dev_priv->rps

[Daniel Vetter]

On Wed, Jul 25, 2012 at 12:25:00PM -0700, Ben Widawsky wrote:
> On Tue, 24 Jul 2012 23:33:45 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > This way it's easier so see what belongs together, and what is used
> > by the ilk ips code. Also add some comments that explain the locking.
> > 
> > v2: Missed one place that the dev_priv->ips change caught ...
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> With a few comments below
> 
> > +	/* gen6+ rps state */
> > +	struct {
> > +		struct work_struct work;
> > +		u32 pm_iir;
> > +		/* lock - irqsave spinlock that protectects the work_struct and
> > +		 * pm_iir. */
> > +		spinlock_t lock;
> > +
> > +		/* The below variables an all the rps hw state are protected by
> > +		 * dev->struct mutext. */
> > +		u8 cur_delay;
> > +		u8 min_delay;
> > +		u8 max_delay;
> > +	} rps;
> > +
> >  
> >  	u8 cur_delay;
> >  	u8 min_delay;
> 
> Could you add the reason for adding new cur/min/max delays to the commit
> message? From just this hunk it would seem we'd want to remove the old
> cur/min/max.

I'll add a comment that (cur|min|max)_delay need to be duplicated, because
they're also used by the ips code when applying the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 9/9] drm/i915: enable rc6 on ilk again

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:50 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> I have the faint hope that the total absence of any locking for the
> rps code wasn't too good an idea and could very well have caused some
> rc6 related regressions.
> 
> Unfortunately we've never managed to reproduce these issues on any of
> our own machines, so the only way to go about this is to enable it and
> see what happens.
> 
> While at it, kill some stale comments and improve the logging.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Here as with the previous patch, I don't think it belongs in the series.

Re: [PATCH 8/9] drm/ips: move drps/ips/ilk related variables into dev_priv->ips

[Daniel Vetter]

On Wed, Jul 25, 2012 at 02:25:20PM -0700, Ben Widawsky wrote:
> On Tue, 24 Jul 2012 23:33:49 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Like with the equivalent change for gen6+ rps state, this helps in
> > clarifying the code (and in fixing a few places that have fallen through
> > the cracks in the locking review).
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I don't think this patch is necessary, and doesn't belong in the series.
> The series was about fixing a locking problem. If you want to submit
> this as a separate patch, I'd prefer it.
> 
> If you're really determined to keep it, I'd roll it into the earlier
> patches that did the rps renaming.

Well, you've already smashed your r-b onto the equivalent patch for the
gen6+ rps code. But the real reason this belongs to this series is that
I've used this rename (and the rps one) to actually figure out (with the
help of the compiler) what is actually touched where and which parts
belong together. 'Cause the current code is a rather decent mess.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 8/9] drm/ips: move drps/ips/ilk related variables into dev_priv->ips

[Ben Widawsky]

On Wed, 25 Jul 2012 23:32:16 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jul 25, 2012 at 02:25:20PM -0700, Ben Widawsky wrote:
> > On Tue, 24 Jul 2012 23:33:49 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > > Like with the equivalent change for gen6+ rps state, this helps in
> > > clarifying the code (and in fixing a few places that have fallen through
> > > the cracks in the locking review).
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > I don't think this patch is necessary, and doesn't belong in the series.
> > The series was about fixing a locking problem. If you want to submit
> > this as a separate patch, I'd prefer it.
> > 
> > If you're really determined to keep it, I'd roll it into the earlier
> > patches that did the rps renaming.
> 
> Well, you've already smashed your r-b onto the equivalent patch for the
> gen6+ rps code. But the real reason this belongs to this series is that
> I've used this rename (and the rps one) to actually figure out (with the
> help of the compiler) what is actually touched where and which parts
> belong together. 'Cause the current code is a rather decent mess.
> 
> -Daniel

You've shot down quite a few patches of mine (usually assertions) which
I've used for similar, 'this helped me track down an issue' purposes.

In any case, the r-b on the other one is because you're restructuring
the code you want to fix, before you fix it. That is fine. As I said,
if you want to put this as a beautifier, I don't think it belongs in
the series.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: properly guard ilk ips state

[Daniel Vetter]

The update_gfx_val function called from mark_busy wasn't taking the
mchdev_lock, as it should have. Also sprinkle a few spinlock asserts
over the code to document things better.

Things are still rather confusing, especially since a few variables
in dev_priv are used by both the gen6+ rps code and the ilk ips code.
But protected by totally different locks. Follow-on patches will clean
that up.

v2: Don't add a deadlock ... hence split up update_gfx_val into a
wrapper that grabs the lock and an internal __ variant for callsites
within intel_pm.c that already have taken the lock.

v3: Mark the internal helper as static, noticed by Ben Widawsky.

v4: Damien Lespiau had questions about the safety of the ips setup
sequence, explain in a comment why it works.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c |   52 +++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5050bb8..ec3fe92 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2710,6 +2710,21 @@ static const struct cparams {
 	{ 0, 800, 231, 23784 },
 };
 
+/**
+ * Lock protecting IPS related data structures
+ *   - i915_mch_dev
+ *   - dev_priv->max_delay
+ *   - dev_priv->min_delay
+ *   - dev_priv->fmax
+ *   - dev_priv->gpu_busy
+ *   - dev_priv->gfx_power
+ */
+static DEFINE_SPINLOCK(mchdev_lock);
+
+/* Global for IPS driver to get at the current i915 device. Protected by
+ * mchdev_lock. */
+static struct drm_i915_private *i915_mch_dev;
+
 unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 {
 	u64 total_count, diff, ret;
@@ -2717,6 +2732,8 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
 	unsigned long now = jiffies_to_msecs(jiffies), diff1;
 	int i;
 
+	assert_spin_locked(&mchdev_lock);
+
 	diff1 = now - dev_priv->last_time1;
 
 	/* Prevent division-by-zero if we are asking too fast.
@@ -2918,15 +2935,14 @@ static u16 pvid_to_extvid(struct drm_i915_private *dev_priv, u8 pxvid)
 		return v_table[pxvid].vd;
 }
 
-void i915_update_gfx_val(struct drm_i915_private *dev_priv)
+static void __i915_update_gfx_val(struct drm_i915_private *dev_priv)
 {
 	struct timespec now, diff1;
 	u64 diff;
 	unsigned long diffms;
 	u32 count;
 
-	if (dev_priv->info->gen != 5)
-		return;
+	assert_spin_locked(&mchdev_lock);
 
 	getrawmonotonic(&now);
 	diff1 = timespec_sub(now, dev_priv->last_time2);
@@ -2954,11 +2970,25 @@ void i915_update_gfx_val(struct drm_i915_private *dev_priv)
 	dev_priv->gfx_power = diff;
 }
 
+void i915_update_gfx_val(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->info->gen != 5)
+		return;
+
+	spin_lock(&mchdev_lock);
+
+	__i915_update_gfx_val(dev_priv);
+
+	spin_unlock(&mchdev_lock);
+}
+
 unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 {
 	unsigned long t, corr, state1, corr2, state2;
 	u32 pxvid, ext_v;
 
+	assert_spin_locked(&mchdev_lock);
+
 	pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
 	pxvid = (pxvid >> 24) & 0x7f;
 	ext_v = pvid_to_extvid(dev_priv, pxvid);
@@ -2984,23 +3014,11 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
 	state2 = (corr2 * state1) / 10000;
 	state2 /= 100; /* convert to mW */
 
-	i915_update_gfx_val(dev_priv);
+	__i915_update_gfx_val(dev_priv);
 
 	return dev_priv->gfx_power + state2;
 }
 
-/* Global for IPS driver to get at the current i915 device */
-static struct drm_i915_private *i915_mch_dev;
-/*
- * Lock protecting IPS related data structures
- *   - i915_mch_dev
- *   - dev_priv->max_delay
- *   - dev_priv->min_delay
- *   - dev_priv->fmax
- *   - dev_priv->gpu_busy
- */
-static DEFINE_SPINLOCK(mchdev_lock);
-
 /**
  * i915_read_mch_val - return value for IPS use
  *
@@ -3163,6 +3181,8 @@ ips_ping_for_i915_load(void)
 
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv)
 {
+	/* We only register the i915 ips part with intel-ips once everything is
+	 * set up, to avoid intel-ips sneaking in and reading bogus values. */
 	spin_lock(&mchdev_lock);
 	i915_mch_dev = dev_priv;
 	dev_priv->mchdev_lock = &mchdev_lock;
-- 
1.7.10.4