Re: [PATCH] drm/i915: eDP HPD connected check to reduce T3 time

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 22, 2015 at 02:34:02PM -0700, Clint Taylor wrote:
> On 09/22/2015 10:27 AM, Ville Syrjälä wrote:
> > On Tue, Sep 22, 2015 at 10:09:39AM -0700, clinton.a.taylor@intel.com wrote:
> >> From: Clint Taylor <clinton.a.taylor@intel.com>
> >>
> >> To reduce eDP T3 time check for digital port connected instead of
> >> msleep. Maintain VBT time if HPD is not asserted on the port.
> >>
> >> Current eDP T3 time is an msleep for the panel_power_up time specified
> >> in VBT. The eDP specification allows maximum T3 time of 200ms.
> >> Typically panels raise HPD from 70ms-105ms and are ready for AUX traffic
> >>   and training. Reading HPD will reduce power-on and resume times by over
> >> 100ms on systems with eDP HPD connected.
> >>
> >> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_dp.c |   13 ++++++++++++-
> >>   1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 77e4115..7caf3ab 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -129,6 +129,8 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
> >>   static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
> >>   static void vlv_steal_power_sequencer(struct drm_device *dev,
> >>   				      enum pipe pipe);
> >> +static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >> +					 struct intel_digital_port *port);
> >>
> >>   static unsigned int intel_dp_unused_lane_mask(int lane_count)
> >>   {
> >> @@ -1772,6 +1774,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
> >>   	u32 pp;
> >>   	u32 pp_stat_reg, pp_ctrl_reg;
> >>   	bool need_to_disable = !intel_dp->want_panel_vdd;
> >> +	int i, step = 0;
> >>
> >>   	lockdep_assert_held(&dev_priv->pps_mutex);
> >>
> >> @@ -1809,7 +1812,15 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
> >>   	if (!edp_have_panel_power(intel_dp)) {
> >>   		DRM_DEBUG_KMS("eDP port %c panel power wasn't enabled\n",
> >>   			      port_name(intel_dig_port->port));
> >> -		msleep(intel_dp->panel_power_up_delay);
> >> +		step = intel_dp->panel_power_up_delay / 10;
> >> +		for (i=0; i < intel_dp->panel_power_up_delay; i+=step) {
> >> +			if (intel_digital_port_connected(dev_priv, intel_dig_port)) {
> >> +				DRM_DEBUG_KMS("Port %c HPD detected\n",
> >> +					      port_name(intel_dig_port->port));
> >> +				break;
> >> +			}
> >> +			msleep(10);
> >> +		}
> >
> > We have the HPD irq wired up for all ports now, so we could even do
> > this without polling.
> 
> I thought about using the HPD irq though I really didn't want to get 
> into full HPD handling of the eDP port since only the rise of HPD is of 
> interest.

Well, since we have it all wired up already, should be mostly a matter
of adding a waitqueue, wake it up from hpd_pulse, and use
wait_event_timeout() to wait for it.

> > There is a slight concern that what if the HPD line is noisy (happened
> > on BSW RVP due wrong pullup settings at least) we could start link
> > training before the panel is powered up. But I have no good solution for
> > that, other than maybe blacklisting bad machines, which would require
> > that we detect them somehow in the first place. Maybe we should have
> > some kind of check in case the HPD is raised suspiciously soon, we would
> > fall back to the full msleep method?
> >
> 
> If for some reason the HPD line asserts early we could always retry AUX 
> transactions until we get a good ACK before starting link training. VBT 
> has a field for "T3 optimization" that when enabled just polls the AUX 
> until the panel becomes ready instead of waiting for T3 time. retrying 
> AUX on a spurious HPD would be the equivalent. We would need a sane way 
> to give up after the expected T3 time.

Hmm. Yeah perhaps just making really sure AUX works before we continue
would be a good way.

> > Another thing I didn't think about fully is how the power sequencer fits
> > into this sort of thing. It seems we always enable the vdd force override
> > before starting the panel on sequence, so i suppose we should be able
> > to program the panel power on delay to 0 (or whatever the min is), and
> > just rely on the driver to do the delays?
> >
> The power panel sequencer is actually causing additional delays during 
> coldboot and resume. The current optimization in the driver for 
> power_panel_cycle T12 delay is not being realized as we still wait for 
> the panel power sequencer to complete its cycle before training the 
> panel. That is my next level of optimization that I was going to work on 
> next. My initial prototypes are causing spikes in the output voltage 
> which could damage the panel. If the power on delays were 0 that may 
> help. Thanks for the idea.

OK.

> 
> >
> >>   	}
> >>
> >>   	return need_to_disable;
> >> --
> >> 1.7.9.5
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx