[Intel-gfx] [PATCH 2/8] drm/i915: Assert that runtime pm is active on user fw access
Deepak S
deepak.s at linux.intel.com
Fri Dec 12 03:39:26 PST 2014
On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
>
> On user forcewake access, assert that runtime pm reference is held.
> Fix and cleanup the callsites accordingly.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +
> drivers/gpu/drm/i915/intel_display.c | 19 +--------
> drivers/gpu/drm/i915/intel_lrc.c | 53 ++-----------------------
> drivers/gpu/drm/i915/intel_uncore.c | 76 ++++++++++++------------------------
> 4 files changed, 31 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d0e445e..e142629 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4250,6 +4250,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;
> @@ -4264,6 +4265,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 d5153a4..86c2885 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7862,19 +7862,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_irq(&dev_priv->uncore.lock);
> - if (dev_priv->uncore.forcewake_count++ == 0)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> - spin_unlock_irq(&dev_priv->uncore.lock);
> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>
> if (val & LCPLL_POWER_DOWN_ALLOW) {
> val &= ~LCPLL_POWER_DOWN_ALLOW;
> @@ -7904,11 +7893,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_irq(&dev_priv->uncore.lock);
> - if (--dev_priv->uncore.forcewake_count == 0)
> - dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> - spin_unlock_irq(&dev_priv->uncore.lock);
> + 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 a82020e..fcb5140 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -284,7 +284,6 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> struct drm_i915_private *dev_priv = dev->dev_private;
> uint64_t temp = 0;
> uint32_t desc[4];
> - unsigned long flags;
>
> /* XXX: You must always write both descriptors in the order below. */
> if (ctx_obj1)
> @@ -298,63 +297,17 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> desc[3] = (u32)(temp >> 32);
> desc[2] = (u32)temp;
>
> - /* 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 (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
> - if (dev_priv->uncore.fw_rendercount++ == 0)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv,
> - FORCEWAKE_RENDER);
> - if (dev_priv->uncore.fw_mediacount++ == 0)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv,
> - FORCEWAKE_MEDIA);
> - if (INTEL_INFO(dev)->gen >= 9) {
> - if (dev_priv->uncore.fw_blittercount++ == 0)
> - dev_priv->uncore.funcs.force_wake_get(dev_priv,
> - FORCEWAKE_BLITTER);
> - }
> - } 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, flags);
> -
> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> I915_WRITE(RING_ELSP(ring), desc[1]);
> I915_WRITE(RING_ELSP(ring), desc[0]);
> I915_WRITE(RING_ELSP(ring), desc[3]);
> +
> /* The context is automatically loaded after the following */
> I915_WRITE(RING_ELSP(ring), desc[2]);
>
> /* ELSP is a wo register, so use another nearby reg for posting instead */
> POSTING_READ(RING_EXECLIST_STATUS(ring));
> -
> - /* Release Force Wakeup (see the big comment above). */
> - spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> - if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
> - if (--dev_priv->uncore.fw_rendercount == 0)
> - dev_priv->uncore.funcs.force_wake_put(dev_priv,
> - FORCEWAKE_RENDER);
> - if (--dev_priv->uncore.fw_mediacount == 0)
> - dev_priv->uncore.funcs.force_wake_put(dev_priv,
> - FORCEWAKE_MEDIA);
> - if (INTEL_INFO(dev)->gen >= 9) {
> - if (--dev_priv->uncore.fw_blittercount == 0)
> - dev_priv->uncore.funcs.force_wake_put(dev_priv,
> - FORCEWAKE_BLITTER);
> - }
> - } else {
> - 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, FORCEWAKE_ALL);
> }
>
> static int execlists_update_context(struct drm_i915_gem_object *ctx_obj,
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 38ac389..a1ceb92 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__))
> @@ -247,10 +249,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;
> @@ -260,16 +258,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)
> @@ -284,8 +276,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 __gen9_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
> @@ -380,10 +370,6 @@ __gen9_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> static void
> gen9_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> {
> - unsigned long irqflags;
> -
> - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
> if (FORCEWAKE_RENDER & fw_engine) {
> if (dev_priv->uncore.fw_rendercount++ == 0)
> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> @@ -401,17 +387,11 @@ gen9_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> FORCEWAKE_BLITTER);
> }
> -
> - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
>
> static void
> gen9_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> {
> - unsigned long irqflags;
> -
> - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
> if (FORCEWAKE_RENDER & fw_engine) {
> WARN_ON(dev_priv->uncore.fw_rendercount == 0);
> if (--dev_priv->uncore.fw_rendercount == 0)
> @@ -432,8 +412,6 @@ gen9_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> dev_priv->uncore.funcs.force_wake_put(dev_priv,
> FORCEWAKE_BLITTER);
> }
> -
> - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }
>
> static void gen6_force_wake_timer(unsigned long arg)
> @@ -564,17 +542,20 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>
> intel_runtime_pm_get(dev_priv);
>
I think we need to remove "intel_runtime_pm_get" here?
I see runtime_put is removed from "gen6_gt_force_wake_put"
> - /* Redirect to Gen9 specific routine */
> - if (IS_GEN9(dev_priv->dev))
> - return gen9_force_wake_get(dev_priv, fw_engine);
> -
> - /* 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);
> +
> + if (IS_GEN9(dev_priv->dev)) {
> + gen9_force_wake_get(dev_priv, fw_engine);
> + } else 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);
> }
>
> @@ -588,31 +569,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;
>
> - /* Redirect to Gen9 specific routine */
> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +
> if (IS_GEN9(dev_priv->dev)) {
> gen9_force_wake_put(dev_priv, fw_engine);
> - goto out;
> - }
> -
> - /* Redirect to VLV specific routine */
> - if (IS_VALLEYVIEW(dev_priv->dev)) {
> + } else 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)
More information about the Intel-gfx
mailing list