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

Message ID 1343165630-21604-3-git-send-email-daniel.vetter@ffwll.ch
State New, archived
Headers show

Commit Message

Daniel Vetter July 24, 2012, 9:33 p.m. UTC
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(-)

Comments

Ben Widawsky July 25, 2012, 6:09 p.m. UTC | #1
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
>   *

Patch
diff mbox

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
  *