[Intel-gfx] [PATCH] drm/i915: Use ktime on wait_for

[Imre Deak]

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 at intel.com>
> > References: https://lkml.org/lkml/2018/4/18/798 [1]
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> 
> 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.

> 
> >  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 at chris-wilson.co.uk>
> 
> Cc stable?
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx