[PATCH] drm/i915: compute wait_ioctl timeout correctly

[PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

We've lost the +1 required for correct timeouts in

commit 5ed0bdf21a85d78e04f89f15ccf227562177cbd9
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Wed Jul 16 21:05:06 2014 +0000

    drm: i915: Use nsec based interfaces

    Use ktime_get_raw_ns() and get rid of the back and forth timespec
    conversions.

    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Signed-off-by: John Stultz <john.stultz@linaro.org>

So fix this up by reinstating our handrolled _timeout function. While
at it bother with handling MAX_JIFFIES.

v2: Convert to usecs (we don't care about the accuracy anyway) first
to avoid overflow issues Dave Gordon spotted.

Cc: Dave Gordon <david.s.gordon@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82749
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++
 drivers/gpu/drm/i915/i915_gem.c |  3 ++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 049482f5d9ac..ef1f00e0a7b3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3097,6 +3097,17 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
 	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
 }
 
+static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
+{
+	u64 usecs = div_u64(m + 999, 1000);
+	unsigned long j = usecs_to_jiffies(usecs);
+
+	if (usecs > (u64)jiffies_to_usecs(MAX_JIFFY_OFFSET))
+		return MAX_JIFFY_OFFSET;
+
+	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
+}
+
 static inline unsigned long
 timespec_to_jiffies_timeout(const struct timespec *value)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9d362d320d82..04a9f26407ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1226,7 +1226,8 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 	if (i915_gem_request_completed(req, true))
 		return 0;
 
-	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
+	timeout_expire = timeout ?
+		jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
 
 	if (INTEL_INFO(dev)->gen >= 6 && ring->id == RCS && can_wait_boost(file_priv)) {
 		gen6_rps_boost(dev_priv);
-- 
1.9.3

[PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

We've lost the +1 required for correct timeouts in

commit 5ed0bdf21a85d78e04f89f15ccf227562177cbd9
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Wed Jul 16 21:05:06 2014 +0000

    drm: i915: Use nsec based interfaces

    Use ktime_get_raw_ns() and get rid of the back and forth timespec
    conversions.

    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Signed-off-by: John Stultz <john.stultz@linaro.org>

So fix this up by reinstating our handrolled _timeout function. While
at it bother with handling MAX_JIFFIES.

v2: Convert to usecs (we don't care about the accuracy anyway) first
to avoid overflow issues Dave Gordon spotted.

v3: Drop the explicit MAX_JIFFY_OFFSET check, usecs_to_jiffies should
take care of that already. It might be a bit too enthusiastic about it
though.

Cc: Dave Gordon <david.s.gordon@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82749
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++
 drivers/gpu/drm/i915/i915_gem.c | 3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 049482f5d9ac..4ea14a8c31f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3097,6 +3097,14 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
 	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
 }
 
+static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
+{
+	u64 usecs = div_u64(m + 999, 1000);
+	unsigned long j = usecs_to_jiffies(usecs);
+
+	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
+}
+
 static inline unsigned long
 timespec_to_jiffies_timeout(const struct timespec *value)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9d362d320d82..04a9f26407ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1226,7 +1226,8 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 	if (i915_gem_request_completed(req, true))
 		return 0;
 
-	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
+	timeout_expire = timeout ?
+		jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
 
 	if (INTEL_INFO(dev)->gen >= 6 && ring->id == RCS && can_wait_boost(file_priv)) {
 		gen6_rps_boost(dev_priv);
-- 
1.9.3

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Chris Wilson]

On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
> We've lost the +1 required for correct timeouts in
> 
> commit 5ed0bdf21a85d78e04f89f15ccf227562177cbd9
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Wed Jul 16 21:05:06 2014 +0000
> 
>     drm: i915: Use nsec based interfaces
> 
>     Use ktime_get_raw_ns() and get rid of the back and forth timespec
>     conversions.
> 
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>     Signed-off-by: John Stultz <john.stultz@linaro.org>
> 
> So fix this up by reinstating our handrolled _timeout function. While
> at it bother with handling MAX_JIFFIES.
> 
> v2: Convert to usecs (we don't care about the accuracy anyway) first
> to avoid overflow issues Dave Gordon spotted.
> 
> v3: Drop the explicit MAX_JIFFY_OFFSET check, usecs_to_jiffies should
> take care of that already. It might be a bit too enthusiastic about it
> though.
> 
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82749
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++
>  drivers/gpu/drm/i915/i915_gem.c | 3 ++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 049482f5d9ac..4ea14a8c31f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3097,6 +3097,14 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
> +{
> +	u64 usecs = div_u64(m + 999, 1000);
> +	unsigned long j = usecs_to_jiffies(usecs);
> +
> +	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);

Or more concisely and review friendly:

static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
{
	return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[John Stultz]

On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
>> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
>> +{
>> +     u64 usecs = div_u64(m + 999, 1000);
>> +     unsigned long j = usecs_to_jiffies(usecs);
>> +
>> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>
> Or more concisely and review friendly:
>
> static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> {
>         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> }

Yea. This looks much nicer. Seems generic enough it might be better
added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
rather then in a driver header.

And clearly the header comment in nsec_to_jiffies() warning its only
for the scheduler and not for use for drivers (for exactly the reason
of this patch) are not obvious/memorable enough for me and Thomas
makes me wonder if we should change its name to be more clear that its
a sched only function.

thanks
-john

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
> On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
> >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
> >> +{
> >> +     u64 usecs = div_u64(m + 999, 1000);
> >> +     unsigned long j = usecs_to_jiffies(usecs);
> >> +
> >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> >
> > Or more concisely and review friendly:
> >
> > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> > {
> >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> > }
> 
> Yea. This looks much nicer. Seems generic enough it might be better
> added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
> rather then in a driver header.
> 
> And clearly the header comment in nsec_to_jiffies() warning its only
> for the scheduler and not for use for drivers (for exactly the reason
> of this patch) are not obvious/memorable enough for me and Thomas
> makes me wonder if we should change its name to be more clear that its
> a sched only function.

This bug here isn't about nsect_to_jiffies vs the 64 bit variant, but
about the +1 that we need to not have a short sleep. In i915 we have a
bunch of jiffies_timeout functions which do just the +1 compared to the
versions in time.c because we screwed this up too often.

Iirc I did float an rfc to move these to time.c once but it resulted in
some bikeshed fest (no, I'm not going to audit every single user of
existing _to_jiffies functions). If there's interest I could try again,
the i915 versions are in drivers/gpu/drm/i915/i915_drv.h.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Imre Deak]

On Wed, 2014-12-03 at 10:22 +0100, Daniel Vetter wrote:
> On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
> > On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
> > >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
> > >> +{
> > >> +     u64 usecs = div_u64(m + 999, 1000);
> > >> +     unsigned long j = usecs_to_jiffies(usecs);
> > >> +
> > >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> > >
> > > Or more concisely and review friendly:
> > >
> > > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> > > {
> > >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> > > }
> > 
> > Yea. This looks much nicer. Seems generic enough it might be better
> > added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
> > rather then in a driver header.
> > 
> > And clearly the header comment in nsec_to_jiffies() warning its only
> > for the scheduler and not for use for drivers (for exactly the reason
> > of this patch) are not obvious/memorable enough for me and Thomas
> > makes me wonder if we should change its name to be more clear that its
> > a sched only function.
> 
> This bug here isn't about nsect_to_jiffies vs the 64 bit variant, but
> about the +1 that we need to not have a short sleep. In i915 we have a
> bunch of jiffies_timeout functions which do just the +1 compared to the
> versions in time.c because we screwed this up too often.
> 
> Iirc I did float an rfc to move these to time.c once but it resulted in
> some bikeshed fest (no, I'm not going to audit every single user of
> existing _to_jiffies functions). If there's interest I could try again,
> the i915 versions are in drivers/gpu/drm/i915/i915_drv.h.

There was at least this attempt:
https://lkml.org/lkml/2013/5/10/187

--Imre

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
> On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
> >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
> >> +{
> >> +     u64 usecs = div_u64(m + 999, 1000);
> >> +     unsigned long j = usecs_to_jiffies(usecs);
> >> +
> >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> >
> > Or more concisely and review friendly:
> >
> > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> > {
> >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> > }
> 
> Yea. This looks much nicer. Seems generic enough it might be better
> added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
> rather then in a driver header.

Ok, that needs an EXPORT_SYMBOL for nsecs_to_jiffies64. Can I count your
"Yea" above as an ack for adding that and pulling it in through
drm-intel.git?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[John Stultz]

On Wed, Dec 3, 2014 at 6:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
>> On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
>> >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
>> >> +{
>> >> +     u64 usecs = div_u64(m + 999, 1000);
>> >> +     unsigned long j = usecs_to_jiffies(usecs);
>> >> +
>> >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>> >
>> > Or more concisely and review friendly:
>> >
>> > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
>> > {
>> >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
>> > }
>>
>> Yea. This looks much nicer. Seems generic enough it might be better
>> added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
>> rather then in a driver header.
>
> Ok, that needs an EXPORT_SYMBOL for nsecs_to_jiffies64. Can I count your
> "Yea" above as an ack for adding that and pulling it in through
> drm-intel.git?

Do you need an EXPORT_SYMBOL if you add the _timeout version next to
nsecs_to_jiffies64 in time.c?

Otherwise no objections to the approach, but I'd like to properly do
an Acked-by: after I see the patch. :)

thanks
-john

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

On Wed, Dec 03, 2014 at 11:07:08AM -0800, John Stultz wrote:
> On Wed, Dec 3, 2014 at 6:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
> >> On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
> >> >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
> >> >> +{
> >> >> +     u64 usecs = div_u64(m + 999, 1000);
> >> >> +     unsigned long j = usecs_to_jiffies(usecs);
> >> >> +
> >> >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> >> >
> >> > Or more concisely and review friendly:
> >> >
> >> > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> >> > {
> >> >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> >> > }
> >>
> >> Yea. This looks much nicer. Seems generic enough it might be better
> >> added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
> >> rather then in a driver header.
> >
> > Ok, that needs an EXPORT_SYMBOL for nsecs_to_jiffies64. Can I count your
> > "Yea" above as an ack for adding that and pulling it in through
> > drm-intel.git?
> 
> Do you need an EXPORT_SYMBOL if you add the _timeout version next to
> nsecs_to_jiffies64 in time.c?

I wouldn't but the patch from Imre to add all the _timeout was killed with
a few bikesheds so really not volunteering. And just moving this single
one doesn't make a lot of sense imo. Also the next patch I'll do is just
add the +1 that we lost to the code and call it a day, really ;-)

> Otherwise no objections to the approach, but I'd like to properly do
> an Acked-by: after I see the patch. :)

I'll send it out.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[John Stultz]

On Thu, Dec 4, 2014 at 2:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 03, 2014 at 11:07:08AM -0800, John Stultz wrote:
>> On Wed, Dec 3, 2014 at 6:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
>> >> On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >> > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
>> >> >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
>> >> >> +{
>> >> >> +     u64 usecs = div_u64(m + 999, 1000);
>> >> >> +     unsigned long j = usecs_to_jiffies(usecs);
>> >> >> +
>> >> >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>> >> >
>> >> > Or more concisely and review friendly:
>> >> >
>> >> > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
>> >> > {
>> >> >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
>> >> > }
>> >>
>> >> Yea. This looks much nicer. Seems generic enough it might be better
>> >> added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
>> >> rather then in a driver header.
>> >
>> > Ok, that needs an EXPORT_SYMBOL for nsecs_to_jiffies64. Can I count your
>> > "Yea" above as an ack for adding that and pulling it in through
>> > drm-intel.git?
>>
>> Do you need an EXPORT_SYMBOL if you add the _timeout version next to
>> nsecs_to_jiffies64 in time.c?
>
> I wouldn't but the patch from Imre to add all the _timeout was killed with
> a few bikesheds so really not volunteering. And just moving this single
> one doesn't make a lot of sense imo. Also the next patch I'll do is just
> add the +1 that we lost to the code and call it a day, really ;-)
>

Sigh. So you're going to make me write a separate patch that moves it over?

I know you'll probably say this is bikeshedding, but the reason why
avoiding the EXPORT_SYMBOL on nsec_to_jiffies would be good is because
nsec_to_jiffies explicitly states in the comments that its not for any
use but the scheduler.

But still, I do see our change broke you here, so I'm not going to object.

thanks
-john

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

On Thu, Dec 4, 2014 at 6:42 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Dec 4, 2014 at 2:42 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Dec 03, 2014 at 11:07:08AM -0800, John Stultz wrote:
>>> On Wed, Dec 3, 2014 at 6:30 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> > On Tue, Dec 02, 2014 at 08:54:13AM -0800, John Stultz wrote:
>>> >> On Tue, Dec 2, 2014 at 8:35 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>> >> > On Tue, Dec 02, 2014 at 04:36:22PM +0100, Daniel Vetter wrote:
>>> >> >> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 m)
>>> >> >> +{
>>> >> >> +     u64 usecs = div_u64(m + 999, 1000);
>>> >> >> +     unsigned long j = usecs_to_jiffies(usecs);
>>> >> >> +
>>> >> >> +     return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>>> >> >
>>> >> > Or more concisely and review friendly:
>>> >> >
>>> >> > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
>>> >> > {
>>> >> >         return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
>>> >> > }
>>> >>
>>> >> Yea. This looks much nicer. Seems generic enough it might be better
>>> >> added next to nsec_to_jiffies64() in kernel/time/time.c or jiffies.h
>>> >> rather then in a driver header.
>>> >
>>> > Ok, that needs an EXPORT_SYMBOL for nsecs_to_jiffies64. Can I count your
>>> > "Yea" above as an ack for adding that and pulling it in through
>>> > drm-intel.git?
>>>
>>> Do you need an EXPORT_SYMBOL if you add the _timeout version next to
>>> nsecs_to_jiffies64 in time.c?
>>
>> I wouldn't but the patch from Imre to add all the _timeout was killed with
>> a few bikesheds so really not volunteering. And just moving this single
>> one doesn't make a lot of sense imo. Also the next patch I'll do is just
>> add the +1 that we lost to the code and call it a day, really ;-)
>>
>
> Sigh. So you're going to make me write a separate patch that moves it over?

We've written it already, Imre posted the link to the old discussion:

https://lkml.org/lkml/2013/5/10/187

But if the first attempt doesn't sufficiently stick I tend to chase
the patches any more. But if you want to resurrect this I could ping
Imre and ask him to pick it up again or you could rebase his patches.

> I know you'll probably say this is bikeshedding, but the reason why
> avoiding the EXPORT_SYMBOL on nsec_to_jiffies would be good is because
> nsec_to_jiffies explicitly states in the comments that its not for any
> use but the scheduler.

Well I only export the 64 variant, the other one (with the too small
return type which would overflow) is already exported with

commit d560fed6abe0f9975b509e4fb824e08ac19adc93
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Wed Jul 16 21:04:31 2014 +0000

    time: Export nsecs_to_jiffies()

    Required for moving drivers to the nanosecond based interfaces.

    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: John Stultz <john.stultz@linaro.org>

So I've figured this is ok.

> But still, I do see our change broke you here, so I'm not going to object.

Ok, thanks I'll pull this in through drm-intel for 3.19 (3.18 is kinda
done already I guess) with cc: stable.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[John Stultz]

On Thu, Dec 4, 2014 at 9:50 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Thu, Dec 4, 2014 at 6:42 PM, John Stultz <john.stultz@linaro.org> wrote:
>> Sigh. So you're going to make me write a separate patch that moves it over?
>
> We've written it already, Imre posted the link to the old discussion:
>
> https://lkml.org/lkml/2013/5/10/187
>
> But if the first attempt doesn't sufficiently stick I tend to chase
> the patches any more. But if you want to resurrect this I could ping
> Imre and ask him to pick it up again or you could rebase his patches.

Well, last I saw the initial patch was buggy, no? I don't think I saw
it being resubmitted.


>> But still, I do see our change broke you here, so I'm not going to object.
>
> Ok, thanks I'll pull this in through drm-intel for 3.19 (3.18 is kinda
> done already I guess) with cc: stable.

You probably should submit it for 3.18 and let Linus decide if its too
late. I've already gotten yelled at by Ingo for pushing patches in the
merge window that cc stable. Even if its out of a desire to let the
patches get wider testing, its something of a hot-button item for
folks. :)

thanks
-john

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

On Thu, Dec 4, 2014 at 7:16 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Dec 4, 2014 at 9:50 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> On Thu, Dec 4, 2014 at 6:42 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> Sigh. So you're going to make me write a separate patch that moves it over?
>>
>> We've written it already, Imre posted the link to the old discussion:
>>
>> https://lkml.org/lkml/2013/5/10/187
>>
>> But if the first attempt doesn't sufficiently stick I tend to chase
>> the patches any more. But if you want to resurrect this I could ping
>> Imre and ask him to pick it up again or you could rebase his patches.
>
> Well, last I saw the initial patch was buggy, no? I don't think I saw
> it being resubmitted.

I didn't see your reply in that thread nor in the v2 follow up at
http://marc.info/?l=linux-kernel&m=136854294730957&w=2 Maybe I missed
it, but response seems to have been lukewarm overall.

>>> But still, I do see our change broke you here, so I'm not going to object.
>>
>> Ok, thanks I'll pull this in through drm-intel for 3.19 (3.18 is kinda
>> done already I guess) with cc: stable.
>
> You probably should submit it for 3.18 and let Linus decide if its too
> late. I've already gotten yelled at by Ingo for pushing patches in the
> merge window that cc stable. Even if its out of a desire to let the
> patches get wider testing, its something of a hot-button item for
> folks. :)

Oh I know, but if you count your regression rate in bugs-per-day you
end up with different standards ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[John Stultz]

On Thu, Dec 4, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Thu, Dec 4, 2014 at 7:16 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Thu, Dec 4, 2014 at 9:50 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> On Thu, Dec 4, 2014 at 6:42 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> Sigh. So you're going to make me write a separate patch that moves it over?
>>>
>>> We've written it already, Imre posted the link to the old discussion:
>>>
>>> https://lkml.org/lkml/2013/5/10/187
>>>
>>> But if the first attempt doesn't sufficiently stick I tend to chase
>>> the patches any more. But if you want to resurrect this I could ping
>>> Imre and ask him to pick it up again or you could rebase his patches.
>>
>> Well, last I saw the initial patch was buggy, no? I don't think I saw
>> it being resubmitted.
>
> I didn't see your reply in that thread nor in the v2 follow up at
> http://marc.info/?l=linux-kernel&m=136854294730957&w=2 Maybe I missed
> it, but response seems to have been lukewarm overall.

Ok, I wasn't cc'ed on the v2, thanks for the pointer.  There's some
general lukewarmness to all things jiffies, since getting rid of them
has been a long term goal forever. But overall that patch set seemed
ok (though I'm not a fan of macro generation of functions). But minor
details..

thanks
-john

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

On Thu, Dec 04, 2014 at 12:35:44PM -0800, John Stultz wrote:
> On Thu, Dec 4, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > On Thu, Dec 4, 2014 at 7:16 PM, John Stultz <john.stultz@linaro.org> wrote:
> >> On Thu, Dec 4, 2014 at 9:50 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >>> On Thu, Dec 4, 2014 at 6:42 PM, John Stultz <john.stultz@linaro.org> wrote:
> >>>> Sigh. So you're going to make me write a separate patch that moves it over?
> >>>
> >>> We've written it already, Imre posted the link to the old discussion:
> >>>
> >>> https://lkml.org/lkml/2013/5/10/187
> >>>
> >>> But if the first attempt doesn't sufficiently stick I tend to chase
> >>> the patches any more. But if you want to resurrect this I could ping
> >>> Imre and ask him to pick it up again or you could rebase his patches.
> >>
> >> Well, last I saw the initial patch was buggy, no? I don't think I saw
> >> it being resubmitted.
> >
> > I didn't see your reply in that thread nor in the v2 follow up at
> > http://marc.info/?l=linux-kernel&m=136854294730957&w=2 Maybe I missed
> > it, but response seems to have been lukewarm overall.
> 
> Ok, I wasn't cc'ed on the v2, thanks for the pointer.  There's some
> general lukewarmness to all things jiffies, since getting rid of them
> has been a long term goal forever. But overall that patch set seemed
> ok (though I'm not a fan of macro generation of functions). But minor
> details..

btw have you seen the other fallout from the ktime->nsec conversion in
i915?

http://www.spinics.net/lists/intel-gfx/msg56445.html

Is this just the inaccuracy of nsec_to_jiffies (and why it explicitly
states that this is for the scheduler only) or is there some bigger fish
in there?

Insight very much appreciated.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: compute wait_ioctl timeout correctly

[Daniel Vetter]

We've lost the +1 required for correct timeouts in

commit 5ed0bdf21a85d78e04f89f15ccf227562177cbd9
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Wed Jul 16 21:05:06 2014 +0000

    drm: i915: Use nsec based interfaces

    Use ktime_get_raw_ns() and get rid of the back and forth timespec
    conversions.

    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Signed-off-by: John Stultz <john.stultz@linaro.org>

So fix this up by reinstating our handrolled _timeout function. While
at it bother with handling MAX_JIFFIES.

v2: Convert to usecs (we don't care about the accuracy anyway) first
to avoid overflow issues Dave Gordon spotted.

v3: Drop the explicit MAX_JIFFY_OFFSET check, usecs_to_jiffies should
take care of that already. It might be a bit too enthusiastic about it
though.

v4: Chris has a much nicer color, so use his implementation.

This requires to export nsec_to_jiffies from time.c.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Gordon <david.s.gordon@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82749
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 5 +++++
 drivers/gpu/drm/i915/i915_gem.c | 3 ++-
 kernel/time/time.c              | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 049482f5d9ac..564a45f4a0ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3097,6 +3097,11 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
 	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
 }
 
+static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
+{
+        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
+}
+
 static inline unsigned long
 timespec_to_jiffies_timeout(const struct timespec *value)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9d362d320d82..04a9f26407ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1226,7 +1226,8 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
 	if (i915_gem_request_completed(req, true))
 		return 0;
 
-	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
+	timeout_expire = timeout ?
+		jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
 
 	if (INTEL_INFO(dev)->gen >= 6 && ring->id == RCS && can_wait_boost(file_priv)) {
 		gen6_rps_boost(dev_priv);
diff --git a/kernel/time/time.c b/kernel/time/time.c
index a9ae20fb0b11..8fae82ca5cbf 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -745,6 +745,7 @@ u64 nsecs_to_jiffies64(u64 n)
 	return div_u64(n * 9, (9ull * NSEC_PER_SEC + HZ / 2) / HZ);
 #endif
 }
+EXPORT_SYMBOL(nsecs_to_jiffies64);
 
 /**
  * nsecs_to_jiffies - Convert nsecs in u64 to jiffies
-- 
1.9.3

Re: [PATCH] drm/i915: compute wait_ioctl timeout correctly

[John Stultz]

On Thu, Dec 4, 2014 at 2:12 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We've lost the +1 required for correct timeouts in
>
> commit 5ed0bdf21a85d78e04f89f15ccf227562177cbd9
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Wed Jul 16 21:05:06 2014 +0000
>
>     drm: i915: Use nsec based interfaces
>
>     Use ktime_get_raw_ns() and get rid of the back and forth timespec
>     conversions.
>
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>     Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> So fix this up by reinstating our handrolled _timeout function. While
> at it bother with handling MAX_JIFFIES.
>
> v2: Convert to usecs (we don't care about the accuracy anyway) first
> to avoid overflow issues Dave Gordon spotted.
>
> v3: Drop the explicit MAX_JIFFY_OFFSET check, usecs_to_jiffies should
> take care of that already. It might be a bit too enthusiastic about it
> though.
>
> v4: Chris has a much nicer color, so use his implementation.
>
> This requires to export nsec_to_jiffies from time.c.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82749
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 5 +++++
>  drivers/gpu/drm/i915/i915_gem.c | 3 ++-
>  kernel/time/time.c              | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 049482f5d9ac..564a45f4a0ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3097,6 +3097,11 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
>         return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>
> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> +{
> +        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> +}
> +
>  static inline unsigned long
>  timespec_to_jiffies_timeout(const struct timespec *value)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9d362d320d82..04a9f26407ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1226,7 +1226,8 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>         if (i915_gem_request_completed(req, true))
>                 return 0;
>
> -       timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
> +       timeout_expire = timeout ?
> +               jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
>
>         if (INTEL_INFO(dev)->gen >= 6 && ring->id == RCS && can_wait_boost(file_priv)) {
>                 gen6_rps_boost(dev_priv);
> diff --git a/kernel/time/time.c b/kernel/time/time.c
> index a9ae20fb0b11..8fae82ca5cbf 100644
> --- a/kernel/time/time.c
> +++ b/kernel/time/time.c
> @@ -745,6 +745,7 @@ u64 nsecs_to_jiffies64(u64 n)
>         return div_u64(n * 9, (9ull * NSEC_PER_SEC + HZ / 2) / HZ);
>  #endif
>  }
> +EXPORT_SYMBOL(nsecs_to_jiffies64);


For the sake of the fix,
Acked-by: John Stultz <john.stultz@linaro.org>

But I'll likely follow this (eventually - since its going through the
drm tree) with a cleanup patch moving nsecs_to_jiffies_timeout over to
time.c.

thanks
-john

Re: [Intel-gfx] [PATCH] drm/i915: compute wait_ioctl timeout correctly

[Jani Nikula]

On Thu, 04 Dec 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We've lost the +1 required for correct timeouts in
>
> commit 5ed0bdf21a85d78e04f89f15ccf227562177cbd9
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Wed Jul 16 21:05:06 2014 +0000
>
>     drm: i915: Use nsec based interfaces
>
>     Use ktime_get_raw_ns() and get rid of the back and forth timespec
>     conversions.
>
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>     Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> So fix this up by reinstating our handrolled _timeout function. While
> at it bother with handling MAX_JIFFIES.
>
> v2: Convert to usecs (we don't care about the accuracy anyway) first
> to avoid overflow issues Dave Gordon spotted.
>
> v3: Drop the explicit MAX_JIFFY_OFFSET check, usecs_to_jiffies should
> take care of that already. It might be a bit too enthusiastic about it
> though.
>
> v4: Chris has a much nicer color, so use his implementation.
>
> This requires to export nsec_to_jiffies from time.c.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82749
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Pushed to drm-intel-next-fixes, thanks for the patch.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 5 +++++
>  drivers/gpu/drm/i915/i915_gem.c | 3 ++-
>  kernel/time/time.c              | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 049482f5d9ac..564a45f4a0ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3097,6 +3097,11 @@ static inline unsigned long msecs_to_jiffies_timeout(const unsigned int m)
>  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
>  }
>  
> +static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> +{
> +        return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
> +}
> +
>  static inline unsigned long
>  timespec_to_jiffies_timeout(const struct timespec *value)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9d362d320d82..04a9f26407ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1226,7 +1226,8 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>  	if (i915_gem_request_completed(req, true))
>  		return 0;
>  
> -	timeout_expire = timeout ? jiffies + nsecs_to_jiffies((u64)*timeout) : 0;
> +	timeout_expire = timeout ?
> +		jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
>  
>  	if (INTEL_INFO(dev)->gen >= 6 && ring->id == RCS && can_wait_boost(file_priv)) {
>  		gen6_rps_boost(dev_priv);
> diff --git a/kernel/time/time.c b/kernel/time/time.c
> index a9ae20fb0b11..8fae82ca5cbf 100644
> --- a/kernel/time/time.c
> +++ b/kernel/time/time.c
> @@ -745,6 +745,7 @@ u64 nsecs_to_jiffies64(u64 n)
>  	return div_u64(n * 9, (9ull * NSEC_PER_SEC + HZ / 2) / HZ);
>  #endif
>  }
> +EXPORT_SYMBOL(nsecs_to_jiffies64);
>  
>  /**
>   * nsecs_to_jiffies - Convert nsecs in u64 to jiffies
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center