| Message ID | 1390985553-3507-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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); } }
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(-)