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

[Daniel Vetter]

On Tue, Aug 28, 2012 at 6:07 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> 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?

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 at ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch