Re: [PATCH 2/6] drm/i915: save some time when waiting the eDP timings

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 20, 2013 at 11:30:15AM +0200, Jani Nikula wrote:
> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > The eDP spec defines some points where after you do action A, you have
> > to wait some time before action B. The thing is that in our driver
> > action B does not happen exactly after action A, but we still use
> > msleep() calls directly. What this patch does is that we record the
> > timestamp of when action A happened, then, just before action B, we
> > look at how much time has passed and only sleep the remaining amount
> > needed.
> >
> > With this change, I am able to save about 5-20ms (out of the total
> > 200ms) of the backlight_off delay and completely skip the 1ms
> > backlight_on delay. The 600ms vdd_off delay doesn't happen during
> > normal usage anymore due to a previous patch.
> >
> > v2: - Rename ironlake_wait_jiffies_delay to intel_wait_until_after and
> >       move it to intel_display.c
> >     - Fix the msleep call: diff is in jiffies
> > v3: - Use "tmp_jiffies" so we don't need to worry about the value of
> >       "jiffies" advancing while we're doing the math.
> > v4: - Rename function again.
> >     - Move function to i915_drv.h.
> >     - Store last_power_cycle at edp_panel_off too.
> >     - Use msecs_to_jiffies_timeout, then replace the msleep with an
> >       open-coded version that avoids the extra +1 jiffy.
> >     - Try to add units to every variable name so we don't confuse
> >       jiffies with milliseconds.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 29 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 27 ++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_drv.h |  3 +++
> >  3 files changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index cc8afff..7e9b436 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2636,4 +2636,33 @@ timespec_to_jiffies_timeout(const struct timespec *value)
> >  	return min_t(unsigned long, MAX_JIFFY_OFFSET, j + 1);
> >  }
> >  
> > +/*
> > + * If you need to wait X milliseconds between events A and B, but event B
> > + * 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.
> > + */
> > +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;
> > +
> > +	/*
> > +	 * Don't re-read the value of "jiffies" every time since it may change
> > +	 * behind our back and break the math.
> > +	 */
> > +	tmp_jiffies = jiffies;
> > +	target_jiffies = timestamp_jiffies +
> > +			 msecs_to_jiffies_timeout(to_wait_ms);
> 
> Is it possible this gets called before the last_* timestamp fields are
> initialized, resulting to timestamp_jiffies being 0? Do we need to
> special case that? If not, perhaps add a WARN_ON(timestamp_jiffies == 0)
> so we maintain this precondition.

afaik jiffies are allowed to wrap around to 0 ...
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch