Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp: Fix MST disable sequences

From: "Souza, Jose" <jose.souza@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
	"Ausmus,  James" <james.ausmus@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"De Marchi, Lucas" <lucas.demarchi@intel.com>
Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp: Fix MST disable sequences
Date: Wed, 11 Dec 2019 02:07:26 +0000	[thread overview]
Message-ID: <691fe1a2a9ef90cc4160f4331c6bd3f8335ecea4.camel@intel.com> (raw)
In-Reply-To: <20191211003015.GA4880@jausmus-gentoo-dev6.jf.intel.com>

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx