Imre Deak <imre.deak@xxxxxxxxx> writes: > On Fri, Apr 20, 2018 at 11:27:55AM +0100, Chris Wilson wrote: >> Quoting Mika Kuoppala (2018-04-20 10:54:26) >> > We use jiffies to determine when wait expires. However >> > Imre did find out that jiffies can and will do a >1 >> > increments on certain situations [1]. When this happens >> > in a wait_for loop, we return timeout errorneously >> > much earlier than what the real wallclock would say. >> > >> > We can't afford our waits to timeout prematurely. >> > Discard jiffies and change to ktime to detect timeouts. >> > >> > Reported-by: Imre Deak <imre.deak@xxxxxxxxx> >> > References: https://lkml.org/lkml/2018/4/18/798 [1] >> > Cc: Imre Deak <imre.deak@xxxxxxxxx> >> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> >> >> The atomic variant is already jiffie-less (since it has to work in >> irq-off contexts). Maybe a bit tricky to suggest that the callers know >> if jiffie incremens are accurate or not. >> >> What is not clear from the link is whether our wait_for() is running >> across suspend, or whether it is just jiffie recalibration some time >> during resume that breaks. > > The wait_for starts on the resume path, so the jump shouldn't be related > to any of the timekeeping adjustments across suspend/resume (happening > already during syscore resume). It looks like a delayed LAPIC timer > interrupt on that GLK system, trying to get more details on that with > irqsoff ftracing. Both patches pushed. Thanks for the report and reviews. -Mika > >> >> > drivers/gpu/drm/i915/intel_drv.h | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> > index 8b20824e806e..ac7565220aa3 100644 >> > --- a/drivers/gpu/drm/i915/intel_drv.h >> > +++ b/drivers/gpu/drm/i915/intel_drv.h >> > @@ -49,12 +49,12 @@ >> > * check the condition before the timeout. >> > */ >> > #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \ >> > - unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1; \ >> > + const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \ >> > long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \ >> > int ret__; \ >> > might_sleep(); \ >> > for (;;) { \ >> > - bool expired__ = time_after(jiffies, timeout__); \ >> > + const bool expired__ = ktime_after(ktime_get_raw(), end__); \ >> > OP; \ >> > if (COND) { \ >> > ret__ = 0; \ >> >> Nevertheless, the patch is ok and I don't have too much objection to >> adding another tsc (at best, hpet at worst!) read around every mmio+sleep, >> plus expanding the code for the function calls. Out of curiosity what is >> the size delta? How many wait_for() do we have left that we need to >> convert to a function call rather than macro expansion? >> >> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> >> Cc stable? >> -Chris >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
