From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] rpm Date: Wed, 10 Sep 2014 20:09:14 +0300 Message-ID: <20140910170914.GX4193@intel.com> References: <1410367395-25093-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id DFE7689F33 for ; Wed, 10 Sep 2014 10:09:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote: > 2014-09-10 13:43 GMT-03:00 Chris Wilson : > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ > > drivers/gpu/drm/i915/intel_display.c | 20 ++------------ > > drivers/gpu/drm/i915/intel_lrc.c | 21 ++------------- > > drivers/gpu/drm/i915/intel_uncore.c | 52 +++++++++++++++-------------= -------- > > 4 files changed, 27 insertions(+), 68 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915= /i915_debugfs.c > > index 5f35048..a72d8b8 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inod= e, struct file *file) > > if (INTEL_INFO(dev)->gen < 6) > > return 0; > > > > + intel_runtime_pm_get(dev_priv); > > gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > > > return 0; > > @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *i= node, struct file *file) > > return 0; > > > > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > > + intel_runtime_pm_put(dev_priv); > > > > return 0; > > } > = > Just a minor comment on the chunk above. I didn't look at the rest of the= patch. > = > We used to have exactly the code that you rewrote, but we decided to > remove it because, without it, we can test that gen6_gt_force_wake_get > actually wakes up the HW and keeps it awake until someone calls > gen6_gt_force_wake_put. > = > If we add these get/put calls above, we won't really detect any > problems related with the actual gen6_gt_force_wake_get/put functions, > so we may hide problems. Well, I think the patch is a great. With the rpm stuff killed from forcewake handling we could actually use the delayed forcewake put in the register access macros, which is where it might actually do some good. IIRC that was even the original intention when the timer got added. In the current form I'm pretty sure the timer is practically useless. > = > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index 794ad8f..fafd202 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_pri= vate *dev_priv, > > static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) > > { > > uint32_t val; > > - unsigned long irqflags; > > > > val =3D I915_READ(LCPLL_CTL); > > > > @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_pr= ivate *dev_priv) > > /* > > * Make sure we're not on PC8 state before disabling PC8, other= wise > > * we'll hang the machine. To prevent PC8 state, just enable fo= rce_wake. > > - * > > - * The other problem is that hsw_restore_lcpll() is called as p= art of > > - * the runtime PM resume sequence, so we can't just call > > - * gen6_gt_force_wake_get() because that function calls > > - * intel_runtime_pm_get(), and we can't change the runtime PM r= efcount > > - * while we are on the resume sequence. So to solve this proble= m we have > > - * to call special forcewake code that doesn't touch runtime PM= and > > - * doesn't enable the forcewake delayed work. > > */ > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - if (dev_priv->uncore.forcewake_count++ =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWA= KE_ALL); > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > > > if (val & LCPLL_POWER_DOWN_ALLOW) { > > val &=3D ~LCPLL_POWER_DOWN_ALLOW; > > @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_pr= ivate *dev_priv) > > DRM_ERROR("Switching back to LCPLL failed\n"); > > } > > > > - /* See the big comment above. */ > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - if (--dev_priv->uncore.forcewake_count =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWA= KE_ALL); > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > > } > > > > /* > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/in= tel_lrc.c > > index 6f1dd00..aeaa1bc 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engi= ne_cs *engine, > > struct drm_i915_private *dev_priv =3D engine->i915; > > uint64_t tmp; > > uint32_t desc[4]; > > - unsigned long flags; > > > > /* XXX: You must always write both descriptors in the order bel= ow. */ > > > > @@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_eng= ine_cs *engine, > > desc[1] =3D upper_32_bits(tmp); > > desc[0] =3D lower_32_bits(tmp); > > > > - /* Set Force Wakeup bit to prevent GT from entering C6 while EL= SP writes > > - * are in progress. > > - * > > - * The other problem is that we can't just call gen6_gt_force_w= ake_get() > > - * because that function calls intel_runtime_pm_get(), which mi= ght sleep. > > - * Instead, we do the runtime_pm_get/put when creating/destroyi= ng requests. > > - */ > > - spin_lock_irqsave(&dev_priv->uncore.lock, flags); > > - if (dev_priv->uncore.forcewake_count++ =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWA= KE_ALL); > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > > - > > + gen6_gt_force_wake_get(dev_priv, engine->power_domains); > > I915_WRITE(RING_ELSP(engine), desc[1]); > > I915_WRITE(RING_ELSP(engine), desc[0]); > > I915_WRITE(RING_ELSP(engine), desc[3]); > > @@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_eng= ine_cs *engine, > > > > /* ELSP is a wo register, so use another nearby reg for posting= instead */ > > POSTING_READ(RING_EXECLIST_STATUS(engine)); > > - > > - /* Release Force Wakeup (see the big comment above). */ > > - spin_lock_irqsave(&dev_priv->uncore.lock, flags); > > - if (--dev_priv->uncore.forcewake_count =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWA= KE_ALL); > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > > + gen6_gt_force_wake_put(dev_priv, engine->power_domains); > > } > > > > static u16 next_tag(struct intel_engine_cs *engine) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915= /intel_uncore.c > > index c99d5ef..3b3d3e0 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -24,6 +24,8 @@ > > #include "i915_drv.h" > > #include "intel_drv.h" > > > > +#include > > + > > #define FORCEWAKE_ACK_TIMEOUT_MS 2 > > > > #define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs += (reg__)) > > @@ -258,10 +260,6 @@ static void __vlv_force_wake_put(struct drm_i915_p= rivate *dev_priv, > > > > static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int = fw_engine) > > { > > - unsigned long irqflags; > > - > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - > > if (fw_engine & FORCEWAKE_RENDER && > > dev_priv->uncore.fw_rendercount++ !=3D 0) > > fw_engine &=3D ~FORCEWAKE_RENDER; > > @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_pr= ivate *dev_priv, int fw_engine) > > > > if (fw_engine) > > dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engi= ne); > > - > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > } > > > > static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int = fw_engine) > > { > > - unsigned long irqflags; > > - > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - > > if (fw_engine & FORCEWAKE_RENDER) { > > WARN_ON(!dev_priv->uncore.fw_rendercount); > > if (--dev_priv->uncore.fw_rendercount !=3D 0) > > @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_priv= ate *dev_priv, int fw_engine) > > > > if (fw_engine) > > dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engi= ne); > > - > > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > } > > > > static void gen6_force_wake_timer(unsigned long arg) > > @@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_priva= te *dev_priv, int fw_engine) > > if (!dev_priv->uncore.funcs.force_wake_get) > > return; > > > > - intel_runtime_pm_get(dev_priv); > > - > > - /* Redirect to VLV specific routine */ > > - if (IS_VALLEYVIEW(dev_priv->dev)) > > - return vlv_force_wake_get(dev_priv, fw_engine); > > + WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev)); > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - if (dev_priv->uncore.forcewake_count++ =3D=3D 0) > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWA= KE_ALL); > > + > > + /* Redirect to VLV specific routine */ > > + if (IS_VALLEYVIEW(dev_priv->dev)) { > > + vlv_force_wake_get(dev_priv, fw_engine); > > + } else { > > + if (dev_priv->uncore.forcewake_count++ =3D=3D 0) > > + dev_priv->uncore.funcs.force_wake_get(dev_priv, > > + FORCEWAKE= _ALL); > > + } > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > } > > > > @@ -428,25 +421,22 @@ void gen6_gt_force_wake_put(struct drm_i915_priva= te *dev_priv, int fw_engine) > > if (!dev_priv->uncore.funcs.force_wake_put) > > return; > > > > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > + > > /* Redirect to VLV specific routine */ > > if (IS_VALLEYVIEW(dev_priv->dev)) { > > vlv_force_wake_put(dev_priv, fw_engine); > > - goto out; > > - } > > - > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - WARN_ON(!dev_priv->uncore.forcewake_count); > > - > > - if (--dev_priv->uncore.forcewake_count =3D=3D 0) { > > - dev_priv->uncore.forcewake_count++; > > - mod_timer_pinned(&dev_priv->uncore.force_wake_timer, > > - jiffies + 1); > > + } else { > > + WARN_ON(!dev_priv->uncore.forcewake_count); > > + if (--dev_priv->uncore.forcewake_count =3D=3D 0) { > > + dev_priv->uncore.forcewake_count++; > > + mod_timer_pinned(&dev_priv->uncore.force_wake_t= imer, > > + jiffies + 1); > > + } > > } > > > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > > > -out: > > - intel_runtime_pm_put(dev_priv); > > } > > > > void assert_force_wake_inactive(struct drm_i915_private *dev_priv) > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > = > = > -- = > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC