[PATCH] drm/i915: fix wait_remaining_ms_from_jiffies

[PATCH] drm/i915: fix wait_remaining_ms_from_jiffies

[Imre Deak]

schedule_timeout_uninterruptible() takes jiffies not ms.

Also we should check whether jiffies has overflowed since the timestamp
for event A was taken. This is highly unlikely on 64 bit, but on 32 bit
machines jiffies initially is -300*HZ. If the panel power is initially
off the first wait from edp_panel_vdd_on()->wait_panel_power_cycle()
will result in a call timestamp_jiffies of 0, so on 32 bit machines we
would wait ~300 sec + to_wait_ms. Fix this by checking if the initial
timestamp is not in the future.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3673ba1..6a80393 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2643,12 +2643,13 @@ timespec_to_jiffies_timeout(const struct timespec *value)
  * doesn't happen exactly after event A, you record the timestamp (jiffies) of
  * when event A happened, then just before event B you call this function and
  * pass the timestamp as the first argument, and X as the second argument.
+ * Note that the recorded timestamp (timestamp_jiffies) can't be in the future
+ * otherwise the function won't wait at all.
  */
 static inline void
 wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 {
-	unsigned long target_jiffies, tmp_jiffies;
-	unsigned int remaining_ms;
+	unsigned long target_jiffies, tmp_jiffies, remaining_jiffies;
 
 	/*
 	 * Don't re-read the value of "jiffies" every time since it may change
@@ -2658,12 +2659,12 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
 	target_jiffies = timestamp_jiffies +
 			 msecs_to_jiffies_timeout(to_wait_ms);
 
-	if (time_after(target_jiffies, tmp_jiffies)) {
-		remaining_ms = jiffies_to_msecs((long)target_jiffies -
-						(long)tmp_jiffies);
-		while (remaining_ms)
-			remaining_ms =
-				schedule_timeout_uninterruptible(remaining_ms);
+	if (time_after(target_jiffies, tmp_jiffies) &&
+	    time_before_eq(timestamp_jiffies, tmp_jiffies)) {
+		remaining_jiffies = target_jiffies - tmp_jiffies;
+		while (remaining_jiffies)
+			remaining_jiffies =
+			    schedule_timeout_uninterruptible(remaining_jiffies);
 	}
 }
 
-- 
1.8.4

Re: [PATCH] drm/i915: fix wait_remaining_ms_from_jiffies

[Chris Wilson]

On Wed, Jan 29, 2014 at 10:52:33AM +0200, Imre Deak wrote:
> Also we should check whether jiffies has overflowed since the timestamp
> for event A was taken. This is highly unlikely on 64 bit, but on 32 bit
> machines jiffies initially is -300*HZ. If the panel power is initially
> off the first wait from edp_panel_vdd_on()->wait_panel_power_cycle()
> will result in a call timestamp_jiffies of 0, so on 32 bit machines we
> would wait ~300 sec + to_wait_ms. Fix this by checking if the initial
> timestamp is not in the future.

Fix the caller. A target + must wait delay set in the future seems like
legal input to me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: fix wait_remaining_ms_from_jiffies

[Jani Nikula]

On Wed, 29 Jan 2014, Imre Deak <imre.deak@intel.com> wrote:
> schedule_timeout_uninterruptible() takes jiffies not ms.
>
> Also we should check whether jiffies has overflowed since the timestamp
> for event A was taken. This is highly unlikely on 64 bit, but on 32 bit
> machines jiffies initially is -300*HZ. If the panel power is initially
> off the first wait from edp_panel_vdd_on()->wait_panel_power_cycle()
> will result in a call timestamp_jiffies of 0, so on 32 bit machines we
> would wait ~300 sec + to_wait_ms. Fix this by checking if the initial
> timestamp is not in the future.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3673ba1..6a80393 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2643,12 +2643,13 @@ timespec_to_jiffies_timeout(const struct timespec *value)
>   * doesn't happen exactly after event A, you record the timestamp (jiffies) of
>   * when event A happened, then just before event B you call this function and
>   * pass the timestamp as the first argument, and X as the second argument.
> + * Note that the recorded timestamp (timestamp_jiffies) can't be in the future
> + * otherwise the function won't wait at all.
>   */
>  static inline void
>  wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>  {
> -	unsigned long target_jiffies, tmp_jiffies;
> -	unsigned int remaining_ms;
> +	unsigned long target_jiffies, tmp_jiffies, remaining_jiffies;
>  
>  	/*
>  	 * Don't re-read the value of "jiffies" every time since it may change
> @@ -2658,12 +2659,12 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
>  	target_jiffies = timestamp_jiffies +
>  			 msecs_to_jiffies_timeout(to_wait_ms);
>  
> -	if (time_after(target_jiffies, tmp_jiffies)) {
> -		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> -						(long)tmp_jiffies);
> -		while (remaining_ms)
> -			remaining_ms =
> -				schedule_timeout_uninterruptible(remaining_ms);
> +	if (time_after(target_jiffies, tmp_jiffies) &&
> +	    time_before_eq(timestamp_jiffies, tmp_jiffies)) {
> +		remaining_jiffies = target_jiffies - tmp_jiffies;
> +		while (remaining_jiffies)
> +			remaining_jiffies =
> +			    schedule_timeout_uninterruptible(remaining_jiffies);
>  	}
>  }

For the record, I spotted the jiffies vs. ms mistake in review [1],
Paulo posted v5 [2], but apparently Daniel applied v4 anyway:

commit dce56b3c626fb1d533258a624d42a1a3fc17da17
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Thu Dec 19 14:29:40 2013 -0200

    drm/i915: save some time when waiting the eDP timings

Wrap around was also discussed.


BR,
Jani.

[1] http://mid.gmane.org/87fvpnkgyg.fsf@intel.com
[2] http://mid.gmane.org/1388778311-2020-1-git-send-email-przanoni@gmail.com



-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: fix wait_remaining_ms_from_jiffies

[Daniel Vetter]

On Wed, Jan 29, 2014 at 01:37:53PM +0200, Jani Nikula wrote:
> On Wed, 29 Jan 2014, Imre Deak <imre.deak@intel.com> wrote:
> > schedule_timeout_uninterruptible() takes jiffies not ms.
> >
> > Also we should check whether jiffies has overflowed since the timestamp
> > for event A was taken. This is highly unlikely on 64 bit, but on 32 bit
> > machines jiffies initially is -300*HZ. If the panel power is initially
> > off the first wait from edp_panel_vdd_on()->wait_panel_power_cycle()
> > will result in a call timestamp_jiffies of 0, so on 32 bit machines we
> > would wait ~300 sec + to_wait_ms. Fix this by checking if the initial
> > timestamp is not in the future.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3673ba1..6a80393 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2643,12 +2643,13 @@ timespec_to_jiffies_timeout(const struct timespec *value)
> >   * doesn't happen exactly after event A, you record the timestamp (jiffies) of
> >   * when event A happened, then just before event B you call this function and
> >   * pass the timestamp as the first argument, and X as the second argument.
> > + * Note that the recorded timestamp (timestamp_jiffies) can't be in the future
> > + * otherwise the function won't wait at all.
> >   */
> >  static inline void
> >  wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> >  {
> > -	unsigned long target_jiffies, tmp_jiffies;
> > -	unsigned int remaining_ms;
> > +	unsigned long target_jiffies, tmp_jiffies, remaining_jiffies;
> >  
> >  	/*
> >  	 * Don't re-read the value of "jiffies" every time since it may change
> > @@ -2658,12 +2659,12 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> >  	target_jiffies = timestamp_jiffies +
> >  			 msecs_to_jiffies_timeout(to_wait_ms);
> >  
> > -	if (time_after(target_jiffies, tmp_jiffies)) {
> > -		remaining_ms = jiffies_to_msecs((long)target_jiffies -
> > -						(long)tmp_jiffies);
> > -		while (remaining_ms)
> > -			remaining_ms =
> > -				schedule_timeout_uninterruptible(remaining_ms);
> > +	if (time_after(target_jiffies, tmp_jiffies) &&
> > +	    time_before_eq(timestamp_jiffies, tmp_jiffies)) {
> > +		remaining_jiffies = target_jiffies - tmp_jiffies;
> > +		while (remaining_jiffies)
> > +			remaining_jiffies =
> > +			    schedule_timeout_uninterruptible(remaining_jiffies);
> >  	}
> >  }
> 
> For the record, I spotted the jiffies vs. ms mistake in review [1],
> Paulo posted v5 [2], but apparently Daniel applied v4 anyway:
> 
> commit dce56b3c626fb1d533258a624d42a1a3fc17da17
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Thu Dec 19 14:29:40 2013 -0200
> 
>     drm/i915: save some time when waiting the eDP timings
> 
> Wrap around was also discussed.
> 
> 
> BR,
> Jani.
> 
> [1] http://mid.gmane.org/87fvpnkgyg.fsf@intel.com
> [2] http://mid.gmane.org/1388778311-2020-1-git-send-email-przanoni@gmail.com

Oh dear, I'll hide in shame. Dunno how I've botched this one up, thanks
for catching it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch