[PATCH 0/3] Some forcewake fixes

[PATCH 0/3] Some forcewake fixes

[Ben Widawsky]

I have no proof that any of these are relevant other than a designer in the
know told me we should be doing this. Furthermore if Mathew's hang is truly on
the very first read to the FORCEWAKE_ACK, these patches almost surely will
*not* help :-(

Ben Widawsky (3):
  drm/i915: Extract forcewake ack timeout
  drm/i915: Change forcewake timeout to 2ms
  drm/i915: Never read FORCEWAKE

 drivers/gpu/drm/i915/intel_pm.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
1.7.12

[PATCH 1/3] drm/i915: Extract forcewake ack timeout

[Ben Widawsky]

It's used all over the place, and we want to be able to play around with
the value, apparently. Note that it doesn't touch other timeouts of the
same value (like gtfifo, and thread C0 wait).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c0721ff..f42c142 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -31,6 +31,8 @@
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
 
+#define FORCEWAKE_ACK_TIMEOUT_US 500
+
 /* FBC, or Frame Buffer Compression, is a technique employed to compress the
  * framebuffer contents in-memory, aiming at reducing the required bandwidth
  * during in-memory transfers and, therefore, reduce the power packet.
@@ -3968,13 +3970,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 	else
 		forcewake_ack = FORCEWAKE_ACK;
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, 500))
+	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
+			       FORCEWAKE_ACK_TIMEOUT_US))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	I915_WRITE_NOTRACE(FORCEWAKE, 1);
 	POSTING_READ(FORCEWAKE);
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), 500))
+	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
+			       FORCEWAKE_ACK_TIMEOUT_US))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	__gen6_gt_wait_for_thread_c0(dev_priv);
@@ -3989,13 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
 	else
 		forcewake_ack = FORCEWAKE_MT_ACK;
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, 500))
+	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
+			       FORCEWAKE_ACK_TIMEOUT_US))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
 	POSTING_READ(FORCEWAKE_MT);
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), 500))
+	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
+			       FORCEWAKE_ACK_TIMEOUT_US))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	__gen6_gt_wait_for_thread_c0(dev_priv);
@@ -4082,7 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
 	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff);
 	POSTING_READ(FORCEWAKE_VLV);
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), 500))
+	if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
+			       FORCEWAKE_ACK_TIMEOUT_US))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	__gen6_gt_wait_for_thread_c0(dev_priv);
-- 
1.7.12

[PATCH 2/3] drm/i915: Change forcewake timeout to 2ms

[Ben Widawsky]

A designer familiar with the hardware has stated that the forcewake
timeout can theoretically be as high as a little over 1ms. Therefore we
modify our code to use 2ms (appropriate fudge and because we don't want
to round down).

Hopefully this can't prevent spurious timeouts.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f42c142..2a8468d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -31,7 +31,7 @@
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
 
-#define FORCEWAKE_ACK_TIMEOUT_US 500
+#define FORCEWAKE_ACK_TIMEOUT_MS 2
 
 /* FBC, or Frame Buffer Compression, is a technique employed to compress the
  * framebuffer contents in-memory, aiming at reducing the required bandwidth
@@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 	else
 		forcewake_ack = FORCEWAKE_ACK;
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
-			       FORCEWAKE_ACK_TIMEOUT_US))
+	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
+			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	I915_WRITE_NOTRACE(FORCEWAKE, 1);
 	POSTING_READ(FORCEWAKE);
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
-			       FORCEWAKE_ACK_TIMEOUT_US))
+	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
+			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	__gen6_gt_wait_for_thread_c0(dev_priv);
@@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
 	else
 		forcewake_ack = FORCEWAKE_MT_ACK;
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
-			       FORCEWAKE_ACK_TIMEOUT_US))
+	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
+			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
 	POSTING_READ(FORCEWAKE_MT);
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
-			       FORCEWAKE_ACK_TIMEOUT_US))
+	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
+			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	__gen6_gt_wait_for_thread_c0(dev_priv);
@@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
 	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff);
 	POSTING_READ(FORCEWAKE_VLV);
 
-	if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
-			       FORCEWAKE_ACK_TIMEOUT_US))
+	if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
+			    FORCEWAKE_ACK_TIMEOUT_MS))
 		DRM_ERROR("Force wake wait timed out\n");
 
 	__gen6_gt_wait_for_thread_c0(dev_priv);
-- 
1.7.12

[PATCH 3/3] drm/i915: Never read FORCEWAKE

[Ben Widawsky]

The same designer from the previous patch has told us to never read
FORCEWAKE. We only do this for the POSTING_READ(), so simply change that
to something within the same cacheline (for no reason in particular
other than it sounds nice). In the _mt case we can leverage
the gtfifodbg check for the POSTING_READ.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2a8468d..83ec02c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3975,7 +3975,7 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 		DRM_ERROR("Force wake wait timed out\n");
 
 	I915_WRITE_NOTRACE(FORCEWAKE, 1);
-	POSTING_READ(FORCEWAKE);
+	POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */
 
 	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
 			    FORCEWAKE_ACK_TIMEOUT_MS))
