[Intel-gfx] [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 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

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

-- 
Ben Widawsky, Intel Open Source Technology Center