From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 2/9] drm/i915: properly guard ilk ips state Date: Wed, 25 Jul 2012 11:09:07 -0700 Message-ID: <20120725110907.637fb03e@bwidawsk.net> References: <1343165630-21604-1-git-send-email-daniel.vetter@ffwll.ch> <1343165630-21604-3-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 9F89CA0E35 for ; Wed, 25 Jul 2012 11:09:14 -0700 (PDT) In-Reply-To: <1343165630-21604-3-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Tue, 24 Jul 2012 23:33:43 +0200 Daniel Vetter 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 > --- > 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