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.
> 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
