Re: [PATCH 2/8] drm/i915: Assert that runtime pm is active on user fw access

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Monday 08 December 2014 11:57 PM, Mika Kuoppala wrote:
From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>

On user forcewake access, assert that runtime pm reference is held.
Fix and cleanup the callsites accordingly.

Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
---
  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)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux