[PATCH 0/8] rps locking fixes v2

[PATCH 0/8] rps locking fixes v2

[Daniel Vetter]

Hi all,

Essentially just rebase, with Ben's review comments taking into account and one
WARN_ON(mutex_is_locked) moved around a bit.

Review&testing highly welcome.

Cheers, Daniel

Daniel Vetter (8):
  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      |  248 ++++++++++++++++++----------------
 6 files changed, 245 insertions(+), 172 deletions(-)

-- 
1.7.10.4

[PATCH 1/8] 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 5050bb8..ad90579 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
  *
-- 
1.7.10.4

[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

[PATCH 2/8] 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 that 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 this disaster ...

v3: Also add a WARN_ON(mutex_is_locked) in set_rps to check the
locking.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (for v2)
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 ad90579..dece479 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2300,6 +2300,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 limits = gen6_rps_limits(dev_priv, &val);
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (val == dev_priv->cur_delay)
 		return;
 
-- 
1.7.10.4

Re: [PATCH 2/8] drm/i915: fixup up debugfs rps state handling

[Chris Wilson]

On Wed,  8 Aug 2012 23:35:34 +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 that 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 this disaster ...
> 
> v3: Also add a WARN_ON(mutex_is_locked) in set_rps to check the
> locking.
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (for v2)
> 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);

In debugfs, I advise that we use mutex_lock_interruptible().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] 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 that 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 this disaster ...

v3: Also add a WARN_ON(mutex_is_locked) in set_rps to check the
locking.

v4: Use mutex_lock_interruptible, suggested by Chris Wilson.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (for v2)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c |   47 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_pm.c     |    2 ++
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1312b79..31aa4b8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1708,10 +1708,18 @@ i915_max_freq_read(struct file *filp,
 	struct drm_device *dev = filp->private_data;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	char buf[80];
-	int len;
+	int len, ret;
+
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
 
 	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);
