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
