On Wed, Dec 18, 2019 at 10:59:08AM -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.
>
> 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.
>
> v2: Only unsetting TGL_TRANS_DDI_PORT_MASK for TGL on the post
> disable sequence
>
> v4: Rebased, moved MST sequences to intel_mst_post_disable_dp()
>
> BSpec: 4231
> BSpec: 4163
> BSpec: 22243
> BSpec: 49190
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 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 | 33 +++++++++++++++------
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 33 +++++++++++++--------
> 2 files changed, 44 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 9d99ec82d072..94ca26be2fee 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"
> @@ -1949,17 +1950,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))
if (!intel_dp_mst_is_master_trans())
val &= ...;
?
> + 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)) {
> @@ -3808,8 +3811,20 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
> */
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>
> - if (INTEL_GEN(dev_priv) < 12 && !is_mst)
> - intel_ddi_disable_pipe_clock(old_crtc_state);
> + if (INTEL_GEN(dev_priv) >= 12) {
> + if (is_mst) {
if (intel_dp_mst_is_master_trans()) {
?
> + enum transcoder cpu_transcoder;
> + u32 val;
> +
> + cpu_transcoder = old_crtc_state->cpu_transcoder;
Assignment can be done when declaring the variable.
> + val = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> + val &= ~TGL_TRANS_DDI_PORT_MASK;
> + I915_WRITE(TRANS_DDI_FUNC_CTL(cpu_transcoder), val);
> + }
> + } else {
> + if (!is_mst)
> + intel_ddi_disable_pipe_clock(old_crtc_state);
> + }
>
> intel_disable_ddi_buf(encoder, 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 710137984c71..efd14b0b507b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -347,6 +347,7 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> to_intel_connector(old_conn_state->connector);
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> bool last_mst_stream;
> + u32 val;
>
> intel_dp->active_mst_links--;
> last_mst_stream = intel_dp->active_mst_links == 0;
> @@ -357,6 +358,19 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
>
> intel_disable_pipe(old_crtc_state);
>
> + drm_dp_update_payload_part2(&intel_dp->mst_mgr);
Hmm. I'm having hard to figuring out what these things do. But from a
cursory glance it almost looks like part1 is the one that does the AUX
stuff to deallocate stuff. So feels like even that part should be here.
> +
> + 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");
I guess we were missing this step entirely. Dunno if we want ERROR for
this. Not sure how much noise it'll generate when someone forcefully
yanks the cable...
> + drm_dp_check_act_status(&intel_dp->mst_mgr);
> +
> + drm_dp_mst_deallocate_vcpi(&intel_dp->mst_mgr, connector->port);
This seems to be pure sw stuff so not so critical where it is. Keeping
it next to the hw payload deallocation seems like a good approach.
> +
> intel_ddi_disable_transcoder_func(old_crtc_state);
>
> if (INTEL_GEN(dev_priv) >= 9)
> @@ -364,6 +378,12 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> else
> ironlake_pfit_disable(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);
And I guess this was needed to avoid the link loss screams from the sink?
Seems to match the spec better than before at least. Still a bit unsure
about he part1/2 stuff for the deallocation. But if that should be
changed we can do it as a followup.
The code movement into .post_disable() really did make for a much
nicer experience when comparing with the spec. Or at least I didn't
lose as much hair as with the previous version :)
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> /*
> * From TGL spec: "If multi-stream slave transcoder: Configure
> * Transcoder Clock Select to direct no clock to the transcoder"
> @@ -374,19 +394,6 @@ 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);
> -
> - /*
> - * 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);
>
> intel_mst->connector = NULL;
> if (last_mst_stream)
> --
> 2.24.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
