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

[Chris Wilson]

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.

>  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