Re: [Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
On Thu, 01 Jun 2017, Imre Deakwrote: > On Thu, Jun 01, 2017 at 04:58:50PM +0300, Ville Syrjälä wrote: >> On Thu, Jun 01, 2017 at 03:55:13PM +0300, Jani Nikula wrote: >> > On Wed, 31 May 2017, Ville Syrjälä wrote: >> > > On Wed, May 31, 2017 at 08:05:35PM +0300, Imre Deak wrote: >> > >> Atm disabling either DP or eDP outputs can generate a spurious short >> > >> pulse interrupt. The reason is that after disabling the port the source >> > >> will stop sending a valid stream data, while the sink expects either a >> > >> valid stream or the idle pattern. Since neither of this is sent the sink >> > >> assumes (after an arbitrary delay) that the link is lost and requests >> > >> for link retraining with a short pulse. >> > >> >> > >> The spurious pulse is a real problem at least for eDP panels with long >> > >> power-off / power-cycle delays: as part of disabling the output we >> > >> disable the panel power. The subsequent spurious short pulse handling >> > >> will have to turn the power back on, which means the driver has to do a >> > >> redundant wait for the power-off and power-cycle delays. During system >> > >> suspend this leads to an unnecessary delay up to ~1s on systems with >> > >> such panels as reported by Rui. >> > >> >> > >> To fix this put the sink to DPMS D3 state before turning off the port. >> > >> According to the DP spec in this state the sink should not request >> > >> retraining. This is also what we do already on pre-ddi platforms. >> > >> >> > >> As an alternative I also tried configuring the port to send idle pattern >> > >> - which is against BSPec - and leave the port in normal mode before >> > >> turning off the port. Neither of these resolved the problem. >> > >> >> > >> Cc: Zhang Rui >> > >> Cc: David Weinehall >> > >> Cc: Ville Syrjälä >> > >> Reported-and-tested-by: Zhang Rui >> > >> Signed-off-by: Imre Deak >> > > >> > > Makes sense to me. >> > >> > I wonder if we should write D0 on hotplug. >> >> D0 is the default power state IIRC, so when you plug something in it >> should automagically go into D0. That's actually a slight problem >> power-wise if there's no subsequent modeset to drop it into D3. > > There was this DP->VGA adaptor that seems to require that we set D0 > explicitly: > https://bugs.freedesktop.org/show_bug.cgi?id=99114#c5 > > But it's an odd behaviour, the DPCD read itself succeeds, only reading a > certain register fails (DPCD_SINK_COUNT). Based on the DP spec a sink > should respond to AUX transfers even after being put to D3, possibly > with a longer wake-up delay (up to 20ms AFAIR). Also [1] where AFAICT there isn't even a hotplug after D3. BR, Jani. [1] https://bugs.freedesktop.org/show_bug.cgi?id=101044 -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
On Thu, Jun 01, 2017 at 04:58:50PM +0300, Ville Syrjälä wrote: > On Thu, Jun 01, 2017 at 03:55:13PM +0300, Jani Nikula wrote: > > On Wed, 31 May 2017, Ville Syrjäläwrote: > > > On Wed, May 31, 2017 at 08:05:35PM +0300, Imre Deak wrote: > > >> Atm disabling either DP or eDP outputs can generate a spurious short > > >> pulse interrupt. The reason is that after disabling the port the source > > >> will stop sending a valid stream data, while the sink expects either a > > >> valid stream or the idle pattern. Since neither of this is sent the sink > > >> assumes (after an arbitrary delay) that the link is lost and requests > > >> for link retraining with a short pulse. > > >> > > >> The spurious pulse is a real problem at least for eDP panels with long > > >> power-off / power-cycle delays: as part of disabling the output we > > >> disable the panel power. The subsequent spurious short pulse handling > > >> will have to turn the power back on, which means the driver has to do a > > >> redundant wait for the power-off and power-cycle delays. During system > > >> suspend this leads to an unnecessary delay up to ~1s on systems with > > >> such panels as reported by Rui. > > >> > > >> To fix this put the sink to DPMS D3 state before turning off the port. > > >> According to the DP spec in this state the sink should not request > > >> retraining. This is also what we do already on pre-ddi platforms. > > >> > > >> As an alternative I also tried configuring the port to send idle pattern > > >> - which is against BSPec - and leave the port in normal mode before > > >> turning off the port. Neither of these resolved the problem. > > >> > > >> Cc: Zhang Rui > > >> Cc: David Weinehall > > >> Cc: Ville Syrjälä > > >> Reported-and-tested-by: Zhang Rui > > >> Signed-off-by: Imre Deak > > > > > > Makes sense to me. > > > > I wonder if we should write D0 on hotplug. > > D0 is the default power state IIRC, so when you plug something in it > should automagically go into D0. That's actually a slight problem > power-wise if there's no subsequent modeset to drop it into D3. There was this DP->VGA adaptor that seems to require that we set D0 explicitly: https://bugs.freedesktop.org/show_bug.cgi?id=99114#c5 But it's an odd behaviour, the DPCD read itself succeeds, only reading a certain register fails (DPCD_SINK_COUNT). Based on the DP spec a sink should respond to AUX transfers even after being put to D3, possibly with a longer wake-up delay (up to 20ms AFAIR). --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
On Thu, 01 Jun 2017, Ville Syrjäläwrote: > On Thu, Jun 01, 2017 at 03:55:13PM +0300, Jani Nikula wrote: >> On Wed, 31 May 2017, Ville Syrjälä wrote: >> > On Wed, May 31, 2017 at 08:05:35PM +0300, Imre Deak wrote: >> >> Atm disabling either DP or eDP outputs can generate a spurious short >> >> pulse interrupt. The reason is that after disabling the port the source >> >> will stop sending a valid stream data, while the sink expects either a >> >> valid stream or the idle pattern. Since neither of this is sent the sink >> >> assumes (after an arbitrary delay) that the link is lost and requests >> >> for link retraining with a short pulse. >> >> >> >> The spurious pulse is a real problem at least for eDP panels with long >> >> power-off / power-cycle delays: as part of disabling the output we >> >> disable the panel power. The subsequent spurious short pulse handling >> >> will have to turn the power back on, which means the driver has to do a >> >> redundant wait for the power-off and power-cycle delays. During system >> >> suspend this leads to an unnecessary delay up to ~1s on systems with >> >> such panels as reported by Rui. >> >> >> >> To fix this put the sink to DPMS D3 state before turning off the port. >> >> According to the DP spec in this state the sink should not request >> >> retraining. This is also what we do already on pre-ddi platforms. >> >> >> >> As an alternative I also tried configuring the port to send idle pattern >> >> - which is against BSPec - and leave the port in normal mode before >> >> turning off the port. Neither of these resolved the problem. >> >> >> >> Cc: Zhang Rui >> >> Cc: David Weinehall >> >> Cc: Ville Syrjälä >> >> Reported-and-tested-by: Zhang Rui >> >> Signed-off-by: Imre Deak >> > >> > Makes sense to me. >> >> I wonder if we should write D0 on hotplug. > > D0 is the default power state IIRC, so when you plug something in it > should automagically go into D0. That's actually a slight problem > power-wise if there's no subsequent modeset to drop it into D3. Right. The thought was about branch devices that don't wake up and aren't disconnected/reconnected after D3. But then we don't receive HPD for them aither. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
On Thu, Jun 01, 2017 at 03:55:13PM +0300, Jani Nikula wrote: > On Wed, 31 May 2017, Ville Syrjäläwrote: > > On Wed, May 31, 2017 at 08:05:35PM +0300, Imre Deak wrote: > >> Atm disabling either DP or eDP outputs can generate a spurious short > >> pulse interrupt. The reason is that after disabling the port the source > >> will stop sending a valid stream data, while the sink expects either a > >> valid stream or the idle pattern. Since neither of this is sent the sink > >> assumes (after an arbitrary delay) that the link is lost and requests > >> for link retraining with a short pulse. > >> > >> The spurious pulse is a real problem at least for eDP panels with long > >> power-off / power-cycle delays: as part of disabling the output we > >> disable the panel power. The subsequent spurious short pulse handling > >> will have to turn the power back on, which means the driver has to do a > >> redundant wait for the power-off and power-cycle delays. During system > >> suspend this leads to an unnecessary delay up to ~1s on systems with > >> such panels as reported by Rui. > >> > >> To fix this put the sink to DPMS D3 state before turning off the port. > >> According to the DP spec in this state the sink should not request > >> retraining. This is also what we do already on pre-ddi platforms. > >> > >> As an alternative I also tried configuring the port to send idle pattern > >> - which is against BSPec - and leave the port in normal mode before > >> turning off the port. Neither of these resolved the problem. > >> > >> Cc: Zhang Rui > >> Cc: David Weinehall > >> Cc: Ville Syrjälä > >> Reported-and-tested-by: Zhang Rui > >> Signed-off-by: Imre Deak > > > > Makes sense to me. > > I wonder if we should write D0 on hotplug. D0 is the default power state IIRC, so when you plug something in it should automagically go into D0. That's actually a slight problem power-wise if there's no subsequent modeset to drop it into D3. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
On Wed, 31 May 2017, Ville Syrjäläwrote: > On Wed, May 31, 2017 at 08:05:35PM +0300, Imre Deak wrote: >> Atm disabling either DP or eDP outputs can generate a spurious short >> pulse interrupt. The reason is that after disabling the port the source >> will stop sending a valid stream data, while the sink expects either a >> valid stream or the idle pattern. Since neither of this is sent the sink >> assumes (after an arbitrary delay) that the link is lost and requests >> for link retraining with a short pulse. >> >> The spurious pulse is a real problem at least for eDP panels with long >> power-off / power-cycle delays: as part of disabling the output we >> disable the panel power. The subsequent spurious short pulse handling >> will have to turn the power back on, which means the driver has to do a >> redundant wait for the power-off and power-cycle delays. During system >> suspend this leads to an unnecessary delay up to ~1s on systems with >> such panels as reported by Rui. >> >> To fix this put the sink to DPMS D3 state before turning off the port. >> According to the DP spec in this state the sink should not request >> retraining. This is also what we do already on pre-ddi platforms. >> >> As an alternative I also tried configuring the port to send idle pattern >> - which is against BSPec - and leave the port in normal mode before >> turning off the port. Neither of these resolved the problem. >> >> Cc: Zhang Rui >> Cc: David Weinehall >> Cc: Ville Syrjälä >> Reported-and-tested-by: Zhang Rui >> Signed-off-by: Imre Deak > > Makes sense to me. I wonder if we should write D0 on hotplug. BR, Jani. > > Reviewed-by: Ville Syrjälä > >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 10 +++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 0914ad9..8bac628 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1732,12 +1732,18 @@ static void intel_ddi_post_disable(struct >> intel_encoder *intel_encoder, >> struct drm_i915_private *dev_priv = to_i915(encoder->dev); >> enum port port = intel_ddi_get_encoder_port(intel_encoder); >> struct intel_digital_port *dig_port = enc_to_dig_port(encoder); >> +struct intel_dp *intel_dp = NULL; >> int type = intel_encoder->type; >> uint32_t val; >> bool wait = false; >> >> /* old_crtc_state and old_conn_state are NULL when called from DP_MST */ >> >> +if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { >> +intel_dp = enc_to_intel_dp(encoder); >> +intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); >> +} >> + >> val = I915_READ(DDI_BUF_CTL(port)); >> if (val & DDI_BUF_CTL_ENABLE) { >> val &= ~DDI_BUF_CTL_ENABLE; >> @@ -1753,9 +1759,7 @@ static void intel_ddi_post_disable(struct >> intel_encoder *intel_encoder, >> if (wait) >> intel_wait_ddi_buf_idle(dev_priv, port); >> >> -if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { >> -struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> -intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); >> +if (intel_dp) { >> intel_edp_panel_vdd_on(intel_dp); >> intel_edp_panel_off(intel_dp); >> } >> -- >> 2.7.4 -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
On Wed, May 31, 2017 at 08:05:35PM +0300, Imre Deak wrote: > Atm disabling either DP or eDP outputs can generate a spurious short > pulse interrupt. The reason is that after disabling the port the source > will stop sending a valid stream data, while the sink expects either a > valid stream or the idle pattern. Since neither of this is sent the sink > assumes (after an arbitrary delay) that the link is lost and requests > for link retraining with a short pulse. > > The spurious pulse is a real problem at least for eDP panels with long > power-off / power-cycle delays: as part of disabling the output we > disable the panel power. The subsequent spurious short pulse handling > will have to turn the power back on, which means the driver has to do a > redundant wait for the power-off and power-cycle delays. During system > suspend this leads to an unnecessary delay up to ~1s on systems with > such panels as reported by Rui. > > To fix this put the sink to DPMS D3 state before turning off the port. > According to the DP spec in this state the sink should not request > retraining. This is also what we do already on pre-ddi platforms. > > As an alternative I also tried configuring the port to send idle pattern > - which is against BSPec - and leave the port in normal mode before > turning off the port. Neither of these resolved the problem. > > Cc: Zhang Rui> Cc: David Weinehall > Cc: Ville Syrjälä > Reported-and-tested-by: Zhang Rui > Signed-off-by: Imre Deak Makes sense to me. Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_ddi.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 0914ad9..8bac628 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1732,12 +1732,18 @@ static void intel_ddi_post_disable(struct > intel_encoder *intel_encoder, > struct drm_i915_private *dev_priv = to_i915(encoder->dev); > enum port port = intel_ddi_get_encoder_port(intel_encoder); > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > + struct intel_dp *intel_dp = NULL; > int type = intel_encoder->type; > uint32_t val; > bool wait = false; > > /* old_crtc_state and old_conn_state are NULL when called from DP_MST */ > > + if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { > + intel_dp = enc_to_intel_dp(encoder); > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + } > + > val = I915_READ(DDI_BUF_CTL(port)); > if (val & DDI_BUF_CTL_ENABLE) { > val &= ~DDI_BUF_CTL_ENABLE; > @@ -1753,9 +1759,7 @@ static void intel_ddi_post_disable(struct intel_encoder > *intel_encoder, > if (wait) > intel_wait_ddi_buf_idle(dev_priv, port); > > - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + if (intel_dp) { > intel_edp_panel_vdd_on(intel_dp); > intel_edp_panel_off(intel_dp); > } > -- > 2.7.4 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/ddi: Avoid long delays during system suspend / eDP disabling
Atm disabling either DP or eDP outputs can generate a spurious short pulse interrupt. The reason is that after disabling the port the source will stop sending a valid stream data, while the sink expects either a valid stream or the idle pattern. Since neither of this is sent the sink assumes (after an arbitrary delay) that the link is lost and requests for link retraining with a short pulse. The spurious pulse is a real problem at least for eDP panels with long power-off / power-cycle delays: as part of disabling the output we disable the panel power. The subsequent spurious short pulse handling will have to turn the power back on, which means the driver has to do a redundant wait for the power-off and power-cycle delays. During system suspend this leads to an unnecessary delay up to ~1s on systems with such panels as reported by Rui. To fix this put the sink to DPMS D3 state before turning off the port. According to the DP spec in this state the sink should not request retraining. This is also what we do already on pre-ddi platforms. As an alternative I also tried configuring the port to send idle pattern - which is against BSPec - and leave the port in normal mode before turning off the port. Neither of these resolved the problem. Cc: Zhang RuiCc: David Weinehall Cc: Ville Syrjälä Reported-and-tested-by: Zhang Rui Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/intel_ddi.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 0914ad9..8bac628 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1732,12 +1732,18 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, struct drm_i915_private *dev_priv = to_i915(encoder->dev); enum port port = intel_ddi_get_encoder_port(intel_encoder); struct intel_digital_port *dig_port = enc_to_dig_port(encoder); + struct intel_dp *intel_dp = NULL; int type = intel_encoder->type; uint32_t val; bool wait = false; /* old_crtc_state and old_conn_state are NULL when called from DP_MST */ + if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { + intel_dp = enc_to_intel_dp(encoder); + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); + } + val = I915_READ(DDI_BUF_CTL(port)); if (val & DDI_BUF_CTL_ENABLE) { val &= ~DDI_BUF_CTL_ENABLE; @@ -1753,9 +1759,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder, if (wait) intel_wait_ddi_buf_idle(dev_priv, port); - if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) { - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); + if (intel_dp) { intel_edp_panel_vdd_on(intel_dp); intel_edp_panel_off(intel_dp); } -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx