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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux