[PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable

[PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable

[Chris Wilson]

Tidy up the routines for interacting with the GT (in particular the
forcewake dance) which are scattered throughout the code in a single
structure.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c      |    2 +-
 drivers/gpu/drm/i915/i915_drv.c      |  100 ++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/i915_drv.h      |   17 +++---
 drivers/gpu/drm/i915/intel_display.c |    3 -
 drivers/gpu/drm/i915/intel_pm.c      |   30 ----------
 5 files changed, 81 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index fa8f269..a605928 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1548,6 +1548,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	intel_irq_init(dev);
+	intel_gt_init(dev);
 
 	/* Try to make sure MCHBAR is enabled before poking at it */
 	intel_setup_mchbar(dev);
@@ -1580,7 +1581,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (!IS_I945G(dev) && !IS_I945GM(dev))
 		pci_enable_msi(dev->pdev);
 
-	spin_lock_init(&dev_priv->gt_lock);
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->error_lock);
 	spin_lock_init(&dev_priv->rps_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 238a521..2058316 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -32,6 +32,7 @@
 #include "drm.h"
 #include "i915_drm.h"
 #include "i915_drv.h"
+#include "i915_trace.h"
 #include "intel_drv.h"
 
 #include <linux/console.h>
@@ -423,36 +424,38 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return 1;
 }
 
-void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
+static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
-	int count;
+	unsigned long timeout;
 
-	count = 0;
-	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
-		udelay(10);
+	timeout = jiffies + msecs_to_jiffies(1);
+	while (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1 &&
+	       time_before(jiffies, timeout))
+		;
 
 	I915_WRITE_NOTRACE(FORCEWAKE, 1);
-	POSTING_READ(FORCEWAKE);
 
-	count = 0;
-	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0)
-		udelay(10);
+	timeout = jiffies + msecs_to_jiffies(1);
+	while ((I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0 &&
+	       time_before(jiffies, timeout))
+		;
 }
 
-void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
+static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
 {
-	int count;
+	unsigned long timeout;
 
-	count = 0;
-	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1))
-		udelay(10);
+	timeout = jiffies + msecs_to_jiffies(1);
+	while (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1 &&
+	       time_before(jiffies, timeout))
+		;
 
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
-	POSTING_READ(FORCEWAKE_MT);
 
-	count = 0;
-	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0)
-		udelay(10);
+	timeout = jiffies + msecs_to_jiffies(1);
+	while ((I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0 &&
+	       time_before(jiffies, timeout))
+		;
 }
 
 /*
@@ -467,7 +470,7 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 
 	spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
 	if (dev_priv->forcewake_count++ == 0)
-		dev_priv->display.force_wake_get(dev_priv);
+		dev_priv->gt.force_wake_get(dev_priv);
 	spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
 }
 
@@ -480,14 +483,14 @@ static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
 		I915_WRITE_NOTRACE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);
 }
 
-void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
+static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
 	/* The below doubles as a POSTING_READ */
 	gen6_gt_check_fifodbg(dev_priv);
 }
 
-void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
+static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(1));
 	/* The below doubles as a POSTING_READ */
@@ -503,7 +506,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 
 	spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
 	if (--dev_priv->forcewake_count == 0)
-		dev_priv->display.force_wake_put(dev_priv);
+		dev_priv->gt.force_wake_put(dev_priv);
 	spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
 }
 
@@ -527,7 +530,7 @@ int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-void vlv_force_wake_get(struct drm_i915_private *dev_priv)
+static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
 {
 	int count;
 
@@ -545,13 +548,54 @@ void vlv_force_wake_get(struct drm_i915_private *dev_priv)
 		udelay(10);
 }
 
-void vlv_force_wake_put(struct drm_i915_private *dev_priv)
+static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffff0000);
 	/* FIXME: confirm VLV behavior with Punit folks */
 	POSTING_READ(FORCEWAKE_VLV);
 }
 
