[PATCH] drm/etnaviv: correct timeout calculation

[Lucas Stach]

Hi Russell,

Am Freitag, den 09.03.2018, 11:44 +0000 schrieb Russell King - ARM Linux:
> Hi Lucas,
> 
> Please retain my authorship of my patch, which was sent on 23 Oct 2017.
> The patch you have below is 100% identical to that which I sent.

I'll gladly do so if you provide me a proper Signed-off-by for this
patch, which was missing from your 23 Oct 2017 submission.

> You should also point out, as per the follow-on discussion, that using
> clock_gettime() on 32-bit systems will not work once the time it
> reports wraps - so something like the comment I suggested in a follow
> up patch:
> 
> + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies.
> + * We need to calculate the timeout in terms of number of jiffies
> + * between the specified timeout and the current CLOCK_MONOTONIC time.
> + * Note: clock_gettime() is 32-bit on 32-bit arch.  Using 64-bit
> + * timespec math here just means that when a wrap occurs, the
> + * specified timeout goes into the past and we can't request a
> + * timeout in the future: IOW, the code breaks.
> 
> would be sensible either in the commit message or the code.

I'll add it to the commit message, as I think the discussion with Arnd
established that this is a very theoretical risk, not likely to be hit
in practice.

Regards,
Lucas

> On Fri, Mar 09, 2018 at 12:21:49PM +0100, Lucas Stach wrote:
> > The old way did clamp the jiffy conversion and thus caused the timeouts
> > to become negative after some time. Also it didn't work with userspace
> > which actually fills the upper 32bits of the 64bit timestamp value.
> > 
> > Fix this by using the solution developed and tested by Russell.
> > 
> > > > Suggested-by: Russell King <linux at armlinux.org.uk>
> > > > Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_drv.h | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > index ddb17ee565e9..17a43da98fb9 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/slab.h>
> >  #include <linux/list.h>
> > +#include <linux/time64.h>
> >  #include <linux/types.h>
> >  #include <linux/sizes.h>
> >  
> > @@ -132,19 +133,27 @@ static inline bool fence_after_eq(u32 a, u32 b)
> > > >  	return (s32)(a - b) >= 0;
> >  }
> >  
> > +/*
> > + * Etnaviv timeouts are specified wrt CLOCK_MONOTONIC, not jiffies.
> > + * We need to calculate the timeout in terms of number of jiffies
> > + * between the specified timeout and the current CLOCK_MONOTONIC time.
> > + */
> >  static inline unsigned long etnaviv_timeout_to_jiffies(
> > > >  	const struct timespec *timeout)
> >  {
> > > > -	unsigned long timeout_jiffies = timespec_to_jiffies(timeout);
> > > > -	unsigned long start_jiffies = jiffies;
> > > > -	unsigned long remaining_jiffies;
> > > > +	struct timespec64 ts, to;
> > +
> > > > +	to = timespec_to_timespec64(*timeout);
> > +
> > > > +	ktime_get_ts64(&ts);
> > +
> > > > +	/* timeouts before "now" have already expired */
> > > > +	if (timespec64_compare(&to, &ts) <= 0)
> > > > +		return 0;
> >  
> > > > -	if (time_after(start_jiffies, timeout_jiffies))
> > > > -		remaining_jiffies = 0;
> > > > -	else
> > > > -		remaining_jiffies = timeout_jiffies - start_jiffies;
> > > > +	ts = timespec64_sub(to, ts);
> >  
> > > > -	return remaining_jiffies;
> > > > +	return timespec64_to_jiffies(&ts);
> >  }
> >  
> >  #endif /* __ETNAVIV_DRV_H__ */
> > -- 
> > 2.16.1
> > 
> 
>