@@ -3998,7 +3998,7 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
 		DRM_ERROR("Force wake wait timed out\n");
 
 	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
-	POSTING_READ(FORCEWAKE_MT);
+	POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */
 
 	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
 			    FORCEWAKE_ACK_TIMEOUT_MS))
@@ -4035,14 +4035,12 @@ void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
 static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
-	POSTING_READ(FORCEWAKE);
 	gen6_gt_check_fifodbg(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));
-	POSTING_READ(FORCEWAKE_MT);
 	gen6_gt_check_fifodbg(dev_priv);
 }
 
-- 
1.7.12

Re: [PATCH 3/3] drm/i915: Never read FORCEWAKE

[Daniel Vetter]

On Fri, Aug 24, 2012 at 12:31:11PM -0700, Ben Widawsky wrote:
> The same designer from the previous patch has told us to never read
> FORCEWAKE. We only do this for the POSTING_READ(), so simply change that
> to something within the same cacheline (for no reason in particular
> other than it sounds nice). In the _mt case we can leverage
> the gtfifodbg check for the POSTING_READ.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

This partially reverts

commit 6af2d180f82151cf3d58952e35a4f96e45bc453a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 26 16:24:50 2012 +0200

    drm/i915: fix forcewake related hangs on snb

I guess the commit message should mention that. Also, please add a comment
it the _put functions that the check_fifodbg serves as a posting read (and
that we have experimental evidence suggesting that we should readback
something "nearby").

btw, can you please ask your nice designer whether he has any clue whether
that "nearby" posting_read has an effect?

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2a8468d..83ec02c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3975,7 +3975,7 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	I915_WRITE_NOTRACE(FORCEWAKE, 1);
> -	POSTING_READ(FORCEWAKE);
> +	POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */
>  
>  	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
> @@ -3998,7 +3998,7 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
> -	POSTING_READ(FORCEWAKE_MT);
> +	POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */
>  
>  	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
> @@ -4035,14 +4035,12 @@ void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
>  static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE, 0);
> -	POSTING_READ(FORCEWAKE);
>  	gen6_gt_check_fifodbg(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));
> -	POSTING_READ(FORCEWAKE_MT);
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> -- 
> 1.7.12
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms

[Jani Nikula]