+void intel_gt_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	spin_lock_init(&dev_priv->gt_lock);
+
+	if (IS_VALLEYVIEW(dev)) {
+		dev_priv->gt.force_wake_get = vlv_force_wake_get;
+		dev_priv->gt.force_wake_put = vlv_force_wake_put;
+	} else if (INTEL_INFO(dev)->gen >= 6) {
+		dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get;
+		dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put;
+
+		/* IVB configs may use multi-threaded forcewake */
+		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
+			u32 ecobus;
+
+			/* A small trick here - if the bios hasn't configured
+			 * MT forcewake, and if the device is in RC6, then
+			 * force_wake_mt_get will not wake the device and the
+			 * ECOBUS read will return zero. Which will be
+			 * (correctly) interpreted by the test below as MT
+			 * forcewake being disabled.
+			 */
+			mutex_lock(&dev->struct_mutex);
+			__gen6_gt_force_wake_mt_get(dev_priv);
+			ecobus = I915_READ_NOTRACE(ECOBUS);
+			__gen6_gt_force_wake_mt_put(dev_priv);
+			mutex_unlock(&dev->struct_mutex);
+
+			if (ecobus & FORCEWAKE_MT_ENABLE) {
+				DRM_DEBUG_KMS("Using MT version of forcewake\n");
+				dev_priv->gt.force_wake_get =
+					__gen6_gt_force_wake_mt_get;
+				dev_priv->gt.force_wake_put =
+					__gen6_gt_force_wake_mt_put;
+			}
+		}
+	}
+}
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -788,9 +832,9 @@ static int gen6_do_reset(struct drm_device *dev)
 
 	/* If reset with a user forcewake, try to restore, otherwise turn it off */
 	if (dev_priv->forcewake_count)
-		dev_priv->display.force_wake_get(dev_priv);
+		dev_priv->gt.force_wake_get(dev_priv);
 	else
-		dev_priv->display.force_wake_put(dev_priv);
+		dev_priv->gt.force_wake_put(dev_priv);
 
 	/* Restore fifo count */
 	dev_priv->gt_fifo_count = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
@@ -1151,10 +1195,10 @@ u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
 		unsigned long irqflags; \
 		spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \
 		if (dev_priv->forcewake_count == 0) \
-			dev_priv->display.force_wake_get(dev_priv); \
+			dev_priv->gt.force_wake_get(dev_priv); \
 		val = read##y(dev_priv->regs + reg); \
 		if (dev_priv->forcewake_count == 0) \
-			dev_priv->display.force_wake_put(dev_priv); \
+			dev_priv->gt.force_wake_put(dev_priv); \
 		spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags); \
 	} else { \
 		val = read##y(dev_priv->regs + reg); \
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6f0dff9..b00119a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -261,8 +261,6 @@ struct drm_i915_display_funcs {
 			  struct drm_i915_gem_object *obj);
 	int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 			    int x, int y);
-	void (*force_wake_get)(struct drm_i915_private *dev_priv);
-	void (*force_wake_put)(struct drm_i915_private *dev_priv);
 	/* clock updates for mode set */
 	/* cursor updates */
 	/* render clock increase/decrease */
@@ -270,6 +268,11 @@ struct drm_i915_display_funcs {
 	/* pll clock increase/decrease */
 };
 
+struct drm_i915_gt_funcs {
+	void (*force_wake_get)(struct drm_i915_private *dev_priv);
+	void (*force_wake_put)(struct drm_i915_private *dev_priv);
+};
+
 struct intel_device_info {
 	u8 gen;
 	u8 is_mobile:1;
@@ -349,6 +352,8 @@ typedef struct drm_i915_private {
 	int relative_constants_mode;
 
 	void __iomem *regs;
+
+	struct drm_i915_gt_funcs gt;
 	/** gt_fifo_count and the subsequent register write are synchronized
 	 * with dev->struct_mutex. */
 	unsigned gt_fifo_count;
@@ -1177,6 +1182,7 @@ void i915_hangcheck_elapsed(unsigned long data);
 void i915_handle_error(struct drm_device *dev, bool wedged);
 
 extern void intel_irq_init(struct drm_device *dev);
+extern void intel_gt_init(struct drm_device *dev);
 
 void i915_error_state_free(struct kref *error_ref);
 
@@ -1484,13 +1490,6 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
 extern int intel_enable_rc6(const struct drm_device *dev);
 
 extern bool i915_semaphore_is_enabled(struct drm_device *dev);
-extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
-extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
-extern void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
-extern void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv);
-
-extern void vlv_force_wake_get(struct drm_i915_private *dev_priv);
-extern void vlv_force_wake_put(struct drm_i915_private *dev_priv);
 
 /* overlay */
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index acbfe9e..45d700d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6656,9 +6656,6 @@ static void intel_init_display(struct drm_device *dev)
 			dev_priv->display.write_eld = ironlake_write_eld;
 		} else
 			dev_priv->display.update_wm = NULL;
