Re: [PATCH] rpm

[Paulo Zanoni <przanoni@gmail.com>]

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



-- 
Paulo Zanoni