On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
> A designer familiar with the hardware has stated that the forcewake
> timeout can theoretically be as high as a little over 1ms. Therefore we
> modify our code to use 2ms (appropriate fudge and because we don't want
> to round down).
>
> Hopefully this can't prevent spurious timeouts.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f42c142..2a8468d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,7 +31,7 @@
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
>  
> -#define FORCEWAKE_ACK_TIMEOUT_US 500
> +#define FORCEWAKE_ACK_TIMEOUT_MS 2
>  
>  /* FBC, or Frame Buffer Compression, is a technique employed to compress the
>   * framebuffer contents in-memory, aiming at reducing the required bandwidth
> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
>  	else
>  		forcewake_ack = FORCEWAKE_ACK;
>  
> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
> -			       FORCEWAKE_ACK_TIMEOUT_US))
> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
> +			    FORCEWAKE_ACK_TIMEOUT_MS))

Superficially this looks okay, but the implementation of
wait_for_atomic() not so. As a surprise, this change drops cpu_relax()
from the busy loop, even thought the timeout is potentially much longer.

The quick fix here would be to just use 2000 us with
wait_for_atomic_us(), but we should do something about wait_for_atomic()
too. Luckily it's only ever used at one place.

BR,
Jani.


>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	I915_WRITE_NOTRACE(FORCEWAKE, 1);
>  	POSTING_READ(FORCEWAKE);
>  
> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
> -			       FORCEWAKE_ACK_TIMEOUT_US))
> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	__gen6_gt_wait_for_thread_c0(dev_priv);
> @@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
>  	else
>  		forcewake_ack = FORCEWAKE_MT_ACK;
>  
> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
> -			       FORCEWAKE_ACK_TIMEOUT_US))
> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
>  	POSTING_READ(FORCEWAKE_MT);
>  
> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
> -			       FORCEWAKE_ACK_TIMEOUT_US))
> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	__gen6_gt_wait_for_thread_c0(dev_priv);
> @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
>  	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff);
>  	POSTING_READ(FORCEWAKE_VLV);
>  
> -	if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
> -			       FORCEWAKE_ACK_TIMEOUT_US))
> +	if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("Force wake wait timed out\n");
>  
>  	__gen6_gt_wait_for_thread_c0(dev_priv);
> -- 
> 1.7.12
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms

[Ben Widawsky]

On 2012-08-26 23:59, Jani Nikula wrote:
> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
>> A designer familiar with the hardware has stated that the forcewake
>> timeout can theoretically be as high as a little over 1ms. Therefore 
>> we
>> modify our code to use 2ms (appropriate fudge and because we don't 
>> want
>> to round down).
>>
>> Hopefully this can't prevent spurious timeouts.
>>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index f42c142..2a8468d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -31,7 +31,7 @@
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include <linux/module.h>
>>
>> -#define FORCEWAKE_ACK_TIMEOUT_US 500
>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2
>>
>>  /* FBC, or Frame Buffer Compression, is a technique employed to 
>> compress the
>>   * framebuffer contents in-memory, aiming at reducing the required 
>> bandwidth
>> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct 
>> drm_i915_private *dev_priv)
>>  	else
>>  		forcewake_ack = FORCEWAKE_ACK;
>>
>> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 
>> 0,
>> -			       FORCEWAKE_ACK_TIMEOUT_US))
>> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
>> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>
> Superficially this looks okay, but the implementation of
> wait_for_atomic() not so. As a surprise, this change drops 
> cpu_relax()
> from the busy loop, even thought the timeout is potentially much 
> longer.
>
> The quick fix here would be to just use 2000 us with
> wait_for_atomic_us(), but we should do something about 
> wait_for_atomic()
> too. Luckily it's only ever used at one place.
>
> BR,
> Jani.

Hmm, dare I say, I think this is a bug in _wait_for. Without spending 
too much brain power on this, I believe the compiler can screw us over 
here. No matter the bug, cpu_relax is still probably desirable, unless 
there is some newer coolness here? I shall insert a patch before this to 
do the cpu_relax in _wait_for.
Nice catch.
Ben

>
>
>>  		DRM_ERROR("Force wake wait timed out\n");
>>
>>  	I915_WRITE_NOTRACE(FORCEWAKE, 1);
>>  	POSTING_READ(FORCEWAKE);
>>
>> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
>> -			       FORCEWAKE_ACK_TIMEOUT_US))
>> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
>> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>>  		DRM_ERROR("Force wake wait timed out\n");
>>
>>  	__gen6_gt_wait_for_thread_c0(dev_priv);
>> @@ -3993,15 +3993,15 @@ static void 
>> __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
>>  	else
>>  		forcewake_ack = FORCEWAKE_MT_ACK;
>>
>> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 
>> 0,
>> -			       FORCEWAKE_ACK_TIMEOUT_US))
>> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
>> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>>  		DRM_ERROR("Force wake wait timed out\n");
>>
>>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
>>  	POSTING_READ(FORCEWAKE_MT);
>>
>> -	if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1),
>> -			       FORCEWAKE_ACK_TIMEOUT_US))
>> +	if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1),
>> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>>  		DRM_ERROR("Force wake wait timed out\n");
>>
>>  	__gen6_gt_wait_for_thread_c0(dev_priv);
>> @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct 
>> drm_i915_private *dev_priv)
>>  	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff);
>>  	POSTING_READ(FORCEWAKE_VLV);
>>
>> -	if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
>> -			       FORCEWAKE_ACK_TIMEOUT_US))
>> +	if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1),
>> +			    FORCEWAKE_ACK_TIMEOUT_MS))
>>  		DRM_ERROR("Force wake wait timed out\n");
>>
>>  	__gen6_gt_wait_for_thread_c0(dev_priv);
>> --
>> 1.7.12
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms

[Daniel Vetter]

On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 2012-08-26 23:59, Jani Nikula wrote:
>>
>> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
>>>
>>> A designer familiar with the hardware has stated that the forcewake
>>> timeout can theoretically be as high as a little over 1ms. Therefore we
>>> modify our code to use 2ms (appropriate fudge and because we don't want
>>> to round down).
>>>
>>> Hopefully this can't prevent spurious timeouts.
>>>
>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>> ---
>>>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>> b/drivers/gpu/drm/i915/intel_pm.c
>>> index f42c142..2a8468d 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -31,7 +31,7 @@
>>>  #include "../../../platform/x86/intel_ips.h"
>>>  #include <linux/module.h>
>>>
>>> -#define FORCEWAKE_ACK_TIMEOUT_US 500
>>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2
>>>
>>>  /* FBC, or Frame Buffer Compression, is a technique employed to compress
>>> the
>>>   * framebuffer contents in-memory, aiming at reducing the required
>>> bandwidth
>>> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct
>>> drm_i915_private *dev_priv)
>>>         else
>>>                 forcewake_ack = FORCEWAKE_ACK;
>>>
>>> -       if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) ==
>>> 0,
>>> -                              FORCEWAKE_ACK_TIMEOUT_US))
>>> +       if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0,
>>> +                           FORCEWAKE_ACK_TIMEOUT_MS))
>>
>>
>> Superficially this looks okay, but the implementation of
>> wait_for_atomic() not so. As a surprise, this change drops cpu_relax()
>> from the busy loop, even thought the timeout is potentially much longer.
>>
>> The quick fix here would be to just use 2000 us with
>> wait_for_atomic_us(), but we should do something about wait_for_atomic()
>> too. Luckily it's only ever used at one place.
>>
>> BR,
>> Jani.
>
>
> Hmm, dare I say, I think this is a bug in _wait_for. Without spending too
> much brain power on this, I believe the compiler can screw us over here. No
> matter the bug, cpu_relax is still probably desirable, unless there is some
> newer coolness here? I shall insert a patch before this to do the cpu_relax
> in _wait_for.

The original idea behind wiat_for_us was that we use udelay and really
limit ourselves to that us timeout (since jiffies is too coarse). Now
that the timeout for forcewake is 2ms (gawk!) I think we can stop
bothering to pretend that this should timeout quickly and drop the _us
variant (but still keep the cpu relax imo).
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms

[Ben Widawsky]

On 2012-08-28 09:00, Daniel Vetter wrote:
> On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben@bwidawsk.net> 
> wrote:
>> On 2012-08-26 23:59, Jani Nikula wrote:
>>>
>>> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
>>>>
>>>> A designer familiar with the hardware has stated that the 
>>>> forcewake
>>>> timeout can theoretically be as high as a little over 1ms. 
>>>> Therefore we
>>>> modify our code to use 2ms (appropriate fudge and because we don't 
>>>> want
>>>> to round down).
>>>>
>>>> Hopefully this can't prevent spurious timeouts.
>>>>
>>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>>> b/drivers/gpu/drm/i915/intel_pm.c
>>>> index f42c142..2a8468d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -31,7 +31,7 @@
>>>>  #include "../../../platform/x86/intel_ips.h"
>>>>  #include <linux/module.h>
>>>>
>>>> -#define FORCEWAKE_ACK_TIMEOUT_US 500
>>>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2
>>>>
>>>>  /* FBC, or Frame Buffer Compression, is a technique employed to 
>>>> compress
>>>> the
>>>>   * framebuffer contents in-memory, aiming at reducing the 
>>>> required
>>>> bandwidth
>>>> @@ -3970,15 +3970,15 @@ static void 
>>>> __gen6_gt_force_wake_get(struct
>>>> drm_i915_private *dev_priv)
>>>>         else
>>>>                 forcewake_ack = FORCEWAKE_ACK;
>>>>
>>>> -       if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 
>>>> 1) ==
>>>> 0,
>>>> -                              FORCEWAKE_ACK_TIMEOUT_US))
>>>> +       if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) 
>>>> == 0,
>>>> +                           FORCEWAKE_ACK_TIMEOUT_MS))
>>>
>>>
>>> Superficially this looks okay, but the implementation of
>>> wait_for_atomic() not so. As a surprise, this change drops 
>>> cpu_relax()
>>> from the busy loop, even thought the timeout is potentially much 
>>> longer.
>>>
>>> The quick fix here would be to just use 2000 us with
>>> wait_for_atomic_us(), but we should do something about 
>>> wait_for_atomic()
>>> too. Luckily it's only ever used at one place.
>>>
>>> BR,
>>> Jani.
>>
>>
>> Hmm, dare I say, I think this is a bug in _wait_for. Without 
>> spending too
>> much brain power on this, I believe the compiler can screw us over 
>> here. No
>> matter the bug, cpu_relax is still probably desirable, unless there 
>> is some
>> newer coolness here? I shall insert a patch before this to do the 
>> cpu_relax
>> in _wait_for.
>
> The original idea behind wiat_for_us was that we use udelay and 
> really
> limit ourselves to that us timeout (since jiffies is too coarse). Now
> that the timeout for forcewake is 2ms (gawk!) I think we can stop
> bothering to pretend that this should timeout quickly and drop the 
> _us
> variant (but still keep the cpu relax imo).
> -Daniel

Unless I'm confused, you're acking what I was planning?

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms

[Daniel Vetter]

On Tue, Aug 28, 2012 at 6:07 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On 2012-08-28 09:00, Daniel Vetter wrote:
>>
>> On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>>>
>>> On 2012-08-26 23:59, Jani Nikula wrote:
>>>>
>>>>
>>>> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
>>>>>
>>>>>
>>>>> A designer familiar with the hardware has stated that the forcewake
>>>>> timeout can theoretically be as high as a little over 1ms. Therefore we
>>>>> modify our code to use 2ms (appropriate fudge and because we don't want
>>>>> to round down).
>>>>>
>>>>> Hopefully this can't prevent spurious timeouts.
>>>>>
>>>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>>>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>>>> b/drivers/gpu/drm/i915/intel_pm.c
>>>>> index f42c142..2a8468d 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>> @@ -31,7 +31,7 @@
>>>>>  #include "../../../platform/x86/intel_ips.h"
>>>>>  #include <linux/module.h>
>>>>>
>>>>> -#define FORCEWAKE_ACK_TIMEOUT_US 500
>>>>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2
>>>>>
>>>>>  /* FBC, or Frame Buffer Compression, is a technique employed to
>>>>> compress
>>>>> the
>>>>>   * framebuffer contents in-memory, aiming at reducing the required
>>>>> bandwidth
>>>>> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct
>>>>> drm_i915_private *dev_priv)
>>>>>         else
>>>>>                 forcewake_ack = FORCEWAKE_ACK;
>>>>>
>>>>> -       if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1)
>>>>> ==
>>>>> 0,
>>>>> -                              FORCEWAKE_ACK_TIMEOUT_US))
>>>>> +       if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) ==
>>>>> 0,
>>>>> +                           FORCEWAKE_ACK_TIMEOUT_MS))
>>>>
>>>>
>>>>
>>>> Superficially this looks okay, but the implementation of
>>>> wait_for_atomic() not so. As a surprise, this change drops cpu_relax()
>>>> from the busy loop, even thought the timeout is potentially much longer.
>>>>
>>>> The quick fix here would be to just use 2000 us with
>>>> wait_for_atomic_us(), but we should do something about wait_for_atomic()
>>>> too. Luckily it's only ever used at one place.
>>>>
>>>> BR,
>>>> Jani.
>>>
>>>
>>>
>>> Hmm, dare I say, I think this is a bug in _wait_for. Without spending too
>>> much brain power on this, I believe the compiler can screw us over here.
>>> No
>>> matter the bug, cpu_relax is still probably desirable, unless there is
>>> some
>>> newer coolness here? I shall insert a patch before this to do the
>>> cpu_relax
>>> in _wait_for.
>>
>>
>> The original idea behind wiat_for_us was that we use udelay and really
>> limit ourselves to that us timeout (since jiffies is too coarse). Now
>> that the timeout for forcewake is 2ms (gawk!) I think we can stop
>> bothering to pretend that this should timeout quickly and drop the _us
>> variant (but still keep the cpu relax imo).
>> -Daniel
>
>
> Unless I'm confused, you're acking what I was planning?

If what you're planing is to fix up wait_for_atomic to look like
wait_for_us and the ditch the _us variant, yep, acked.
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch