Re: [PATCH] drm/i915: Use ktime on wait_for

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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

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