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

[Ben Widawsky]

On Tue, 24 Jul 2012 23:33:43 +0200
Daniel Vetter <daniel.vetter at 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 at ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |   50 ++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b6e8fbf..21a0088 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2693,6 +2693,21 @@ static const struct cparams {
>  	{ 0, 800, 231, 23784 },
>  };
>  
> +/**
> + * Lock protecting IPS related data structures
> + *   - i915_mch_dev
> + *   - dev_priv->max_delay
> + *   - dev_priv->min_delay
> + *   - dev_priv->fmax
> + *   - dev_priv->gpu_busy
> + *   - dev_priv->gfx_power
> + */
> +static DEFINE_SPINLOCK(mchdev_lock);
> +
> +/* Global for IPS driver to get at the current i915 device. Protected by
> + * mchdev_lock. */
> +static struct drm_i915_private *i915_mch_dev;
> +
>  unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
>  {
>  	u64 total_count, diff, ret;
> @@ -2700,6 +2715,8 @@ unsigned long i915_chipset_val(struct drm_i915_private *dev_priv)
>  	unsigned long now = jiffies_to_msecs(jiffies), diff1;
>  	int i;
>  
> +	assert_spin_locked(&mchdev_lock);
> +
>  	diff1 = now - dev_priv->last_time1;
>  
>  	/* Prevent division-by-zero if we are asking too fast.
> @@ -2901,15 +2918,14 @@ static u16 pvid_to_extvid(struct drm_i915_private *dev_priv, u8 pxvid)
>  		return v_table[pxvid].vd;
>  }
>  
> -void i915_update_gfx_val(struct drm_i915_private *dev_priv)
> +void __i915_update_gfx_val(struct drm_i915_private *dev_priv)

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

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

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

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



-- 
Ben Widawsky, Intel Open Source Technology Center