| Message ID | 20191207011832.422566-3-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/tgl: Select master transcoder for MST stream | expand |
On Fri, Dec 06, 2019 at 05:18:31PM -0800, José Roberto de Souza wrote: > The disable sequence after wait for transcoder off was not correctly > implemented. > The MST disable sequence is basically the same for HSW, SKL, ICL and > TGL, with just minor changes for TGL. > > So here calling a new MST function to do the MST sequences, most of > the steps just moved from the post disable hook. > > With this last patch we finally fixed the hotplugs triggered by MST > sinks during the disable/enable sequence, those were causing source > to try to do a link training while it was not ready causing CPU pipe > FIFO underrrus on TGL. > > BSpec: 4231 > BSpec: 4163 > BSpec: 22243 > BSpec: 49190 > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 17 +++-- > drivers/gpu/drm/i915/display/intel_display.c | 2 + > drivers/gpu/drm/i915/display/intel_dp_mst.c | 79 ++++++++++++++++---- > drivers/gpu/drm/i915/display/intel_dp_mst.h | 1 + > 4 files changed, 79 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index be5bc506d4d3..f06c6dfec888 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -34,6 +34,7 @@ > #include "intel_ddi.h" > #include "intel_display_types.h" > #include "intel_dp.h" > +#include "intel_dp_mst.h" > #include "intel_dp_link_training.h" > #include "intel_dpio_phy.h" > #include "intel_dsi.h" > @@ -1953,17 +1954,19 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > - i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder); > - u32 val = I915_READ(reg); > + u32 val; > + > + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); > + val &= ~TRANS_DDI_FUNC_ENABLE; > > if (INTEL_GEN(dev_priv) >= 12) { > - val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK | > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) || > + intel_dp_mst_is_slave_trans(crtc_state)) > + val &= ~TGL_TRANS_DDI_PORT_MASK; > } else { > - val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK | > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); > + val &= ~TRANS_DDI_PORT_MASK; > } > - I915_WRITE(reg, val); > + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); > > if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME && > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 2f74c0bfb2a8..d3eefb271fa4 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -6776,6 +6776,8 @@ static void haswell_crtc_disable(struct intel_atomic_state *state, > if (!transcoder_is_dsi(cpu_transcoder)) > intel_disable_pipe(old_crtc_state); > > + intel_dp_mst_post_trans_disabled(old_crtc_state); > + Basically a new ad-hoc encoder hook :( I think we either want to suck in more crap from the crtc_enable/disable into the encoder hooks, or we try to move everything from the current .post_disable() into .post_pll_disable() and relocate .post_disabel() to a better place in the sequence. Though the whole .post_pll_disable() is a farily poor match for these platforms as there's nothing to do after disabling the PLL. So from the hook naming POV I'd kinda want move things the other way. So I think moving more things into the encoder hooks sounds like a better plan. Eg. in this case I think we could probably shovel everything after intel_pipe_disable() into .post_disable(). Would also help us eliminate some of those 'if !dsi' things. I'm also thinking intel_disable_ddi_buf() should be split up and just inlined into the proper place(s). Would help in making the sequence more easily comparable with the spec. > if (INTEL_GEN(dev_priv) >= 11) > icl_disable_transcoder_port_sync(old_crtc_state); > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 49f1518e3ab9..3c98a25e6308 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -365,6 +365,57 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, > old_crtc_state, old_conn_state); > } > > +static void > +dp_mst_post_trans_disabled(struct intel_encoder *encoder, > + const struct intel_crtc_state *old_crtc_state, > + const struct drm_connector_state *old_conn_state) > +{ > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base); > + struct intel_digital_port *intel_dig_port = intel_mst->primary; > + struct intel_dp *intel_dp = &intel_dig_port->dp; > + struct intel_connector *connector = > + to_intel_connector(old_conn_state->connector); > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > + u32 val; > + > + drm_dp_update_payload_part2(&intel_dp->mst_mgr); > + > + val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder)); > + val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC; > + I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), val); > + > + if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status, > + DP_TP_STATUS_ACT_SENT, 1)) > + DRM_ERROR("Timed out waiting for ACT sent when disabling\n"); > + > + drm_dp_check_act_status(&intel_dp->mst_mgr); > + > + drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port); > +} > + > +void > +intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state) > +{ > + struct drm_atomic_state *state = old_crtc_state->uapi.state; > + struct drm_connector_state *old_conn_state; > + struct drm_connector *conn; > + int i; > + > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) > + return; > + > + for_each_old_connector_in_state(state, conn, old_conn_state, i) { > + struct intel_encoder *encoder; > + > + if (old_conn_state->crtc != old_crtc_state->uapi.crtc) > + continue; > + > + encoder = to_intel_encoder(old_conn_state->best_encoder); > + dp_mst_post_trans_disabled(encoder, old_crtc_state, > + old_conn_state); > + } > +} > + > static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > const struct intel_crtc_state *old_crtc_state, > const struct drm_connector_state *old_conn_state) > @@ -383,6 +434,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream && > !intel_dp_mst_is_master_trans(old_crtc_state)); > > + /* > + * Power down mst path before disabling the port, otherwise we end > + * up getting interrupts from the sink upon detecting link loss. > + */ > + drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, > + false); > /* > * From TGL spec: "If multi-stream slave transcoder: Configure > * Transcoder Clock Select to direct no clock to the transcoder" > @@ -393,24 +450,20 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream) > intel_ddi_disable_pipe_clock(old_crtc_state); > > - /* this can fail */ > - drm_dp_check_act_status(&intel_dp->mst_mgr); > - /* and this can also fail */ > - drm_dp_update_payload_part2(&intel_dp->mst_mgr); > > - drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port); > + intel_mst->connector = NULL; > + if (last_mst_stream) { > + enum transcoder cpu_transcoder; > + u32 val; > > - /* > - * Power down mst path before disabling the port, otherwise we end > - * up getting interrupts from the sink upon detecting link loss. > - */ > - drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, > - false); > + cpu_transcoder = old_crtc_state->cpu_transcoder; > + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); > + val &= ~TGL_TRANS_DDI_PORT_MASK; Unconditional TGL stuff? > + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); > > - intel_mst->connector = NULL; > - if (last_mst_stream) > intel_dig_port->base.post_disable(&intel_dig_port->base, > old_crtc_state, NULL); > + } > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > } > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h > index e40767f78343..87f32fab90fc 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > @@ -16,5 +16,6 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port); > int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port); > bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state); > bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state); > +void intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state); > > #endif /* __INTEL_DP_MST_H__ */ > -- > 2.24.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Dec 10, 2019 at 11:38:50PM +0200, Ville Syrjälä wrote: > On Fri, Dec 06, 2019 at 05:18:31PM -0800, José Roberto de Souza wrote: > > The disable sequence after wait for transcoder off was not correctly > > implemented. > > The MST disable sequence is basically the same for HSW, SKL, ICL and > > TGL, with just minor changes for TGL. > > > > So here calling a new MST function to do the MST sequences, most of > > the steps just moved from the post disable hook. > > > > With this last patch we finally fixed the hotplugs triggered by MST > > sinks during the disable/enable sequence, those were causing source > > to try to do a link training while it was not ready causing CPU pipe > > FIFO underrrus on TGL. > > > > BSpec: 4231 > > BSpec: 4163 > > BSpec: 22243 > > BSpec: 49190 > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 17 +++-- > > drivers/gpu/drm/i915/display/intel_display.c | 2 + > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 79 ++++++++++++++++---- > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 1 + > > 4 files changed, 79 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index be5bc506d4d3..f06c6dfec888 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -34,6 +34,7 @@ > > #include "intel_ddi.h" > > #include "intel_display_types.h" > > #include "intel_dp.h" > > +#include "intel_dp_mst.h" > > #include "intel_dp_link_training.h" > > #include "intel_dpio_phy.h" > > #include "intel_dsi.h" > > @@ -1953,17 +1954,19 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > - i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder); > > - u32 val = I915_READ(reg); > > + u32 val; > > + > > + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); > > + val &= ~TRANS_DDI_FUNC_ENABLE; > > > > if (INTEL_GEN(dev_priv) >= 12) { > > - val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK | > > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) || > > + intel_dp_mst_is_slave_trans(crtc_state)) > > + val &= ~TGL_TRANS_DDI_PORT_MASK; > > } else { > > - val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK | > > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); > > + val &= ~TRANS_DDI_PORT_MASK; > > } > > - I915_WRITE(reg, val); > > + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); > > > > if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME && > > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 2f74c0bfb2a8..d3eefb271fa4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -6776,6 +6776,8 @@ static void haswell_crtc_disable(struct intel_atomic_state *state, > > if (!transcoder_is_dsi(cpu_transcoder)) > > intel_disable_pipe(old_crtc_state); > > > > + intel_dp_mst_post_trans_disabled(old_crtc_state); > > + > > Basically a new ad-hoc encoder hook :( > > I think we either want to suck in more crap from the crtc_enable/disable > into the encoder hooks, or we try to move everything from the current > .post_disable() into .post_pll_disable() and relocate .post_disabel() > to a better place in the sequence. Though the whole .post_pll_disable() > is a farily poor match for these platforms as there's nothing to do after > disabling the PLL. So from the hook naming POV I'd kinda want move things > the other way. > > So I think moving more things into the encoder hooks sounds like a > better plan. Eg. in this case I think we could probably shovel everything > after intel_pipe_disable() into .post_disable(). Would also help us > eliminate some of those 'if !dsi' things. > > I'm also thinking intel_disable_ddi_buf() should be split up and > just inlined into the proper place(s). Would help in making the sequence > more easily comparable with the spec. Does this refactoring and cleanup need to happen for the immediate goal of fixing MST, or can it land as a cleanup after we get this series merged and MST is functional? Thanks! -James > > > > if (INTEL_GEN(dev_priv) >= 11) > > icl_disable_transcoder_port_sync(old_crtc_state); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index 49f1518e3ab9..3c98a25e6308 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -365,6 +365,57 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, > > old_crtc_state, old_conn_state); > > } > > > > +static void > > +dp_mst_post_trans_disabled(struct intel_encoder *encoder, > > + const struct intel_crtc_state *old_crtc_state, > > + const struct drm_connector_state *old_conn_state) > > +{ > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base); > > + struct intel_digital_port *intel_dig_port = intel_mst->primary; > > + struct intel_dp *intel_dp = &intel_dig_port->dp; > > + struct intel_connector *connector = > > + to_intel_connector(old_conn_state->connector); > > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > > + u32 val; > > + > > + drm_dp_update_payload_part2(&intel_dp->mst_mgr); > > + > > + val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder)); > > + val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC; > > + I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), val); > > + > > + if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status, > > + DP_TP_STATUS_ACT_SENT, 1)) > > + DRM_ERROR("Timed out waiting for ACT sent when disabling\n"); > > + > > + drm_dp_check_act_status(&intel_dp->mst_mgr); > > + > > + drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port); > > +} > > + > > +void > > +intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state) > > +{ > > + struct drm_atomic_state *state = old_crtc_state->uapi.state; > > + struct drm_connector_state *old_conn_state; > > + struct drm_connector *conn; > > + int i; > > + > > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) > > + return; > > + > > + for_each_old_connector_in_state(state, conn, old_conn_state, i) { > > + struct intel_encoder *encoder; > > + > > + if (old_conn_state->crtc != old_crtc_state->uapi.crtc) > > + continue; > > + > > + encoder = to_intel_encoder(old_conn_state->best_encoder); > > + dp_mst_post_trans_disabled(encoder, old_crtc_state, > > + old_conn_state); > > + } > > +} > > + > > static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > > const struct intel_crtc_state *old_crtc_state, > > const struct drm_connector_state *old_conn_state) > > @@ -383,6 +434,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > > WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream && > > !intel_dp_mst_is_master_trans(old_crtc_state)); > > > > + /* > > + * Power down mst path before disabling the port, otherwise we end > > + * up getting interrupts from the sink upon detecting link loss. > > + */ > > + drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, > > + false); > > /* > > * From TGL spec: "If multi-stream slave transcoder: Configure > > * Transcoder Clock Select to direct no clock to the transcoder" > > @@ -393,24 +450,20 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, > > if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream) > > intel_ddi_disable_pipe_clock(old_crtc_state); > > > > - /* this can fail */ > > - drm_dp_check_act_status(&intel_dp->mst_mgr); > > - /* and this can also fail */ > > - drm_dp_update_payload_part2(&intel_dp->mst_mgr); > > > > - drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port); > > + intel_mst->connector = NULL; > > + if (last_mst_stream) { > > + enum transcoder cpu_transcoder; > > + u32 val; > > > > - /* > > - * Power down mst path before disabling the port, otherwise we end > > - * up getting interrupts from the sink upon detecting link loss. > > - */ > > - drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, > > - false); > > + cpu_transcoder = old_crtc_state->cpu_transcoder; > > + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); > > + val &= ~TGL_TRANS_DDI_PORT_MASK; > > Unconditional TGL stuff? > > > + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); > > > > - intel_mst->connector = NULL; > > - if (last_mst_stream) > > intel_dig_port->base.post_disable(&intel_dig_port->base, > > old_crtc_state, NULL); > > + } > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > } > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > index e40767f78343..87f32fab90fc 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > @@ -16,5 +16,6 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port); > > int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port); > > bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state); > > bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state); > > +void intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state); > > > > #endif /* __INTEL_DP_MST_H__ */ > > -- > > 2.24.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2019-12-10 at 16:30 -0800, James Ausmus wrote: > On Tue, Dec 10, 2019 at 11:38:50PM +0200, Ville Syrjälä wrote: > > On Fri, Dec 06, 2019 at 05:18:31PM -0800, José Roberto de Souza > > wrote: > > > The disable sequence after wait for transcoder off was not > > > correctly > > > implemented. > > > The MST disable sequence is basically the same for HSW, SKL, ICL > > > and > > > TGL, with just minor changes for TGL. > > > > > > So here calling a new MST function to do the MST sequences, most > > > of > > > the steps just moved from the post disable hook. > > > > > > With this last patch we finally fixed the hotplugs triggered by > > > MST > > > sinks during the disable/enable sequence, those were causing > > > source > > > to try to do a link training while it was not ready causing CPU > > > pipe > > > FIFO underrrus on TGL. > > > > > > BSpec: 4231 > > > BSpec: 4163 > > > BSpec: 22243 > > > BSpec: 49190 > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 17 +++-- > > > drivers/gpu/drm/i915/display/intel_display.c | 2 + > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 79 > > > ++++++++++++++++---- > > > drivers/gpu/drm/i915/display/intel_dp_mst.h | 1 + > > > 4 files changed, 79 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index be5bc506d4d3..f06c6dfec888 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -34,6 +34,7 @@ > > > #include "intel_ddi.h" > > > #include "intel_display_types.h" > > > #include "intel_dp.h" > > > +#include "intel_dp_mst.h" > > > #include "intel_dp_link_training.h" > > > #include "intel_dpio_phy.h" > > > #include "intel_dsi.h" > > > @@ -1953,17 +1954,19 @@ void > > > intel_ddi_disable_transcoder_func(const struct intel_crtc_state > > > *crtc_state > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > > enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > > > - i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder); > > > - u32 val = I915_READ(reg); > > > + u32 val; > > > + > > > + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); > > > + val &= ~TRANS_DDI_FUNC_ENABLE; > > > > > > if (INTEL_GEN(dev_priv) >= 12) { > > > - val &= ~(TRANS_DDI_FUNC_ENABLE | > > > TGL_TRANS_DDI_PORT_MASK | > > > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); > > > + if (!intel_crtc_has_type(crtc_state, > > > INTEL_OUTPUT_DP_MST) || > > > + intel_dp_mst_is_slave_trans(crtc_state)) > > > + val &= ~TGL_TRANS_DDI_PORT_MASK; > > > } else { > > > - val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK | > > > - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); > > > + val &= ~TRANS_DDI_PORT_MASK; > > > } > > > - I915_WRITE(reg, val); > > > + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); > > > > > > if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME && > > > intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 2f74c0bfb2a8..d3eefb271fa4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -6776,6 +6776,8 @@ static void haswell_crtc_disable(struct > > > intel_atomic_state *state, > > > if (!transcoder_is_dsi(cpu_transcoder)) > > > intel_disable_pipe(old_crtc_state); > > > > > > + intel_dp_mst_post_trans_disabled(old_crtc_state); > > > + > > > > Basically a new ad-hoc encoder hook :( > > > > I think we either want to suck in more crap from the > > crtc_enable/disable > > into the encoder hooks, or we try to move everything from the > > current > > .post_disable() into .post_pll_disable() and relocate > > .post_disabel() > > to a better place in the sequence. Though the whole > > .post_pll_disable() > > is a farily poor match for these platforms as there's nothing to do > > after > > disabling the PLL. So from the hook naming POV I'd kinda want move > > things > > the other way. > > > > So I think moving more things into the encoder hooks sounds like a > > better plan. Eg. in this case I think we could probably shovel > > everything > > after intel_pipe_disable() into .post_disable(). Would also help us > > eliminate some of those 'if !dsi' things. > > > > I'm also thinking intel_disable_ddi_buf() should be split up and > > just inlined into the proper place(s). Would help in making the > > sequence > > more easily comparable with the spec. > > Does this refactoring and cleanup need to happen for the immediate > goal > of fixing MST, or can it land as a cleanup after we get this series > merged and MST is functional? > > Thanks! I'm about to send a new version without this changes but with the "Unconditional TGL stuff?" fixed. > > -James > > > > > > if (INTEL_GEN(dev_priv) >= 11) > > > icl_disable_transcoder_port_sync(old_crtc_state); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index 49f1518e3ab9..3c98a25e6308 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -365,6 +365,57 @@ static void intel_mst_disable_dp(struct > > > intel_encoder *encoder, > > > old_crtc_state, > > > old_conn_state); > > > } > > > > > > +static void > > > +dp_mst_post_trans_disabled(struct intel_encoder *encoder, > > > + const struct intel_crtc_state > > > *old_crtc_state, > > > + const struct drm_connector_state > > > *old_conn_state) > > > +{ > > > + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder- > > > >base); > > > + struct intel_digital_port *intel_dig_port = intel_mst->primary; > > > + struct intel_dp *intel_dp = &intel_dig_port->dp; > > > + struct intel_connector *connector = > > > + to_intel_connector(old_conn_state->connector); > > > + struct drm_i915_private *dev_priv = to_i915(connector- > > > >base.dev); > > > + u32 val; > > > + > > > + drm_dp_update_payload_part2(&intel_dp->mst_mgr); > > > + > > > + val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state- > > > >cpu_transcoder)); > > > + val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC; > > > + I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), > > > val); > > > + > > > + if (intel_de_wait_for_set(dev_priv, intel_dp- > > > >regs.dp_tp_status, > > > + DP_TP_STATUS_ACT_SENT, 1)) > > > + DRM_ERROR("Timed out waiting for ACT sent when > > > disabling\n"); > > > + > > > + drm_dp_check_act_status(&intel_dp->mst_mgr); > > > + > > > + drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector- > > > >port); > > > +} > > > + > > > +void > > > +intel_dp_mst_post_trans_disabled(const struct intel_crtc_state > > > *old_crtc_state) > > > +{ > > > + struct drm_atomic_state *state = old_crtc_state->uapi.state; > > > + struct drm_connector_state *old_conn_state; > > > + struct drm_connector *conn; > > > + int i; > > > + > > > + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) > > > + return; > > > + > > > + for_each_old_connector_in_state(state, conn, old_conn_state, i) > > > { > > > + struct intel_encoder *encoder; > > > + > > > + if (old_conn_state->crtc != old_crtc_state->uapi.crtc) > > > + continue; > > > + > > > + encoder = to_intel_encoder(old_conn_state- > > > >best_encoder); > > > + dp_mst_post_trans_disabled(encoder, old_crtc_state, > > > + old_conn_state); > > > + } > > > +} > > > + > > > static void intel_mst_post_disable_dp(struct intel_encoder > > > *encoder, > > > const struct intel_crtc_state > > > *old_crtc_state, > > > const struct drm_connector_state > > > *old_conn_state) > > > @@ -383,6 +434,12 @@ static void intel_mst_post_disable_dp(struct > > > intel_encoder *encoder, > > > WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream && > > > !intel_dp_mst_is_master_trans(old_crtc_state)); > > > > > > + /* > > > + * Power down mst path before disabling the port, otherwise we > > > end > > > + * up getting interrupts from the sink upon detecting link > > > loss. > > > + */ > > > + drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector- > > > >port, > > > + false); > > > /* > > > * From TGL spec: "If multi-stream slave transcoder: Configure > > > * Transcoder Clock Select to direct no clock to the > > > transcoder" > > > @@ -393,24 +450,20 @@ static void > > > intel_mst_post_disable_dp(struct intel_encoder *encoder, > > > if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream) > > > intel_ddi_disable_pipe_clock(old_crtc_state); > > > > > > - /* this can fail */ > > > - drm_dp_check_act_status(&intel_dp->mst_mgr); > > > - /* and this can also fail */ > > > - drm_dp_update_payload_part2(&intel_dp->mst_mgr); > > > > > > - drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector- > > > >port); > > > + intel_mst->connector = NULL; > > > + if (last_mst_stream) { > > > + enum transcoder cpu_transcoder; > > > + u32 val; > > > > > > - /* > > > - * Power down mst path before disabling the port, otherwise we > > > end > > > - * up getting interrupts from the sink upon detecting link > > > loss. > > > - */ > > > - drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector- > > > >port, > > > - false); > > > + cpu_transcoder = old_crtc_state->cpu_transcoder; > > > + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); > > > + val &= ~TGL_TRANS_DDI_PORT_MASK; > > > > Unconditional TGL stuff? > > > > > + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); > > > > > > - intel_mst->connector = NULL; > > > - if (last_mst_stream) > > > intel_dig_port->base.post_disable(&intel_dig_port- > > > >base, > > > old_crtc_state, > > > NULL); > > > + } > > > > > > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); > > > } > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > index e40767f78343..87f32fab90fc 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h > > > @@ -16,5 +16,6 @@ void intel_dp_mst_encoder_cleanup(struct > > > intel_digital_port *intel_dig_port); > > > int intel_dp_mst_encoder_active_links(struct intel_digital_port > > > *intel_dig_port); > > > bool intel_dp_mst_is_master_trans(const struct intel_crtc_state > > > *crtc_state); > > > bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state > > > *crtc_state); > > > +void intel_dp_mst_post_trans_disabled(const struct > > > intel_crtc_state *old_crtc_state); > > > > > > #endif /* __INTEL_DP_MST_H__ */ > > > -- > > > 2.24.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index be5bc506d4d3..f06c6dfec888 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -34,6 +34,7 @@ #include "intel_ddi.h" #include "intel_display_types.h" #include "intel_dp.h" +#include "intel_dp_mst.h" #include "intel_dp_link_training.h" #include "intel_dpio_phy.h" #include "intel_dsi.h" @@ -1953,17 +1954,19 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; - i915_reg_t reg = TRANS_DDI_FUNC_CTL(cpu_transcoder); - u32 val = I915_READ(reg); + u32 val; + + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); + val &= ~TRANS_DDI_FUNC_ENABLE; if (INTEL_GEN(dev_priv) >= 12) { - val &= ~(TRANS_DDI_FUNC_ENABLE | TGL_TRANS_DDI_PORT_MASK | - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) || + intel_dp_mst_is_slave_trans(crtc_state)) + val &= ~TGL_TRANS_DDI_PORT_MASK; } else { - val &= ~(TRANS_DDI_FUNC_ENABLE | TRANS_DDI_PORT_MASK | - TRANS_DDI_DP_VC_PAYLOAD_ALLOC); + val &= ~TRANS_DDI_PORT_MASK; } - I915_WRITE(reg, val); + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); if (dev_priv->quirks & QUIRK_INCREASE_DDI_DISABLED_TIME && intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 2f74c0bfb2a8..d3eefb271fa4 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6776,6 +6776,8 @@ static void haswell_crtc_disable(struct intel_atomic_state *state, if (!transcoder_is_dsi(cpu_transcoder)) intel_disable_pipe(old_crtc_state); + intel_dp_mst_post_trans_disabled(old_crtc_state); + if (INTEL_GEN(dev_priv) >= 11) icl_disable_transcoder_port_sync(old_crtc_state); diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 49f1518e3ab9..3c98a25e6308 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -365,6 +365,57 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder, old_crtc_state, old_conn_state); } +static void +dp_mst_post_trans_disabled(struct intel_encoder *encoder, + const struct intel_crtc_state *old_crtc_state, + const struct drm_connector_state *old_conn_state) +{ + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base); + struct intel_digital_port *intel_dig_port = intel_mst->primary; + struct intel_dp *intel_dp = &intel_dig_port->dp; + struct intel_connector *connector = + to_intel_connector(old_conn_state->connector); + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); + u32 val; + + drm_dp_update_payload_part2(&intel_dp->mst_mgr); + + val = I915_READ(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder)); + val &= ~TRANS_DDI_DP_VC_PAYLOAD_ALLOC; + I915_WRITE(TRANS_DDI_FUNC_CTL(old_crtc_state->cpu_transcoder), val); + + if (intel_de_wait_for_set(dev_priv, intel_dp->regs.dp_tp_status, + DP_TP_STATUS_ACT_SENT, 1)) + DRM_ERROR("Timed out waiting for ACT sent when disabling\n"); + + drm_dp_check_act_status(&intel_dp->mst_mgr); + + drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port); +} + +void +intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state) +{ + struct drm_atomic_state *state = old_crtc_state->uapi.state; + struct drm_connector_state *old_conn_state; + struct drm_connector *conn; + int i; + + if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) + return; + + for_each_old_connector_in_state(state, conn, old_conn_state, i) { + struct intel_encoder *encoder; + + if (old_conn_state->crtc != old_crtc_state->uapi.crtc) + continue; + + encoder = to_intel_encoder(old_conn_state->best_encoder); + dp_mst_post_trans_disabled(encoder, old_crtc_state, + old_conn_state); + } +} + static void intel_mst_post_disable_dp(struct intel_encoder *encoder, const struct intel_crtc_state *old_crtc_state, const struct drm_connector_state *old_conn_state) @@ -383,6 +434,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, WARN_ON(INTEL_GEN(dev_priv) >= 12 && last_mst_stream && !intel_dp_mst_is_master_trans(old_crtc_state)); + /* + * Power down mst path before disabling the port, otherwise we end + * up getting interrupts from the sink upon detecting link loss. + */ + drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, + false); /* * From TGL spec: "If multi-stream slave transcoder: Configure * Transcoder Clock Select to direct no clock to the transcoder" @@ -393,24 +450,20 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder, if (INTEL_GEN(dev_priv) < 12 || !last_mst_stream) intel_ddi_disable_pipe_clock(old_crtc_state); - /* this can fail */ - drm_dp_check_act_status(&intel_dp->mst_mgr); - /* and this can also fail */ - drm_dp_update_payload_part2(&intel_dp->mst_mgr); - drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port); + intel_mst->connector = NULL; + if (last_mst_stream) { + enum transcoder cpu_transcoder; + u32 val; - /* - * Power down mst path before disabling the port, otherwise we end - * up getting interrupts from the sink upon detecting link loss. - */ - drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, - false); + cpu_transcoder = old_crtc_state->cpu_transcoder; + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder)); + val &= ~TGL_TRANS_DDI_PORT_MASK; + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val); - intel_mst->connector = NULL; - if (last_mst_stream) intel_dig_port->base.post_disable(&intel_dig_port->base, old_crtc_state, NULL); + } DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links); } diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h index e40767f78343..87f32fab90fc 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.h +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h @@ -16,5 +16,6 @@ void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port); int intel_dp_mst_encoder_active_links(struct intel_digital_port *intel_dig_port); bool intel_dp_mst_is_master_trans(const struct intel_crtc_state *crtc_state); bool intel_dp_mst_is_slave_trans(const struct intel_crtc_state *crtc_state); +void intel_dp_mst_post_trans_disabled(const struct intel_crtc_state *old_crtc_state); #endif /* __INTEL_DP_MST_H__ */
The disable sequence after wait for transcoder off was not correctly implemented. The MST disable sequence is basically the same for HSW, SKL, ICL and TGL, with just minor changes for TGL. So here calling a new MST function to do the MST sequences, most of the steps just moved from the post disable hook. With this last patch we finally fixed the hotplugs triggered by MST sinks during the disable/enable sequence, those were causing source to try to do a link training while it was not ready causing CPU pipe FIFO underrrus on TGL. BSpec: 4231 BSpec: 4163 BSpec: 22243 BSpec: 49190 Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 17 +++-- drivers/gpu/drm/i915/display/intel_display.c | 2 + drivers/gpu/drm/i915/display/intel_dp_mst.c | 79 ++++++++++++++++---- drivers/gpu/drm/i915/display/intel_dp_mst.h | 1 + 4 files changed, 79 insertions(+), 20 deletions(-)