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:15:47 +0300 Message-ID: <20140910171546.GY4193@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 508F76E394 for ; Wed, 10 Sep 2014 10:15:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1410367395-25093-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote: > --- > 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/i= 915_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 *inode,= 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 *ino= de, struct file *file) > return 0; > = > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > + intel_runtime_pm_put(dev_priv); > = > return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= 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_priva= te *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_priv= ate *dev_priv) > /* > * Make sure we're not on PC8 state before disabling PC8, otherwise > * we'll hang the machine. To prevent PC8 state, just enable force_wake. > - * > - * The other problem is that hsw_restore_lcpll() is called as part 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 refcount > - * while we are on the resume sequence. So to solve this problem 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, FORCEWAKE_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_priv= ate *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, FORCEWAKE_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/inte= l_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_engine= _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 below. */ > = > @@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engin= e_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 ELSP writes > - * are in progress. > - * > - * The other problem is that we can't just call gen6_gt_force_wake_get() > - * because that function calls intel_runtime_pm_get(), which might slee= p. > - * Instead, we do the runtime_pm_get/put when creating/destroying reque= sts. > - */ > - 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, FORCEWAKE_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_engin= e_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, FORCEWAKE_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/i= ntel_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_pri= vate *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_priv= ate *dev_priv, int fw_engine) > = > if (fw_engine) > dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine); > - > - 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_privat= e *dev_priv, int fw_engine) > = > if (fw_engine) > dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine); > - > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > = > static void gen6_force_wake_timer(unsigned long arg) Looks like you forgot to kill the rpm_put() from the timer. And then we also need to make sure the timer is approriately cancelled when runtime suspending the device. -- = Ville Syrj=E4l=E4 Intel OTC