-	} else if (IS_VALLEYVIEW(dev)) {
-		dev_priv->display.force_wake_get = vlv_force_wake_get;
-		dev_priv->display.force_wake_put = vlv_force_wake_put;
 	} else if (IS_G4X(dev)) {
 		dev_priv->display.write_eld = g4x_write_eld;
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a62c673..82fcbb4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3686,34 +3686,6 @@ void intel_init_pm(struct drm_device *dev)
 
 	/* For FIFO watermark updates */
 	if (HAS_PCH_SPLIT(dev)) {
-		dev_priv->display.force_wake_get = __gen6_gt_force_wake_get;
-		dev_priv->display.force_wake_put = __gen6_gt_force_wake_put;
-
-		/* IVB configs may use multi-threaded forcewake */
-		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
-			u32	ecobus;
-
-			/* A small trick here - if the bios hasn't configured MT forcewake,
-			 * and if the device is in RC6, then force_wake_mt_get will not wake
-			 * the device and the ECOBUS read will return zero. Which will be
-			 * (correctly) interpreted by the test below as MT forcewake being
-			 * disabled.
-			 */
-			mutex_lock(&dev->struct_mutex);
-			__gen6_gt_force_wake_mt_get(dev_priv);
-			ecobus = I915_READ_NOTRACE(ECOBUS);
-			__gen6_gt_force_wake_mt_put(dev_priv);
-			mutex_unlock(&dev->struct_mutex);
-
-			if (ecobus & FORCEWAKE_MT_ENABLE) {
-				DRM_DEBUG_KMS("Using MT version of forcewake\n");
-				dev_priv->display.force_wake_get =
-					__gen6_gt_force_wake_mt_get;
-				dev_priv->display.force_wake_put =
-					__gen6_gt_force_wake_mt_put;
-			}
-		}
-
 		if (HAS_PCH_IBX(dev))
 			dev_priv->display.init_pch_clock_gating = ibx_init_clock_gating;
 		else if (HAS_PCH_CPT(dev))
@@ -3769,8 +3741,6 @@ void intel_init_pm(struct drm_device *dev)
 		dev_priv->display.update_wm = valleyview_update_wm;
 		dev_priv->display.init_clock_gating =
 			valleyview_init_clock_gating;
-		dev_priv->display.force_wake_get = vlv_force_wake_get;
-		dev_priv->display.force_wake_put = vlv_force_wake_put;
 	} else if (IS_PINEVIEW(dev)) {
 		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
 					    dev_priv->is_ddr3,
-- 
1.7.10

[PATCH 2/2] drm/i915: Implement w/a for sporadic read failures on waking from rc6

[Chris Wilson]

As a w/a to prevent reads sporadically returning 0, we need to wait for
the GT thread to return to TC0 before proceeding to read the registers.

References: https://bugs.freedesktop.org/show_bug.cgi?id=50243
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c |   19 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h |    3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2058316..ba3f1a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -424,6 +424,19 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return 1;
 }
 
+static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
+{
+	unsigned long timeout;
+
+	/* w/a for a sporadic read returning 0 by waiting for the GT
+	 * thread to wake up.
+	 */
+	timeout = jiffies + msecs_to_jiffies(1);
+	while (I915_READ_NOTRACE(GEN6_GT_THREAD_STATUS_REG) & GEN6_GT_THREAD_STATUS_CORE_MASK &&
+	       time_before(jiffies, timeout))
+		;
+}
+
 static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
 	unsigned long timeout;
@@ -439,6 +452,8 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 	while ((I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0 &&
 	       time_before(jiffies, timeout))
 		;
+
+	__gen6_gt_wait_for_thread_c0(dev_priv);
 }
 
 static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
