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@xxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
