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
