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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 17, 2014 at 03:09:58PM +0200, Jani Nikula wrote:
> On Thu, 19 Dec 2013, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>
> 
> 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.
-Daniel

> 
> >  
> >  /* Read the current pp_control value, unlocking the register if it
> >   * is locked
> > @@ -1147,7 +1164,7 @@ static void ironlake_panel_vdd_off_sync(struct intel_dp *intel_dp)
> >  		I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
> >  
> >  		if ((pp & POWER_TARGET_ON) == 0)
> > -			msleep(intel_dp->panel_power_cycle_delay);
> > +			intel_dp->last_power_cycle = jiffies;
> >  
> >  		intel_runtime_pm_put(dev_priv);
> >  	}
> > @@ -1222,6 +1239,7 @@ void ironlake_edp_panel_on(struct intel_dp *intel_dp)
> >  	POSTING_READ(pp_ctrl_reg);
> >  
> >  	ironlake_wait_panel_on(intel_dp);
> > +	intel_dp->last_power_on = jiffies;
> >  
> >  	if (IS_GEN5(dev)) {
> >  		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
> > @@ -1242,6 +1260,8 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
> >  
> >  	DRM_DEBUG_KMS("Turn eDP power off\n");
> >  
> > +	ironlake_edp_wait_backlight_off(intel_dp);
> > +
> >  	pp = ironlake_get_pp_control(intel_dp);
> >  	/* We need to switch off panel power _and_ force vdd, for otherwise some
> >  	 * panels get very unhappy and cease to work. */
> > @@ -1252,6 +1272,7 @@ void ironlake_edp_panel_off(struct intel_dp *intel_dp)
> >  	I915_WRITE(pp_ctrl_reg, pp);
> >  	POSTING_READ(pp_ctrl_reg);
> >  
> > +	intel_dp->last_power_cycle = jiffies;
> >  	ironlake_wait_panel_off(intel_dp);
> >  }
> >  
> > @@ -1273,7 +1294,7 @@ void ironlake_edp_backlight_on(struct intel_dp *intel_dp)
> >  	 * link.  So delay a bit to make sure the image is solid before
> >  	 * allowing it to appear.
> >  	 */
> > -	msleep(intel_dp->backlight_on_delay);
> > +	ironlake_wait_backlight_on(intel_dp);
> >  	pp = ironlake_get_pp_control(intel_dp);
> >  	pp |= EDP_BLC_ENABLE;
> >  
> > @@ -1305,7 +1326,7 @@ void ironlake_edp_backlight_off(struct intel_dp *intel_dp)
> >  
> >  	I915_WRITE(pp_ctrl_reg, pp);
> >  	POSTING_READ(pp_ctrl_reg);
> > -	msleep(intel_dp->backlight_off_delay);
> > +	intel_dp->last_backlight_off = jiffies;
> >  }
> >  
> >  static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46aea6c..92de688 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -485,6 +485,9 @@ struct intel_dp {
> >  	int backlight_off_delay;
> >  	struct delayed_work panel_vdd_work;
> >  	bool want_panel_vdd;
> > +	unsigned long last_power_cycle;
> > +	unsigned long last_power_on;
> > +	unsigned long last_backlight_off;
> >  	bool psr_setup_done;
> >  	struct intel_connector *attached_connector;
> >  };
> > -- 
> > 1.8.3.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux