[PATCH 0/5] bdw forcewake fixes

[PATCH 0/5] bdw forcewake fixes

[mika.kuoppala]

From: Mika Kuoppala <mika.kuoppala@intel.com>

Tweaked 2/5 commit message a bit to remove mention
of hang, as we are not sure about the root cause.

4/5 is hopefully something that Ben was looking after in his
review.

Thanks for Ben and Ville for looking at these.

-Mika

Mika Kuoppala (5):
  drm/i915: Fix forcewake counts for gen8
  drm/i915: Do forcewake reset on gen8
  drm/i915: Don't access fifodbg registers on gen8
  drm/i915: Always set fifo count to zero in gen6_reset
  drm/i915: No need to put forcewake after a reset

 drivers/gpu/drm/i915/intel_uncore.c |   44 +++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 20 deletions(-)

-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 1/5] drm/i915: Fix forcewake counts for gen8

[mika.kuoppala]

From: Mika Kuoppala <mika.kuoppala@intel.com>

Sometimes generic driver code gets forcewake explicitly by
gen6_gt_force_wake_get(), which check forcewake_count before accessing
hardware. However the register access with gen8_write function access
low level hw accessors directly, ignoring the forcewake_count. This
leads to nested forcewake get from hardware, in ring init and possibly
elsewhere, causing forcewake ack clear errors and/or hangs.

Fix this by checking the forcewake count also in gen8_write

v2: Read side doesn't care about shadowed registers,
    Remove __needs_put funkiness from gen8_write. (Ville)
    Improved commit message.

References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_uncore.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c628414..d1e9d63 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -634,16 +634,17 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
 #define __gen8_write(x) \
 static void \
 gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
-	bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
 	REG_WRITE_HEADER; \
-	if (__needs_put) { \
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
-							FORCEWAKE_ALL); \
-	} \
-	__raw_i915_write##x(dev_priv, reg, val); \
-	if (__needs_put) { \
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, \
-							FORCEWAKE_ALL); \
+	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
+		if (dev_priv->uncore.forcewake_count == 0) \
+			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
+							      FORCEWAKE_ALL); \
+		__raw_i915_write##x(dev_priv, reg, val); \
+		if (dev_priv->uncore.forcewake_count == 0) \
+			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
+							      FORCEWAKE_ALL); \
+	} else { \
+		__raw_i915_write##x(dev_priv, reg, val); \
 	} \
 	REG_WRITE_FOOTER; \
 }
-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Do forcewake reset on gen8

[mika.kuoppala]

From: Mika Kuoppala <mika.kuoppala@intel.com>

When we get control from BIOS there might be mt forcewake
bits already set. This causes us to do double mt get
without proper clear/ack sequence.

Fix this by clearing mt forcewake register on init,
like we do with older gens.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_uncore.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index d1e9d63..b2a6295 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -305,13 +305,13 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (IS_VALLEYVIEW(dev)) {
+	if (IS_VALLEYVIEW(dev))
 		vlv_force_wake_reset(dev_priv);
-	} else if (INTEL_INFO(dev)->gen >= 6) {
+	else if (IS_GEN6(dev) || IS_GEN7(dev))
 		__gen6_gt_force_wake_reset(dev_priv);
-		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
-			__gen6_gt_force_wake_mt_reset(dev_priv);
-	}
+
+	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_GEN8(dev))
+		__gen6_gt_force_wake_mt_reset(dev_priv);
 }
 
 void intel_uncore_early_sanitize(struct drm_device *dev)
-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 3/5] drm/i915: Don't access fifodbg registers on gen8

[mika.kuoppala]

From: Mika Kuoppala <mika.kuoppala@intel.com>

as they don't exists.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_uncore.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b2a6295..5ce8282 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -143,7 +143,9 @@ static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
 			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
 	/* something from same cacheline, but !FORCEWAKE_MT */
 	__raw_posting_read(dev_priv, ECOBUS);
-	gen6_gt_check_fifodbg(dev_priv);
+
+	if (IS_GEN6(dev_priv->dev) || IS_GEN7(dev_priv->dev))
+		gen6_gt_check_fifodbg(dev_priv);
 }
 
 static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
@@ -974,7 +976,10 @@ static int gen6_do_reset(struct drm_device *dev)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
 
 	/* Restore fifo count */
-	dev_priv->uncore.fifo_count = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
+	if (IS_GEN6(dev) || IS_GEN7(dev))
+		dev_priv->uncore.fifo_count =
+			__raw_i915_read32(dev_priv, GTFIFOCTL) &
+			GT_FIFO_FREE_ENTRIES_MASK;
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 	return ret;
-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 4/5] drm/i915: Always set fifo count to zero in gen6_reset

[mika.kuoppala]

From: Mika Kuoppala <mika.kuoppala@intel.com>

There should not be a case where fifo count is other
than zero after a successful reset. Always set
count to zero, but be paranoid enough to warn.

Suggested-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 5ce8282..ae068c1 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -975,11 +975,11 @@ static int gen6_do_reset(struct drm_device *dev)
 	else
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
 
-	/* Restore fifo count */
 	if (IS_GEN6(dev) || IS_GEN7(dev))
-		dev_priv->uncore.fifo_count =
-			__raw_i915_read32(dev_priv, GTFIFOCTL) &
-			GT_FIFO_FREE_ENTRIES_MASK;
+		WARN_ON((__raw_i915_read32(dev_priv, GTFIFOCTL) &
+			 GT_FIFO_FREE_ENTRIES_MASK) != 0);
+
+	dev_priv->uncore.fifo_count = 0;
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 	return ret;
-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 5/5] drm/i915: No need to put forcewake after a reset

[mika.kuoppala]

From: Mika Kuoppala <mika.kuoppala@intel.com>

As we now have intel_uncore_forcewake_reset() no need
to do explicit put after reset.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index ae068c1..176b678 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -969,11 +969,9 @@ static int gen6_do_reset(struct drm_device *dev)
 
 	intel_uncore_forcewake_reset(dev);
 
-	/* If reset with a user forcewake, try to restore, otherwise turn it off */
+	/* If reset with a user forcewake, try to restore */
 	if (dev_priv->uncore.forcewake_count)
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
-	else
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
 
 	if (IS_GEN6(dev) || IS_GEN7(dev))
 		WARN_ON((__raw_i915_read32(dev_priv, GTFIFOCTL) &
-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 3/5] drm/i915: Don't access fifodbg registers on gen8

[Chris Wilson]

On Fri, Feb 21, 2014 at 05:32:01PM +0200, mika.kuoppala@intel.com wrote:
> From: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> as they don't exists.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b2a6295..5ce8282 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -143,7 +143,9 @@ static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
>  			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
>  	/* something from same cacheline, but !FORCEWAKE_MT */
>  	__raw_posting_read(dev_priv, ECOBUS);
> -	gen6_gt_check_fifodbg(dev_priv);
> +
> +	if (IS_GEN6(dev_priv->dev) || IS_GEN7(dev_priv->dev))
> +		gen6_gt_check_fifodbg(dev_priv);

Don't let the function name fool you, this cannot be called on gen6.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/5] drm/i915: Don't access fifodbg registers on gen8

[Mika Kuoppala]

as they don't exists.

v2: rename gen6_*_mt_* to gen7_*_mt_* as they never get called
    with gen6 (Chris)

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (v1)
---
 drivers/gpu/drm/i915/intel_uncore.c |   29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b2a6295..d94e587 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -83,14 +83,14 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
 	__gen6_gt_wait_for_thread_c0(dev_priv);
 }
 
-static void __gen6_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
+static void __gen7_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
 {
 	__raw_i915_write32(dev_priv, FORCEWAKE_MT, _MASKED_BIT_DISABLE(0xffff));
 	/* something from same cacheline, but !FORCEWAKE_MT */
 	__raw_posting_read(dev_priv, ECOBUS);
 }
 
-static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv,
+static void __gen7_gt_force_wake_mt_get(struct drm_i915_private *dev_priv,
 							int fw_engine)
 {
 	u32 forcewake_ack;
@@ -136,14 +136,16 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
 	gen6_gt_check_fifodbg(dev_priv);
 }
 
-static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
+static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
 							int fw_engine)
 {
 	__raw_i915_write32(dev_priv, FORCEWAKE_MT,
 			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
 	/* something from same cacheline, but !FORCEWAKE_MT */
 	__raw_posting_read(dev_priv, ECOBUS);
-	gen6_gt_check_fifodbg(dev_priv);
+
+	if (IS_GEN7(dev_priv->dev))
+		gen6_gt_check_fifodbg(dev_priv);
 }
 
 static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
@@ -311,7 +313,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev)
 		__gen6_gt_force_wake_reset(dev_priv);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_GEN8(dev))
-		__gen6_gt_force_wake_mt_reset(dev_priv);
+		__gen7_gt_force_wake_mt_reset(dev_priv);
 }
 
 void intel_uncore_early_sanitize(struct drm_device *dev)
@@ -689,8 +691,8 @@ void intel_uncore_init(struct drm_device *dev)
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
 	} else if (IS_HASWELL(dev) || IS_GEN8(dev)) {
-		dev_priv->uncore.funcs.force_wake_get = __gen6_gt_force_wake_mt_get;
-		dev_priv->uncore.funcs.force_wake_put = __gen6_gt_force_wake_mt_put;
+		dev_priv->uncore.funcs.force_wake_get = __gen7_gt_force_wake_mt_get;
+		dev_priv->uncore.funcs.force_wake_put = __gen7_gt_force_wake_mt_put;
 	} else if (IS_IVYBRIDGE(dev)) {
 		u32 ecobus;
 
@@ -704,16 +706,16 @@ void intel_uncore_init(struct drm_device *dev)
 		 * forcewake being disabled.
 		 */
 		mutex_lock(&dev->struct_mutex);
-		__gen6_gt_force_wake_mt_get(dev_priv, FORCEWAKE_ALL);
+		__gen7_gt_force_wake_mt_get(dev_priv, FORCEWAKE_ALL);
 		ecobus = __raw_i915_read32(dev_priv, ECOBUS);
-		__gen6_gt_force_wake_mt_put(dev_priv, FORCEWAKE_ALL);
+		__gen7_gt_force_wake_mt_put(dev_priv, FORCEWAKE_ALL);
 		mutex_unlock(&dev->struct_mutex);
 
 		if (ecobus & FORCEWAKE_MT_ENABLE) {
 			dev_priv->uncore.funcs.force_wake_get =
-				__gen6_gt_force_wake_mt_get;
+				__gen7_gt_force_wake_mt_get;
 			dev_priv->uncore.funcs.force_wake_put =
-				__gen6_gt_force_wake_mt_put;
+				__gen7_gt_force_wake_mt_put;
 		} else {
 			DRM_INFO("No MT forcewake available on Ivybridge, this can result in issues\n");
 			DRM_INFO("when using vblank-synced partial screen updates.\n");
@@ -974,7 +976,10 @@ static int gen6_do_reset(struct drm_device *dev)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
 
 	/* Restore fifo count */
-	dev_priv->uncore.fifo_count = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
+	if (IS_GEN6(dev) || IS_GEN7(dev))
+		dev_priv->uncore.fifo_count =
+			__raw_i915_read32(dev_priv, GTFIFOCTL) &
+			GT_FIFO_FREE_ENTRIES_MASK;
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 	return ret;
-- 
1.7.9.5

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/5] drm/i915: Fix forcewake counts for gen8

[Ben Widawsky]

On Fri, Feb 21, 2014 at 05:31:59PM +0200, mika.kuoppala@intel.com wrote:
> From: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Sometimes generic driver code gets forcewake explicitly by
> gen6_gt_force_wake_get(), which check forcewake_count before accessing
> hardware. However the register access with gen8_write function access
> low level hw accessors directly, ignoring the forcewake_count. This
> leads to nested forcewake get from hardware, in ring init and possibly
> elsewhere, causing forcewake ack clear errors and/or hangs.
> 
> Fix this by checking the forcewake count also in gen8_write
> 
> v2: Read side doesn't care about shadowed registers,
>     Remove __needs_put funkiness from gen8_write. (Ville)
>     Improved commit message.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

For those concerned with the performance implication of the extra if
(if anyone at all cares, it's Chris) - I suppose we could also just add
the lock to gen6_gt_force_wake_get/put.

[snip]


-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Fix forcewake counts for gen8

[Ben Widawsky]

On Fri, Feb 21, 2014 at 11:38:48AM -0800, Ben Widawsky wrote:
> On Fri, Feb 21, 2014 at 05:31:59PM +0200, mika.kuoppala@intel.com wrote:
> > From: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Sometimes generic driver code gets forcewake explicitly by
> > gen6_gt_force_wake_get(), which check forcewake_count before accessing
> > hardware. However the register access with gen8_write function access
> > low level hw accessors directly, ignoring the forcewake_count. This
> > leads to nested forcewake get from hardware, in ring init and possibly
> > elsewhere, causing forcewake ack clear errors and/or hangs.
> > 
> > Fix this by checking the forcewake count also in gen8_write
> > 
> > v2: Read side doesn't care about shadowed registers,
> >     Remove __needs_put funkiness from gen8_write. (Ville)
> >     Improved commit message.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> For those concerned with the performance implication of the extra if
> (if anyone at all cares, it's Chris) - I suppose we could also just add
> the lock to gen6_gt_force_wake_get/put.
> 
> [snip]

I should have also said, we have very few cases where it's actually a
problem (ie. not just done at init). And in fact, I am wondering now how
one could hit this at all without enabling rps.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Fix forcewake counts for gen8

[Chris Wilson]

On Fri, Feb 21, 2014 at 11:38:48AM -0800, Ben Widawsky wrote:
> On Fri, Feb 21, 2014 at 05:31:59PM +0200, mika.kuoppala@intel.com wrote:
> > From: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Sometimes generic driver code gets forcewake explicitly by
> > gen6_gt_force_wake_get(), which check forcewake_count before accessing
> > hardware. However the register access with gen8_write function access
> > low level hw accessors directly, ignoring the forcewake_count. This
> > leads to nested forcewake get from hardware, in ring init and possibly
> > elsewhere, causing forcewake ack clear errors and/or hangs.
> > 
> > Fix this by checking the forcewake count also in gen8_write
> > 
> > v2: Read side doesn't care about shadowed registers,
> >     Remove __needs_put funkiness from gen8_write. (Ville)
> >     Improved commit message.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> For those concerned with the performance implication of the extra if
> (if anyone at all cares, it's Chris) - I suppose we could also just add
> the lock to gen6_gt_force_wake_get/put.

Nah, my goal is to minimise the number of register reads (and even the
writes since they are not light but end up involving register reads) out
of hotpaths (execbuf, interrupts, fences).
-Chris

Re: [PATCH 1/5] drm/i915: Fix forcewake counts for gen8

[Ville Syrjälä]

On Fri, Feb 21, 2014 at 11:38:48AM -0800, Ben Widawsky wrote:
> On Fri, Feb 21, 2014 at 05:31:59PM +0200, mika.kuoppala@intel.com wrote:
> > From: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Sometimes generic driver code gets forcewake explicitly by
> > gen6_gt_force_wake_get(), which check forcewake_count before accessing
> > hardware. However the register access with gen8_write function access
> > low level hw accessors directly, ignoring the forcewake_count. This
> > leads to nested forcewake get from hardware, in ring init and possibly
> > elsewhere, causing forcewake ack clear errors and/or hangs.
> > 
> > Fix this by checking the forcewake count also in gen8_write
> > 
> > v2: Read side doesn't care about shadowed registers,
> >     Remove __needs_put funkiness from gen8_write. (Ville)
> >     Improved commit message.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> For those concerned with the performance implication of the extra if
> (if anyone at all cares, it's Chris) - I suppose we could also just add
> the lock to gen6_gt_force_wake_get/put.

Two things:
a) I don't understand what you mean here. uncore.lock already protects
the forcewake count in gen6_gt_force_wake_get/put. Also there's no way
to avoid the branch here since doing a .force_wake_put() w/o checking
the forcewake count is never ok.

b) The cost of branch should be a drop in the ocean compared to the
cost of the register reads/writes in .forcewake_get/put.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/5] drm/i915: Fix forcewake counts for gen8

[Ben Widawsky]

On Mon, Feb 24, 2014 at 01:27:09PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 21, 2014 at 11:38:48AM -0800, Ben Widawsky wrote:
> > On Fri, Feb 21, 2014 at 05:31:59PM +0200, mika.kuoppala@intel.com wrote:
> > > From: Mika Kuoppala <mika.kuoppala@intel.com>
> > > 
> > > Sometimes generic driver code gets forcewake explicitly by
> > > gen6_gt_force_wake_get(), which check forcewake_count before accessing
> > > hardware. However the register access with gen8_write function access
> > > low level hw accessors directly, ignoring the forcewake_count. This
> > > leads to nested forcewake get from hardware, in ring init and possibly
> > > elsewhere, causing forcewake ack clear errors and/or hangs.
> > > 
> > > Fix this by checking the forcewake count also in gen8_write
> > > 
> > > v2: Read side doesn't care about shadowed registers,
> > >     Remove __needs_put funkiness from gen8_write. (Ville)
> > >     Improved commit message.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > For those concerned with the performance implication of the extra if
> > (if anyone at all cares, it's Chris) - I suppose we could also just add
> > the lock to gen6_gt_force_wake_get/put.
> 
> Two things:
> a) I don't understand what you mean here. uncore.lock already protects
> the forcewake count in gen6_gt_force_wake_get/put. Also there's no way
> to avoid the branch here since doing a .force_wake_put() w/o checking
> the forcewake count is never ok.
> 

I meant acquire the lock in get() release the lock in put()

> b) The cost of branch should be a drop in the ocean compared to the
> cost of the register reads/writes in .forcewake_get/put.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: No need to put forcewake after a reset

[Daniel Vetter]

On Fri, Feb 21, 2014 at 05:32:03PM +0200, mika.kuoppala@intel.com wrote:
> From: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> As we now have intel_uncore_forcewake_reset() no need
> to do explicit put after reset.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

I've merged patches 2-4 from this series now too (patch 1 is already
merged). But this one conflicts badly with Ville's patch, can you please
respin?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: No need to put forcewake after a reset

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, Feb 21, 2014 at 05:32:03PM +0200, mika.kuoppala@intel.com wrote:
>> From: Mika Kuoppala <mika.kuoppala@intel.com>
>> 
>> As we now have intel_uncore_forcewake_reset() no need
>> to do explicit put after reset.
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> I've merged patches 2-4 from this series now too (patch 1 is already
> merged). But this one conflicts badly with Ville's patch, can you please
> respin?

Didn't find 4/5 from dinq so rebased 4 and 5 on top of dinq at:
1394035700-19630-1-git-send-email-mika.kuoppala@intel.com

-Mika

> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch