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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 10, 2013 at 11:28:59PM +0100, Daniel Vetter wrote:
> On Fri, Dec 06, 2013 at 05:32:43PM -0200, Paulo Zanoni 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.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 26 +++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/intel_drv.h     |  4 ++++
> >  3 files changed, 45 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3c59b67..0c238dd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -806,6 +806,24 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
> >  		DRM_DEBUG_KMS("vblank wait timed out\n");
> >  }
> >  
> > +/* If you need to wait X ms 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 intel_wait_until_after and pass
> > + * the timestamp as the first argument, and X as the second argument. */
> > +void intel_wait_until_after(unsigned long timestamp, int to_wait_ms)
> > +{
> > +	unsigned long target = timestamp + msecs_to_jiffies(to_wait_ms);
> 
> msec_to_jiffies_timeout is what we want here. Also I nowadays prefer
> multiline comments with the /* and */ on their own line for more
> consistency ...
> -Daniel
> 
> > +	unsigned long diff;
> > +	/* Don't re-read the value of "jiffies" every time since it may change
> > +	 * behind our back and break the math. */
> > +	unsigned long tmp_jiffies = jiffies;
> > +
> > +	if (time_after(target, tmp_jiffies)) {
> > +		diff = (long)target - (long)tmp_jiffies;
> > +		msleep(jiffies_to_msecs(diff));
> 
> This will add one more jiffy again, so for optimal results we need to use
> something else here I think.

schedule_timeout is what I think we should use here. Also, this function
isn't really anything intel specific, so I think we should drop the intel_
prefix and shovel it as a static inline (it's really small after all) next
to the other generic timeout helpers at the bottom of i915_drv.h.

I'm also a bit unhappy still about the name - we should somehow make it
clearer that the timeout is in ms. A few ideas:

msleep_start_from_jiffies
msleep_form_jiffies
wait_after_until_ms
wait_remaining_ms_from_jiffies

Also mabye rename timestamp to ts_jiffies or so. All this bikeshedding
here is because the mixing of time-units here irks me a bit (and I pretty
much expect someone to botch it eventually). So spending a bit more time
on coming up with a really good name would be good imo.

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