@@ -1728,7 +1736,10 @@ i915_max_freq_write(struct file *filp,
 	struct drm_device *dev = filp->private_data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	char buf[20];
-	int val = 1;
+	int val = 1, ret;
+
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
 
 	if (cnt > 0) {
 		if (cnt > sizeof(buf) - 1)
@@ -1743,12 +1754,17 @@ i915_max_freq_write(struct file *filp,
 
 	DRM_DEBUG_DRIVER("Manually setting max freq to %d\n", val);
 
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	/*
 	 * 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;
 }
@@ -1768,10 +1784,18 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
 	struct drm_device *dev = filp->private_data;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	char buf[80];
-	int len;
+	int len, ret;
+
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
 
 	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);
@@ -1786,7 +1810,10 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	struct drm_device *dev = filp->private_data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	char buf[20];
-	int val = 1;
+	int val = 1, ret;
+
+	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
+		return -ENODEV;
 
 	if (cnt > 0) {
 		if (cnt > sizeof(buf) - 1)
@@ -1801,12 +1828,17 @@ 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);
 
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	/*
 	 * 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 +1863,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 +1892,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 +2099,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 ad90579..dece479 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2300,6 +2300,8 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 limits = gen6_rps_limits(dev_priv, &val);
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	if (val == dev_priv->cur_delay)
 		return;
 
-- 
1.7.10.4

[PATCH 2/2] drm/i915: use mutex_lock_interruptible for debugfs files

[Daniel Vetter]

It's no fun if your shell hangs when the driver has gone on vacation
and you want to know why ...

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 31aa4b8..8acb8e9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -794,10 +794,14 @@ i915_error_state_write(struct file *filp,
 	struct seq_file *m = filp->private_data;
 	struct i915_error_state_file_priv *error_priv = m->private;
 	struct drm_device *dev = error_priv->dev;
+	int ret;
 
 	DRM_DEBUG_DRIVER("Resetting error state\n");
 
-	mutex_lock(&dev->struct_mutex);
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	i915_destroy_error_state(dev);
 	mutex_unlock(&dev->struct_mutex);
 
@@ -1467,8 +1471,12 @@ static int i915_swizzle_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;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
 
-	mutex_lock(&dev->struct_mutex);
 	seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
 		   swizzle_string(dev_priv->mm.bit_6_swizzle_x));
 	seq_printf(m, "bit6 swizzle for Y-tiling = %s\n",
@@ -1669,7 +1677,7 @@ i915_ring_stop_write(struct file *filp,
 	struct drm_device *dev = filp->private_data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	char buf[20];
-	int val = 0;
+	int val = 0, ret;
 
 	if (cnt > 0) {
 		if (cnt > sizeof(buf) - 1)
@@ -1684,7 +1692,10 @@ i915_ring_stop_write(struct file *filp,
 
 	DRM_DEBUG_DRIVER("Stopping rings 0x%08x\n", val);
 
-	mutex_lock(&dev->struct_mutex);
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	dev_priv->stop_rings = val;
 	mutex_unlock(&dev->struct_mutex);
 
@@ -1861,12 +1872,15 @@ i915_cache_sharing_read(struct file *filp,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	char buf[80];
 	u32 snpcr;
-	int len;
+	int len, ret;
 
 	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
 		return -ENODEV;
 
-	mutex_lock(&dev_priv->dev->struct_mutex);
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
 	mutex_unlock(&dev_priv->dev->struct_mutex);
 
@@ -1976,6 +1990,7 @@ static 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;
+	int ret;
 
 	if (INTEL_INFO(dev)->gen < 6)
 		return 0;
@@ -1987,7 +2002,10 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
 	 * hanging here is probably a minor inconvenience not to be seen my
 	 * almost every user.
 	 */
-	mutex_lock(&dev->struct_mutex);
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
 	gen6_gt_force_wake_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.7.10.4

Re: [PATCH 2/2] drm/i915: use mutex_lock_interruptible for debugfs files

[Ben Widawsky]

On Thu,  9 Aug 2012 15:07:02 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> It's no fun if your shell hangs when the driver has gone on vacation
> and you want to know why ...
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 31aa4b8..8acb8e9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -794,10 +794,14 @@ i915_error_state_write(struct file *filp,
>  	struct seq_file *m = filp->private_data;
>  	struct i915_error_state_file_priv *error_priv = m->private;
>  	struct drm_device *dev = error_priv->dev;
> +	int ret;
>  
>  	DRM_DEBUG_DRIVER("Resetting error state\n");
>  
> -	mutex_lock(&dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	i915_destroy_error_state(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -1467,8 +1471,12 @@ static int i915_swizzle_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;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
>  		   swizzle_string(dev_priv->mm.bit_6_swizzle_x));
>  	seq_printf(m, "bit6 swizzle for Y-tiling = %s\n",
> @@ -1669,7 +1677,7 @@ i915_ring_stop_write(struct file *filp,
>  	struct drm_device *dev = filp->private_data;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	char buf[20];
> -	int val = 0;
> +	int val = 0, ret;
>  
>  	if (cnt > 0) {
>  		if (cnt > sizeof(buf) - 1)
> @@ -1684,7 +1692,10 @@ i915_ring_stop_write(struct file *filp,
>  
>  	DRM_DEBUG_DRIVER("Stopping rings 0x%08x\n", val);
>  
> -	mutex_lock(&dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	dev_priv->stop_rings = val;
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -1861,12 +1872,15 @@ i915_cache_sharing_read(struct file *filp,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	char buf[80];
>  	u32 snpcr;
> -	int len;
> +	int len, ret;
>  
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev)))
>  		return -ENODEV;
>  
> -	mutex_lock(&dev_priv->dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	snpcr = I915_READ(GEN6_MBCUNIT_SNPCR);
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
>  
> @@ -1976,6 +1990,7 @@ static 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;
> +	int ret;
>  
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
> @@ -1987,7 +2002,10 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  	 * hanging here is probably a minor inconvenience not to be seen my
>  	 * almost every user.
>  	 */
> -	mutex_lock(&dev->struct_mutex);
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
>  	gen6_gt_force_wake_put(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
>  


If you feel like changing this intentionally non interruptible lock, you
should kill the comment about it too.



-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 3/8] 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.

Note that (cur|min|max)_delay need to be duplicated, because
they're also used by the ips code.

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

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
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      |   47 +++++++++++++++++-----------------
 6 files changed, 63 insertions(+), 49 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 d57ea16..54dc8d1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1589,7 +1589,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 0b2eb17..2d354e4 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 440c905..74c9a0e 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 2e1f28f..bddb290 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7218,7 +7218,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 dece479..3ab86bd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2277,9 +2277,10 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 *val)
 	u32 limits;
 
 	limits = 0;
-	if (*val >= dev_priv->max_delay)
-		*val = dev_priv->max_delay;
-	limits |= dev_priv->max_delay << 24;
+
+	if (*val >= dev_priv->rps.max_delay)
+		*val = dev_priv->rps.max_delay;
+	limits |= dev_priv->rps.max_delay << 24;
 
 	/* Only set the down limit when we've reached the lowest level to avoid
 	 * getting more interrupts, otherwise leave this clear. This prevents a
@@ -2287,9 +2288,9 @@ static u32 gen6_rps_limits(struct drm_i915_private *dev_priv, u8 *val)
 	 * the hw runs at the minimal clock before selecting the desired
 	 * frequency, if the down threshold expires in that window we will not
 	 * receive a down interrupt. */
-	if (*val <= dev_priv->min_delay) {
-		*val = dev_priv->min_delay;
-		limits |= dev_priv->min_delay << 16;
+	if (*val <= dev_priv->rps.min_delay) {
+		*val = dev_priv->rps.min_delay;
+		limits |= dev_priv->rps.min_delay << 16;
 	}
 
 	return limits;
@@ -2302,7 +2303,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (val == dev_priv->cur_delay)
+	if (val == dev_priv->rps.cur_delay)
 		return;
 
 	I915_WRITE(GEN6_RPNSWREQ,
@@ -2315,7 +2316,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)
@@ -2331,9 +2332,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));
 }
@@ -2402,9 +2403,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);
@@ -2457,8 +2458,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);
@@ -2503,7 +2504,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);
 	}
 
@@ -2511,10 +2512,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);
 
@@ -2546,9 +2547,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
@@ -2991,7 +2992,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);
 
