On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote:
> 2014-09-10 13:43 GMT-03:00 Chris Wilson <ch...@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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
