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@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!
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@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
