[Intel-gfx] [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 at 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 at 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center