-- 
1.7.10.4

[PATCH 4/8] 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 2d354e4..d6072c0 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 3ab86bd..44d58e7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3186,7 +3186,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 5/8] 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.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
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 74c9a0e..3e203da 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 6/8] 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 3e203da..d15ea50 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 44d58e7..5f9f93e 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);
 }
 
 /* There's a funny hw issue where the hw returns all 0 when reading from
@@ -2713,21 +2739,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;
@@ -2978,11 +2989,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)
@@ -3033,7 +3044,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;
@@ -3044,7 +3055,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;
 }
@@ -3060,7 +3071,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;
@@ -3071,7 +3082,7 @@ bool i915_gpu_raise(void)
 		dev_priv->max_delay--;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3088,7 +3099,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;
@@ -3099,7 +3110,7 @@ bool i915_gpu_lower(void)
 		dev_priv->max_delay++;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3117,7 +3128,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;
@@ -3126,7 +3137,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;
 }
@@ -3143,7 +3154,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;
@@ -3156,7 +3167,7 @@ bool i915_gpu_turbo_disable(void)
 		ret = false;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3184,18 +3195,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] 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 ...

v3: Resolve rebase conflict.

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 3e203da..d15ea50 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 212b28e..6d68a05 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);
 }
 
 /* There's a funny hw issue where the hw returns all 0 when reading from
@@ -2713,21 +2739,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;
@@ -2978,11 +2989,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)
@@ -3033,7 +3044,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;
@@ -3044,7 +3055,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;
 }
@@ -3060,7 +3071,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;
@@ -3071,7 +3082,7 @@ bool i915_gpu_raise(void)
 		dev_priv->max_delay--;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3088,7 +3099,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;
@@ -3099,7 +3110,7 @@ bool i915_gpu_lower(void)
 		dev_priv->max_delay++;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3117,7 +3128,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;
@@ -3126,7 +3137,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;
 }
@@ -3143,7 +3154,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;
@@ -3156,7 +3167,7 @@ bool i915_gpu_turbo_disable(void)
 		ret = false;
 
 out_unlock:
-	spin_unlock(&mchdev_lock);
+	spin_unlock_irq(&mchdev_lock);
 
 	return ret;
 }
@@ -3186,18 +3197,18 @@ 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);
+	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 7/8] 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 |   86 ++++++++++++++++++---------------------
 3 files changed, 70 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d6072c0..5225784 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 d15ea50..135a84d 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 5f9f93e..eff0753 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);
@@ -2748,7 +2742,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
@@ -2756,7 +2750,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);
@@ -2765,16 +2759,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;
@@ -2785,10 +2779,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;
 }
@@ -2959,7 +2953,7 @@ static 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;
@@ -2968,20 +2962,20 @@ static 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)
@@ -3023,14 +3017,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;
 }
 
 /**
@@ -3078,8 +3072,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);
@@ -3106,8 +3100,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);
@@ -3161,9 +3155,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:
@@ -3276,7 +3270,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)
-- 
1.7.10.4

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

[Daniel Vetter]

On Wed, Aug 08, 2012 at 11:35:39PM +0200, Daniel Vetter 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've just read through the ips code again and got annoyed how things are
splattered all over. Patch applied.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h |   36 ++++++++--------
>  drivers/gpu/drm/i915/i915_irq.c |   20 ++++-----
>  drivers/gpu/drm/i915/intel_pm.c |   86 ++++++++++++++++++---------------------
>  3 files changed, 70 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d6072c0..5225784 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 d15ea50..135a84d 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 5f9f93e..eff0753 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);
> @@ -2748,7 +2742,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
> @@ -2756,7 +2750,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);
> @@ -2765,16 +2759,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;
> @@ -2785,10 +2779,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;
>  }
> @@ -2959,7 +2953,7 @@ static 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;
> @@ -2968,20 +2962,20 @@ static 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)
> @@ -3023,14 +3017,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;
>  }
>  
>  /**
> @@ -3078,8 +3072,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);
> @@ -3106,8 +3100,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);
> @@ -3161,9 +3155,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:
> @@ -3276,7 +3270,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)
> -- 
> 1.7.10.4
> 

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

[PATCH 8/8] 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 eff0753..a87c625 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2361,31 +2361,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 8/8] drm/i915: enable rc6 on ilk again

[Ben Widawsky]

On Wed,  8 Aug 2012 23:35:40 +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>
Acked-by: Ben Widawsky <ben@bwidawsk.net>

After discussing with Daniel on IRC a bit, I've decided it would take
too much time to actually verify the theory. I am acking on the basis
that I also couldn't find the bug report via google, rc6 has worked for
me on ilk forever, and the savings are too big not to try again.

> ---
>  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 eff0753..a87c625 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2361,31 +2361,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);
>  }



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 0/8] rps locking fixes v2

[Chris Wilson]

On Wed,  8 Aug 2012 23:35:32 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hi all,
> 
> Essentially just rebase, with Ben's review comments taking into account and one
> WARN_ON(mutex_is_locked) moved around a bit.
> 
> Review&testing highly welcome.
> 
> Cheers, Daniel
> 
> Daniel Vetter (8):
>   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
1-7 look like good mechanical changes to improve readability and remove
some superstition (multiple igfx perchance). Only the single comment,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

>   drm/i915: enable rc6 on ilk again

Optimist.

I think you need an ack from the guilty parties if they believe that
the code is in good order first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/8] rps locking fixes v2

[Daniel Vetter]

On Thu, Aug 09, 2012 at 10:43:53AM +0100, Chris Wilson wrote:
> On Wed,  8 Aug 2012 23:35:32 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hi all,
> > 
> > Essentially just rebase, with Ben's review comments taking into account and one
> > WARN_ON(mutex_is_locked) moved around a bit.
> > 
> > Review&testing highly welcome.
> > 
> > Cheers, Daniel
> > 
> > Daniel Vetter (8):
> >   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
> 1-7 look like good mechanical changes to improve readability and remove
> some superstition (multiple igfx perchance). Only the single comment,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> >   drm/i915: enable rc6 on ilk again
> 
> Optimist.
> 
> I think you need an ack from the guilty parties if they believe that
> the code is in good order first.

Well, I've dug around in the git history, but could only find "we have
still reports", no link nor mail address :( And my google-fu failed me,
too. But the locking fixes clearly close a hole - the pcu mbox needs
multiple writes to send out a message, and I can easily believe that we
blow up the hw if we race with different messages. So I guess it can't
hurt to retest, but since I couldn't get hold of any of the reporters I've
figured to be the optimist and just try ;-)

I'll poke Ben to at least ack it, since he last changed the ilk rps code,
he owns it ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 0/8] rps locking fixes v2

[Lespiau, Damien]

On Wed, Aug 8, 2012 at 10:35 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Review&testing highly welcome.

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

FWIW, I'll be running my main dev machine (ILK) with RC6 enabled from
now one and see what happens.

-- 
Damien

Re: [PATCH 0/8] rps locking fixes v2

[Daniel Vetter]

On Thu, Aug 09, 2012 at 03:58:46PM +0100, Lespiau, Damien wrote:
> On Wed, Aug 8, 2012 at 10:35 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Review&testing highly welcome.
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Ok, I've slurped in all the patches for -next, thanks for comments&review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48