| Message ID | 1410367395-25093-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > --- > 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 *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 *inode, 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. > 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_private *dev_priv, > static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) > { > uint32_t val; > - unsigned long irqflags; > > val = I915_READ(LCPLL_CTL); > > @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *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++ == 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 &= ~LCPLL_POWER_DOWN_ALLOW; > @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *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 == 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/intel_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 = 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_engine_cs *engine, > desc[1] = upper_32_bits(tmp); > desc[0] = 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 sleep. > - * Instead, we do the runtime_pm_get/put when creating/destroying requests. > - */ > - spin_lock_irqsave(&dev_priv->uncore.lock, flags); > - if (dev_priv->uncore.forcewake_count++ == 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_engine_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 == 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/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 <linux/pm_runtime.h> > + > #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_private *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++ != 0) > fw_engine &= ~FORCEWAKE_RENDER; > @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *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 != 0) > @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *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) > @@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *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++ == 0) > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_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++ == 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_private *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 == 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 == 0) { > + dev_priv->uncore.forcewake_count++; > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, > + 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
On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote: > 2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > --- > > 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 *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 *inode, 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. But why? gen6_gt_force_wake_get() should never be called outside of a rpm context - it is lowlevel register access. > 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. See above. That's how the design got broken in the first place and we have very lowlevel code being copied and pasted throughout the driver. -Chris
On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote: > 2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > --- > > 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 *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 *inode, 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/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_private *dev_priv, > > static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) > > { > > uint32_t val; > > - unsigned long irqflags; > > > > val = I915_READ(LCPLL_CTL); > > > > @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *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++ == 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 &= ~LCPLL_POWER_DOWN_ALLOW; > > @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *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 == 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/intel_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 = 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_engine_cs *engine, > > desc[1] = upper_32_bits(tmp); > > desc[0] = 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 sleep. > > - * Instead, we do the runtime_pm_get/put when creating/destroying requests. > > - */ > > - spin_lock_irqsave(&dev_priv->uncore.lock, flags); > > - if (dev_priv->uncore.forcewake_count++ == 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_engine_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 == 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/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 <linux/pm_runtime.h> > > + > > #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_private *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++ != 0) > > fw_engine &= ~FORCEWAKE_RENDER; > > @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *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 != 0) > > @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *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) > > @@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *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++ == 0) > > - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_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++ == 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_private *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 == 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 == 0) { > > + dev_priv->uncore.forcewake_count++; > > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, > > + 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
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/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 *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 *inode, 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_private *dev_priv, > static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) > { > uint32_t val; > - unsigned long irqflags; > > val = I915_READ(LCPLL_CTL); > > @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *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++ == 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 &= ~LCPLL_POWER_DOWN_ALLOW; > @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *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 == 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/intel_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 = 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_engine_cs *engine, > desc[1] = upper_32_bits(tmp); > desc[0] = 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 sleep. > - * Instead, we do the runtime_pm_get/put when creating/destroying requests. > - */ > - spin_lock_irqsave(&dev_priv->uncore.lock, flags); > - if (dev_priv->uncore.forcewake_count++ == 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_engine_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 == 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/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 <linux/pm_runtime.h> > + > #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_private *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++ != 0) > fw_engine &= ~FORCEWAKE_RENDER; > @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *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 != 0) > @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *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.
On Wed, Sep 10, 2014 at 08:15:47PM +0300, Ville Syrjälä wrote: > On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote: > > 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. Or maybe I have that already fixed in my tree so you don't see it in this patch ;-) -Chris
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 *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 *inode, 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_private *dev_priv, static void hsw_restore_lcpll(struct drm_i915_private *dev_priv) { uint32_t val; - unsigned long irqflags; val = I915_READ(LCPLL_CTL); @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *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++ == 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 &= ~LCPLL_POWER_DOWN_ALLOW; @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *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 == 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/intel_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 = 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_engine_cs *engine, desc[1] = upper_32_bits(tmp); desc[0] = 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 sleep. - * Instead, we do the runtime_pm_get/put when creating/destroying requests. - */ - spin_lock_irqsave(&dev_priv->uncore.lock, flags); - if (dev_priv->uncore.forcewake_count++ == 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_engine_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 == 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/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 <linux/pm_runtime.h> + #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_private *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++ != 0) fw_engine &= ~FORCEWAKE_RENDER; @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *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 != 0) @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *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) @@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *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++ == 0) - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_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++ == 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_private *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 == 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 == 0) { + dev_priv->uncore.forcewake_count++; + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, + 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)