@@ -456,6 +471,8 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
 	while ((I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0 &&
 	       time_before(jiffies, timeout))
 		;
+
+	__gen6_gt_wait_for_thread_c0(dev_priv);
 }
 
 /*
@@ -546,6 +563,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
 	count = 0;
 	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1) == 0)
 		udelay(10);
+
+	__gen6_gt_wait_for_thread_c0(dev_priv);
 }
 
 static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 23f8954..b51414e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1443,6 +1443,9 @@
 #define DDRMPLL1		0X12c20
 #define PEG_BAND_GAP_DATA	0x14d68
 
+#define GEN6_GT_THREAD_STATUS_REG 0x13805c
+#define GEN6_GT_THREAD_STATUS_CORE_MASK 0x7
+
 #define GEN6_GT_PERF_STATUS	0x145948
 #define GEN6_RP_STATE_LIMITS	0x145994
 #define GEN6_RP_STATE_CAP	0x145998
-- 
1.7.10

Re: [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable

[Ben Widawsky]

On Wed, 13 Jun 2012 18:29:51 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Tidy up the routines for interacting with the GT (in particular the
> forcewake dance) which are scattered throughout the code in a single
> structure.

A few comments inline. First though, the bikeshed:

I'd really rather the structure not be named, "gt" unless you have
further reaching plans for it. GT is way to generic. Also, I think it
makes a lot of sense to move the forcewake dancing into intel_pm.c

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |    2 +-
>  drivers/gpu/drm/i915/i915_drv.c      |  100 ++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/i915_drv.h      |   17 +++---
>  drivers/gpu/drm/i915/intel_display.c |    3 -
>  drivers/gpu/drm/i915/intel_pm.c      |   30 ----------
>  5 files changed, 81 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index fa8f269..a605928 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1548,6 +1548,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  
>  	intel_irq_init(dev);
> +	intel_gt_init(dev);
>  
>  	/* Try to make sure MCHBAR is enabled before poking at it */
>  	intel_setup_mchbar(dev);
> @@ -1580,7 +1581,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (!IS_I945G(dev) && !IS_I945GM(dev))
>  		pci_enable_msi(dev->pdev);
>  
> -	spin_lock_init(&dev_priv->gt_lock);
>  	spin_lock_init(&dev_priv->irq_lock);
>  	spin_lock_init(&dev_priv->error_lock);
>  	spin_lock_init(&dev_priv->rps_lock);

By this logic, we should also move the irq_lock init to intel_irq_init,
and the rps_lock init to enable_rps or something.

> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 238a521..2058316 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -32,6 +32,7 @@
>  #include "drm.h"
>  #include "i915_drm.h"
>  #include "i915_drv.h"
> +#include "i915_trace.h"
>  #include "intel_drv.h"
>  
>  #include <linux/console.h>

What is this doing here?

> @@ -423,36 +424,38 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
>  	return 1;
>  }
>  
> -void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
>  {
> -	int count;
> +	unsigned long timeout;
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1 &&
> +	       time_before(jiffies, timeout))
> +		;
> 

Can't we just use the wait_for macros here?
 
>  	I915_WRITE_NOTRACE(FORCEWAKE, 1);
> -	POSTING_READ(FORCEWAKE);
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0)
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while ((I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0 &&
> +	       time_before(jiffies, timeout))
> +		;
>  }

Same as above.

>  
> -void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
>  {
> -	int count;
> +	unsigned long timeout;
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1))
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1 &&
> +	       time_before(jiffies, timeout))
> +		;
>  

Again

>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
> -	POSTING_READ(FORCEWAKE_MT);
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0)
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while ((I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0 &&
> +	       time_before(jiffies, timeout))
> +		;
>  }
>  

You get the idea

