[Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
Daniel Vetter
daniel.vetter at ffwll.ch
Tue Aug 28 18:00:36 CEST 2012
On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> 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.
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 at ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list