diff mbox

[RFC,06/44] drm/i915: Fixes for FIFO space queries

Message ID 1403803475-16337-7-git-send-email-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Harrison June 26, 2014, 5:23 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

The previous code was not correctly masking the value of the GTFIFOCTL register,
leading to overruns and the message "MMIO read or write has been dropped". In
addition, the checks were repeated in several different places. This commit
replaces these various checks with a simple (inline) function to encapsulate the
read-and-mask operation. In addition, it adds a custom wait-for-fifo function
for VLV, as the timing parameters are somewhat different from those on earlier
chips.
---
 drivers/gpu/drm/i915/intel_uncore.c |   49 ++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 7 deletions(-)

Comments

Jesse Barnes July 2, 2014, 5:50 p.m. UTC | #1 
On Thu, 26 Jun 2014 18:23:57 +0100
John.C.Harrison@Intel.com wrote:

> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The previous code was not correctly masking the value of the GTFIFOCTL register,
> leading to overruns and the message "MMIO read or write has been dropped". In
> addition, the checks were repeated in several different places. This commit
> replaces these various checks with a simple (inline) function to encapsulate the
> read-and-mask operation. In addition, it adds a custom wait-for-fifo function
> for VLV, as the timing parameters are somewhat different from those on earlier
> chips.
> ---
>  drivers/gpu/drm/i915/intel_uncore.c |   49 ++++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 871c284..6a3dddf 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -47,6 +47,12 @@ assert_device_not_suspended(struct drm_i915_private *dev_priv)
>  	     "Device suspended\n");
>  }
>  
> +static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv)
> +{
> +	u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL);
> +	return count & GT_FIFO_FREE_ENTRIES_MASK;
> +}
> +
>  static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
>  {
>  	u32 gt_thread_status_mask;
> @@ -154,6 +160,28 @@ static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
>  		gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> +static int __vlv_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
> +{
> +	u32 free = fifo_free_entries(dev_priv);
> +	int loop1, loop2;
> +
> +	for (loop1 = 0; loop1 < 5000 && free < GT_FIFO_NUM_RESERVED_ENTRIES; ) {
> +		for (loop2 = 0; loop2 < 1000 && free < GT_FIFO_NUM_RESERVED_ENTRIES; loop2 += 10) {
> +			udelay(10);
> +			free = fifo_free_entries(dev_priv);
> +		}
> +		loop1 += loop2;
> +		if (loop1 > 1000 || free < 48)
> +			DRM_DEBUG("after %d us, the FIFO has %d slots", loop1, free);
> +	}
> +
> +	dev_priv->uncore.fifo_count = free;
> +	if (WARN(free < GT_FIFO_NUM_RESERVED_ENTRIES,
> +		"FIFO has insufficient space (%d slots)", free))
> +		return -1;
> +	return 0;
> +}
> +
>  static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  {
>  	int ret = 0;
> @@ -161,16 +189,15 @@ static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  	/* On VLV, FIFO will be shared by both SW and HW.
>  	 * So, we need to read the FREE_ENTRIES everytime */
>  	if (IS_VALLEYVIEW(dev_priv->dev))
> -		dev_priv->uncore.fifo_count =
> -			__raw_i915_read32(dev_priv, GTFIFOCTL) &
> -						GT_FIFO_FREE_ENTRIES_MASK;
> +		return __vlv_gt_wait_for_fifo(dev_priv);
>  
>  	if (dev_priv->uncore.fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) {
>  		int loop = 500;
> -		u32 fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> +		u32 fifo = fifo_free_entries(dev_priv);
> +
>  		while (fifo <= GT_FIFO_NUM_RESERVED_ENTRIES && loop--) {
>  			udelay(10);
> -			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
> +			fifo = fifo_free_entries(dev_priv);
>  		}
>  		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
>  			++ret;
> @@ -194,6 +221,11 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
>  static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  						int fw_engine)
>  {
> +#if	1
> +	if (__gen6_gt_wait_for_fifo(dev_priv))
> +		gen6_gt_check_fifodbg(dev_priv);
> +#endif
> +
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine) {
>  		if (wait_for_atomic((__raw_i915_read32(dev_priv,
> @@ -238,6 +270,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  					int fw_engine)
>  {
> +#if	1
> +	if (__gen6_gt_wait_for_fifo(dev_priv))
> +		gen6_gt_check_fifodbg(dev_priv);
> +#endif
>  
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine)
> @@ -355,8 +391,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  
>  		if (IS_GEN6(dev) || IS_GEN7(dev))
>  			dev_priv->uncore.fifo_count =
> -				__raw_i915_read32(dev_priv, GTFIFOCTL) &
> -				GT_FIFO_FREE_ENTRIES_MASK;
> +				fifo_free_entries(dev_priv);
>  	} else {
>  		dev_priv->uncore.forcewake_count = 0;
>  		dev_priv->uncore.fw_rendercount = 0;

It would be best to split out the free_entries cleanup (a good one)
from the vlv bug fix, and also drop the #if 1s.

With that done:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 871c284..6a3dddf 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -47,6 +47,12 @@  assert_device_not_suspended(struct drm_i915_private *dev_priv)
 	     "Device suspended\n");
 }
 
+static inline u32 fifo_free_entries(struct drm_i915_private *dev_priv)
+{
+	u32 count = __raw_i915_read32(dev_priv, GTFIFOCTL);
+	return count & GT_FIFO_FREE_ENTRIES_MASK;
+}
+
 static void __gen6_gt_wait_for_thread_c0(struct drm_i915_private *dev_priv)
 {
 	u32 gt_thread_status_mask;
@@ -154,6 +160,28 @@  static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
 		gen6_gt_check_fifodbg(dev_priv);
 }
 
+static int __vlv_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
+{
+	u32 free = fifo_free_entries(dev_priv);
+	int loop1, loop2;
+
+	for (loop1 = 0; loop1 < 5000 && free < GT_FIFO_NUM_RESERVED_ENTRIES; ) {
+		for (loop2 = 0; loop2 < 1000 && free < GT_FIFO_NUM_RESERVED_ENTRIES; loop2 += 10) {
+			udelay(10);
+			free = fifo_free_entries(dev_priv);
+		}
+		loop1 += loop2;
+		if (loop1 > 1000 || free < 48)
+			DRM_DEBUG("after %d us, the FIFO has %d slots", loop1, free);
+	}
+
+	dev_priv->uncore.fifo_count = free;
+	if (WARN(free < GT_FIFO_NUM_RESERVED_ENTRIES,
+		"FIFO has insufficient space (%d slots)", free))
+		return -1;
+	return 0;
+}
+
 static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
 	int ret = 0;
@@ -161,16 +189,15 @@  static int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 	/* On VLV, FIFO will be shared by both SW and HW.
 	 * So, we need to read the FREE_ENTRIES everytime */
 	if (IS_VALLEYVIEW(dev_priv->dev))
-		dev_priv->uncore.fifo_count =
-			__raw_i915_read32(dev_priv, GTFIFOCTL) &
-						GT_FIFO_FREE_ENTRIES_MASK;
+		return __vlv_gt_wait_for_fifo(dev_priv);
 
 	if (dev_priv->uncore.fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES) {
 		int loop = 500;
-		u32 fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
+		u32 fifo = fifo_free_entries(dev_priv);
+
 		while (fifo <= GT_FIFO_NUM_RESERVED_ENTRIES && loop--) {
 			udelay(10);
-			fifo = __raw_i915_read32(dev_priv, GTFIFOCTL) & GT_FIFO_FREE_ENTRIES_MASK;
+			fifo = fifo_free_entries(dev_priv);
 		}
 		if (WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES))
 			++ret;
@@ -194,6 +221,11 @@  static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
 static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 						int fw_engine)
 {
+#if	1
+	if (__gen6_gt_wait_for_fifo(dev_priv))
+		gen6_gt_check_fifodbg(dev_priv);
+#endif
+
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine) {
 		if (wait_for_atomic((__raw_i915_read32(dev_priv,
@@ -238,6 +270,10 @@  static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
 					int fw_engine)
 {
+#if	1
+	if (__gen6_gt_wait_for_fifo(dev_priv))
+		gen6_gt_check_fifodbg(dev_priv);
+#endif
 
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine)
@@ -355,8 +391,7 @@  static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 
 		if (IS_GEN6(dev) || IS_GEN7(dev))
 			dev_priv->uncore.fifo_count =
-				__raw_i915_read32(dev_priv, GTFIFOCTL) &
-				GT_FIFO_FREE_ENTRIES_MASK;
+				fifo_free_entries(dev_priv);
 	} else {
 		dev_priv->uncore.forcewake_count = 0;
 		dev_priv->uncore.fw_rendercount = 0;