>  /*
> @@ -467,7 +470,7 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
>  
>  	spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>  	if (dev_priv->forcewake_count++ == 0)
> -		dev_priv->display.force_wake_get(dev_priv);
> +		dev_priv->gt.force_wake_get(dev_priv);
>  	spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
>  }
>  
> @@ -480,14 +483,14 @@ static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
>  		I915_WRITE_NOTRACE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);
>  }
>  
> -void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE, 0);
>  	/* The below doubles as a POSTING_READ */
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> -void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(1));
>  	/* The below doubles as a POSTING_READ */
> @@ -503,7 +506,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  
>  	spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>  	if (--dev_priv->forcewake_count == 0)
> -		dev_priv->display.force_wake_put(dev_priv);
> +		dev_priv->gt.force_wake_put(dev_priv);
>  	spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
>  }
>  
> @@ -527,7 +530,7 @@ int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -void vlv_force_wake_get(struct drm_i915_private *dev_priv)
> +static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
>  {
>  	int count;
>  
> @@ -545,13 +548,54 @@ void vlv_force_wake_get(struct drm_i915_private *dev_priv)
>  		udelay(10);
>  }
>  
> -void vlv_force_wake_put(struct drm_i915_private *dev_priv)
> +static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffff0000);
>  	/* FIXME: confirm VLV behavior with Punit folks */
>  	POSTING_READ(FORCEWAKE_VLV);
>  }
>  
> +void intel_gt_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	spin_lock_init(&dev_priv->gt_lock);
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		dev_priv->gt.force_wake_get = vlv_force_wake_get;
> +		dev_priv->gt.force_wake_put = vlv_force_wake_put;
> +	} else if (INTEL_INFO(dev)->gen >= 6) {
> +		dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get;
> +		dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put;
> +
> +		/* IVB configs may use multi-threaded forcewake */
> +		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> +			u32 ecobus;
> +
> +			/* A small trick here - if the bios hasn't configured
> +			 * MT forcewake, and if the device is in RC6, then
> +			 * force_wake_mt_get will not wake the device and the
> +			 * ECOBUS read will return zero. Which will be
> +			 * (correctly) interpreted by the test below as MT
> +			 * forcewake being disabled.
> +			 */
> +			mutex_lock(&dev->struct_mutex);
> +			__gen6_gt_force_wake_mt_get(dev_priv);
> +			ecobus = I915_READ_NOTRACE(ECOBUS);
> +			__gen6_gt_force_wake_mt_put(dev_priv);
> +			mutex_unlock(&dev->struct_mutex);
> +
> +			if (ecobus & FORCEWAKE_MT_ENABLE) {
> +				DRM_DEBUG_KMS("Using MT version of forcewake\n");
> +				dev_priv->gt.force_wake_get =
> +					__gen6_gt_force_wake_mt_get;
> +				dev_priv->gt.force_wake_put =
> +					__gen6_gt_force_wake_mt_put;
> +			}
> +		}
> +	}
> +}
> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -788,9 +832,9 @@ static int gen6_do_reset(struct drm_device *dev)
>  
>  	/* If reset with a user forcewake, try to restore, otherwise turn it off */
>  	if (dev_priv->forcewake_count)
> -		dev_priv->display.force_wake_get(dev_priv);
> +		dev_priv->gt.force_wake_get(dev_priv);
>  	else
> -		dev_priv->display.force_wake_put(dev_priv);
> +		dev_priv->gt.force_wake_put(dev_priv);
>  
>  	/* Restore fifo count */
>  	dev_priv->gt_fifo_count = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
> @@ -1151,10 +1195,10 @@ u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
>  		unsigned long irqflags; \
>  		spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \
>  		if (dev_priv->forcewake_count == 0) \
> -			dev_priv->display.force_wake_get(dev_priv); \
> +			dev_priv->gt.force_wake_get(dev_priv); \
gg>  		val = read##y(dev_priv->regs + reg); \
>  		if (dev_priv->forcewake_count == 0) \
> -			dev_priv->display.force_wake_put(dev_priv); \
> +			dev_priv->gt.force_wake_put(dev_priv); \
>  		spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags); \
>  	} else { \
>  		val = read##y(dev_priv->regs + reg); \
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6f0dff9..b00119a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -261,8 +261,6 @@ struct drm_i915_display_funcs {
>  			  struct drm_i915_gem_object *obj);
>  	int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  			    int x, int y);
> -	void (*force_wake_get)(struct drm_i915_private *dev_priv);
> -	void (*force_wake_put)(struct drm_i915_private *dev_priv);
>  	/* clock updates for mode set */
>  	/* cursor updates */
>  	/* render clock increase/decrease */
> @@ -270,6 +268,11 @@ struct drm_i915_display_funcs {
>  	/* pll clock increase/decrease */
>  };
>  
> +struct drm_i915_gt_funcs {
> +	void (*force_wake_get)(struct drm_i915_private *dev_priv);
> +	void (*force_wake_put)(struct drm_i915_private *dev_priv);
> +};
> +
>  struct intel_device_info {
>  	u8 gen;
>  	u8 is_mobile:1;
> @@ -349,6 +352,8 @@ typedef struct drm_i915_private {
>  	int relative_constants_mode;
>  
>  	void __iomem *regs;
> +
> +	struct drm_i915_gt_funcs gt;
>  	/** gt_fifo_count and the subsequent register write are synchronized
>  	 * with dev->struct_mutex. */
>  	unsigned gt_fifo_count;
> @@ -1177,6 +1182,7 @@ void i915_hangcheck_elapsed(unsigned long data);
>  void i915_handle_error(struct drm_device *dev, bool wedged);
>  
>  extern void intel_irq_init(struct drm_device *dev);
> +extern void intel_gt_init(struct drm_device *dev);
>  
>  void i915_error_state_free(struct kref *error_ref);
>  
> @@ -1484,13 +1490,6 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>  extern int intel_enable_rc6(const struct drm_device *dev);
>  
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> -extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
> -extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
> -extern void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
> -extern void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv);
> -
> -extern void vlv_force_wake_get(struct drm_i915_private *dev_priv);
> -extern void vlv_force_wake_put(struct drm_i915_private *dev_priv);
>  
>  /* overlay */
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index acbfe9e..45d700d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6656,9 +6656,6 @@ static void intel_init_display(struct drm_device *dev)
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else
>  			dev_priv->display.update_wm = NULL;
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		dev_priv->display.force_wake_get = vlv_force_wake_get;
> -		dev_priv->display.force_wake_put = vlv_force_wake_put;
>  	} else if (IS_G4X(dev)) {
>  		dev_priv->display.write_eld = g4x_write_eld;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a62c673..82fcbb4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3686,34 +3686,6 @@ void intel_init_pm(struct drm_device *dev)
>  
>  	/* For FIFO watermark updates */
>  	if (HAS_PCH_SPLIT(dev)) {
> -		dev_priv->display.force_wake_get = __gen6_gt_force_wake_get;
> -		dev_priv->display.force_wake_put = __gen6_gt_force_wake_put;
> -
> -		/* IVB configs may use multi-threaded forcewake */
> -		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> -			u32	ecobus;
> -
> -			/* A small trick here - if the bios hasn't configured MT forcewake,
> -			 * and if the device is in RC6, then force_wake_mt_get will not wake
> -			 * the device and the ECOBUS read will return zero. Which will be
> -			 * (correctly) interpreted by the test below as MT forcewake being
> -			 * disabled.
> -			 */
> -			mutex_lock(&dev->struct_mutex);
> -			__gen6_gt_force_wake_mt_get(dev_priv);
> -			ecobus = I915_READ_NOTRACE(ECOBUS);
> -			__gen6_gt_force_wake_mt_put(dev_priv);
> -			mutex_unlock(&dev->struct_mutex);
> -
> -			if (ecobus & FORCEWAKE_MT_ENABLE) {
> -				DRM_DEBUG_KMS("Using MT version of forcewake\n");
> -				dev_priv->display.force_wake_get =
> -					__gen6_gt_force_wake_mt_get;
> -				dev_priv->display.force_wake_put =
> -					__gen6_gt_force_wake_mt_put;
> -			}
> -		}
> -
>  		if (HAS_PCH_IBX(dev))
>  			dev_priv->display.init_pch_clock_gating = ibx_init_clock_gating;
>  		else if (HAS_PCH_CPT(dev))
> @@ -3769,8 +3741,6 @@ void intel_init_pm(struct drm_device *dev)
>  		dev_priv->display.update_wm = valleyview_update_wm;
>  		dev_priv->display.init_clock_gating =
>  			valleyview_init_clock_gating;
> -		dev_priv->display.force_wake_get = vlv_force_wake_get;
> -		dev_priv->display.force_wake_put = vlv_force_wake_put;
>  	} else if (IS_PINEVIEW(dev)) {
>  		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
>  					    dev_priv->is_ddr3,



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable

[Chris Wilson]

On Wed, 13 Jun 2012 12:07:19 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Wed, 13 Jun 2012 18:29:51 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Tidy up the routines for interacting with the GT (in particular the
> > forcewake dance) which are scattered throughout the code in a single
> > structure.
> 
> A few comments inline. First though, the bikeshed:
> 
> I'd really rather the structure not be named, "gt" unless you have
> further reaching plans for it. GT is way to generic. Also, I think it
> makes a lot of sense to move the forcewake dancing into intel_pm.c

This patch predated the intel_pm split. I toyed with the idea of
updating it, but preferred to get feedback first. Shall we call it
grantsdale instead? Or uncore? My opinion is that this more core
functionality than power-management, but first and foremost it
should not be scattered across multiple files.
 
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |    2 +-
> >  drivers/gpu/drm/i915/i915_drv.c      |  100 ++++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/i915_drv.h      |   17 +++---
> >  drivers/gpu/drm/i915/intel_display.c |    3 -
> >  drivers/gpu/drm/i915/intel_pm.c      |   30 ----------
> >  5 files changed, 81 insertions(+), 71 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index fa8f269..a605928 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1548,6 +1548,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	}
> >  
> >  	intel_irq_init(dev);
> > +	intel_gt_init(dev);
> >  
> >  	/* Try to make sure MCHBAR is enabled before poking at it */
> >  	intel_setup_mchbar(dev);
> > @@ -1580,7 +1581,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	if (!IS_I945G(dev) && !IS_I945GM(dev))
> >  		pci_enable_msi(dev->pdev);
> >  
> > -	spin_lock_init(&dev_priv->gt_lock);
> >  	spin_lock_init(&dev_priv->irq_lock);
> >  	spin_lock_init(&dev_priv->error_lock);
> >  	spin_lock_init(&dev_priv->rps_lock);
> 
> By this logic, we should also move the irq_lock init to intel_irq_init,
> and the rps_lock init to enable_rps or something.
> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 238a521..2058316 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -32,6 +32,7 @@
> >  #include "drm.h"
> >  #include "i915_drm.h"
> >  #include "i915_drv.h"
> > +#include "i915_trace.h"
> >  #include "intel_drv.h"
> >  
> >  #include <linux/console.h>
> 
> What is this doing here?

An earlier version was more funky with grander purpose.
 
> > @@ -423,36 +424,38 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >  	return 1;
> >  }
> >  
> > -void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> > +static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> >  {
> > -	int count;
> > +	unsigned long timeout;
> >  
> > -	count = 0;
> > -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
> > -		udelay(10);
> > +	timeout = jiffies + msecs_to_jiffies(1);
> > +	while (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1 &&
> > +	       time_before(jiffies, timeout))
> > +		;
> > 
> 
> Can't we just use the wait_for macros here?
Yes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable

[Eugeni Dodonov]

On 06/13/2012 04:47 PM, Chris Wilson wrote:
> On Wed, 13 Jun 2012 12:07:19 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
>> On Wed, 13 Jun 2012 18:29:51 +0100
>> Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>
>>> Tidy up the routines for interacting with the GT (in particular the
>>> forcewake dance) which are scattered throughout the code in a single
>>> structure.
>>
>> A few comments inline. First though, the bikeshed:
>>
>> I'd really rather the structure not be named, "gt" unless you have
>> further reaching plans for it. GT is way to generic. Also, I think it
>> makes a lot of sense to move the forcewake dancing into intel_pm.c
> 
> This patch predated the intel_pm split. I toyed with the idea of
> updating it, but preferred to get feedback first. Shall we call it
> grantsdale instead? Or uncore? My opinion is that this more core
> functionality than power-management, but first and foremost it
> should not be scattered across multiple files.

I liked the idea of 'gt' because this is how the docs call it too. And
our power code is hidden all around indeed.

So I'd vote for this patch once it gets updated to intel_pm. I'll even
volunteer myself to adjust the power wells and new force wake stuff to
it when it becomes ready :).

Eugeni

Re: [PATCH 2/2] drm/i915: Implement w/a for sporadic read failures on waking from rc6

[Eugeni Dodonov]

On 06/13/2012 02:29 PM, Chris Wilson wrote:
> As a w/a to prevent reads sporadically returning 0, we need to wait for
> the GT thread to return to TC0 before proceeding to read the registers.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=50243
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

I think that this patch could solve some of the RC6-related issues which
are still out there. So the ones of you affected by random GPU hangs or
dropped writes, could you please give this patch a try?

Eugeni