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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 17, 2014 at 02:29:51PM +0100, Daniel Vetter wrote:
> On Fri, Jan 17, 2014 at 03:09:58PM +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);
> > > +
> > > +	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);
> > > +	}
> > > +}
> > > +
> > >  #endif
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 9d96447..2f82af4 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1057,9 +1057,26 @@ static void ironlake_wait_panel_off(struct intel_dp *intel_dp)
> > >  static void ironlake_wait_panel_power_cycle(struct intel_dp *intel_dp)
> > >  {
> > >  	DRM_DEBUG_KMS("Wait for panel power cycle\n");
> > > +
> > > +	/* When we disable the VDD override bit last we have to do the manual
> > > +	 * wait. */
> > > +	wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
> > > +				       intel_dp->panel_power_cycle_delay);
> > > +
> > >  	ironlake_wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
> > >  }
> > >  
> > > +static void ironlake_wait_backlight_on(struct intel_dp *intel_dp)
> > > +{
> > > +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> > > +				       intel_dp->backlight_on_delay);
> > > +}
> > > +
> > > +static void ironlake_edp_wait_backlight_off(struct intel_dp *intel_dp)
> > > +{
> > > +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> > > +				       intel_dp->backlight_off_delay);
> > > +}
> > 
> > I think the naming here is a bit unfortunate. You call wait_backlight_on()
> > *before* you enable backlight, but wait_backlight_off() *after* you
> > disable backlight.
> > 
> > The wait_panel_{on,off} functions wait for some event to happen.
> > 
> > I think ironlake_wait_backlight_on() *sounds* like something to call
> > *after* you've enabled backlight. So instead, I think the function
> > should be called something like wait_power_on()... but then it gets
> > confusing with the wait_panel_on(). Ugh.
> > 
> > I don't know. I'll just say
> > 
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> > 
> > because I think the patch looks good otherwise, and maybe someone will
> > come up with the Perfect Naming Scheme and send follow-up patches we can
> > bikeshed until the end of time...
> 
> I think a bikeshed patch on top to drop the ironlake_ prefixes would be
> good, since this isn't anything hw specific at all. I'll do that when
> applying. Otherwise no Clever Ideas for the actual names atm.

I've merged the first two patches for now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch