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
