As it turns out, the aux block being off was not the real problem here,
as transition from D3 to D0 is mandated by the DP spec to take a maximum
of 1ms, whereas we're allowed a 100ms timeframe to respond to ESI irqs.
The real problem here is a bit more subtle.
When doing a modeset where the problem of the sink timing out to our
sideband requests when transitioning from D3 to D0 occurs, the timeout
is from the aux block not coming on. However, nothing else times out
other than the initial phy_up message because the DPCD on call in
intel_ddi_enable_dp() ends up waking up the AUX block on the hub, not
the phy_up sideband message. This means that the real fix we need is to
use the DPMS on before sending a phy_up to ensure that the hub is ready
to accept sideband messages.
Signed-off-by: Lyude Paul <[email protected]>
Cc: Dhinakaran Pandiyan <[email protected]>
Cc: Ville Syrjälä <[email protected]>
Cc: Laura Abbott <[email protected]>
Cc: [email protected]
Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
---
drivers/gpu/drm/i915/intel_ddi.c | 6 +++++-
drivers/gpu/drm/i915/intel_dp_mst.c | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index a6672a9abd85..9bd675f73f7b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2324,7 +2324,11 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
intel_prepare_dp_ddi_buffers(encoder, crtc_state);
intel_ddi_init_dp_buf_reg(encoder);
- intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
+ /* for MST, we do DPMS_ON outside of here so that DPMS_ON can happen
+ * before drm_dp_send_power_updown_phy()
+ */
+ if (!intel_dp->is_mst)
+ intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
intel_dp_start_link_train(intel_dp);
if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
intel_dp_stop_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index c3de0918ee13..eff9a4eae1f0 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -223,6 +223,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
+ intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
if (intel_dp->active_mst_links == 0)
intel_dig_port->base.pre_enable(&intel_dig_port->base,
--
2.14.3
On Wed, Apr 04, 2018 at 07:27:21PM -0400, Lyude Paul wrote:
> As it turns out, the aux block being off was not the real problem here,
> as transition from D3 to D0 is mandated by the DP spec to take a maximum
> of 1ms, whereas we're allowed a 100ms timeframe to respond to ESI irqs.
> The real problem here is a bit more subtle.
>
> When doing a modeset where the problem of the sink timing out to our
> sideband requests when transitioning from D3 to D0 occurs, the timeout
> is from the aux block not coming on. However, nothing else times out
> other than the initial phy_up message because the DPCD on call in
> intel_ddi_enable_dp() ends up waking up the AUX block on the hub, not
> the phy_up sideband message. This means that the real fix we need is to
> use the DPMS on before sending a phy_up to ensure that the hub is ready
> to accept sideband messages.
>
> Signed-off-by: Lyude Paul <[email protected]>
> Cc: Dhinakaran Pandiyan <[email protected]>
> Cc: Ville Syrj?l? <[email protected]>
> Cc: Laura Abbott <[email protected]>
> Cc: [email protected]
> Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 6 +++++-
> drivers/gpu/drm/i915/intel_dp_mst.c | 1 +
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index a6672a9abd85..9bd675f73f7b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2324,7 +2324,11 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> intel_prepare_dp_ddi_buffers(encoder, crtc_state);
>
> intel_ddi_init_dp_buf_reg(encoder);
> - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> + /* for MST, we do DPMS_ON outside of here so that DPMS_ON can happen
> + * before drm_dp_send_power_updown_phy()
> + */
> + if (!intel_dp->is_mst)
Just 'is_mst' should do here.
And in general I'd like to see the enable and disable paths remain
symmetric. Ie. also move out the dpms call in the disable path (or
maybe move the phy_power_up/down in?).
> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> intel_dp_start_link_train(intel_dp);
> if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> intel_dp_stop_link_train(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index c3de0918ee13..eff9a4eae1f0 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -223,6 +223,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>
> DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>
> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
This could use a comment to remind people that the order does matter.
> if (intel_dp->active_mst_links == 0)
> intel_dig_port->base.pre_enable(&intel_dig_port->base,
> --
> 2.14.3
--
Ville Syrj?l?
Intel OTC
On Thu, 2018-04-05 at 19:38 +0300, Ville Syrjälä wrote:
> On Wed, Apr 04, 2018 at 07:27:21PM -0400, Lyude Paul wrote:
> > As it turns out, the aux block being off was not the real problem here,
> > as transition from D3 to D0 is mandated by the DP spec to take a maximum
> > of 1ms, whereas we're allowed a 100ms timeframe to respond to ESI irqs.
> > The real problem here is a bit more subtle.
> >
> > When doing a modeset where the problem of the sink timing out to our
> > sideband requests when transitioning from D3 to D0 occurs, the timeout
> > is from the aux block not coming on. However, nothing else times out
> > other than the initial phy_up message because the DPCD on call in
> > intel_ddi_enable_dp() ends up waking up the AUX block on the hub, not
> > the phy_up sideband message.
This is the case only when intel_dp_sink_dpms(DRM_MODE_DPMS_OFF) was the
last action. With power_down_phy in post_disable() and power_up_phy in
pre_enable(), we weren't seeing this issue.
> This means that the real fix we need is to
> > use the DPMS on before sending a phy_up to ensure that the hub is ready
> > to accept sideband messages.
> >
> > Signed-off-by: Lyude Paul <[email protected]>
> > Cc: Dhinakaran Pandiyan <[email protected]>
> > Cc: Ville Syrjälä <[email protected]>
> > Cc: Laura Abbott <[email protected]>
> > Cc: [email protected]
> > Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 6 +++++-
> > drivers/gpu/drm/i915/intel_dp_mst.c | 1 +
> > 2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index a6672a9abd85..9bd675f73f7b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2324,7 +2324,11 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > intel_prepare_dp_ddi_buffers(encoder, crtc_state);
> >
> > intel_ddi_init_dp_buf_reg(encoder);
> > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > + /* for MST, we do DPMS_ON outside of here so that DPMS_ON can happen
> > + * before drm_dp_send_power_updown_phy()
> > + */
> > + if (!intel_dp->is_mst)
>
> Just 'is_mst' should do here.
>
> And in general I'd like to see the enable and disable paths remain
> symmetric. Ie. also move out the dpms call in the disable path (or
> maybe move the phy_power_up/down in?).
>
> > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > intel_dp_start_link_train(intel_dp);
> > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > intel_dp_stop_link_train(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index c3de0918ee13..eff9a4eae1f0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -223,6 +223,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> >
> > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >
> > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
>
> This could use a comment to remind people that the order does matter.
>
> > if (intel_dp->active_mst_links == 0)
> > intel_dig_port->base.pre_enable(&intel_dig_port->base,
> > --
> > 2.14.3
>