| Message ID | 1442941779-13633-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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? 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? > } > > 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
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. > > 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. > 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. > >> } >> >> 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 >
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 > >
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); + } } return need_to_disable;