[Intel-gfx] [PATCH 2/6] drm/i915/dp_mst: Disable link training fallback on MST links

[Imre Deak]

On Tue, Jun 16, 2020 at 06:22:51PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 16, 2020 at 05:18:51PM +0300, Imre Deak wrote:
> > We calculate the MST available bandwidth using the link's maximum rate
> > and lane count. This bandwidth won't be recalculated in response to a
> 
> Thw wording here is a bit ambiguousr as to who "we" is, and what exactly
> "link's maximum rate and lane count". I would try to clarify a bit that
> it's drm_dp_mst_topology.c who is mostly in error here by directly
> interpreting the max data rate/lanes from the DPCD.
> 
> Althoguh the i915 code is not wihtout faults, as it lacks any logic
> to modeset all the MST streams for this link when the link params
> change (except on tgl+ where the master/slave stuff should force
> all streams to do a modeset anyway).
> 
> > link training error and fallback setting, so modesets following a link
> > training error will calculate incorrect timing parameters (like the
> > transcoder's data M/N params or the number of MST TUs).
> > 
> > Prevent the above problem by disabling the link training fallback on MST
> > links for now, until the MST compute config can deal with changing link
> > parameters.
> > 
> > The misconfigured timing lead at least to a
> > 'Timed out waiting for DP idle patterns'
> > error.
> > 
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 42589cae766d..c585b002783a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -468,6 +468,13 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >  	int index;
> >  
> > +	/*
> > +	 * TODO: Enable fallback on MST links once MST link compute can handle
> > +	 * the fallback params.
> > +	 */
> > +	if (intel_dp->is_mst)
> > +		return -1;
> 
> Should we duplicate the drm_error() from the other path here maybe?

Yes, will add it.

> 
> > +
> >  	index = intel_dp_rate_index(intel_dp->common_rates,
> >  				    intel_dp->num_common_rates,
> >  				    link_rate);
> > @@ -6163,7 +6170,17 @@ intel_dp_detect(struct drm_connector *connector,
> >  		goto out;
> >  	}
> >  
> > -	if (intel_dp->reset_link_params) {
> > +	/* Read DP Sink DSC Cap DPCD regs for DP v1.4 */
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		intel_dp_get_dsc_sink_cap(intel_dp);
> > +
> > +	intel_dp_configure_mst(intel_dp);
> > +
> > +	/*
> > +	 * TODO: Reset link params when switching to MST mode, until MST
> > +	 * supports link training fallback params.
> > +	 */
> > +	if (intel_dp->reset_link_params || intel_dp->is_mst) {
> 
> /me confused. Why do we need to touch this code?

It's possible to switch to MST mode after the link rate/lane count got
reduced by a link training error in SST mode.

> >  		/* Initial max link lane count */
> >  		intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
> >  
> > @@ -6175,12 +6192,6 @@ intel_dp_detect(struct drm_connector *connector,
> >  
> >  	intel_dp_print_rates(intel_dp);
> >  
> > -	/* Read DP Sink DSC Cap DPCD regs for DP v1.4 */
> > -	if (INTEL_GEN(dev_priv) >= 11)
> > -		intel_dp_get_dsc_sink_cap(intel_dp);
> > -
> > -	intel_dp_configure_mst(intel_dp);
> > -
> >  	if (intel_dp->is_mst) {
> >  		/*
> >  		 * If we are in MST mode then this connector
> > -- 
> > 2.23.1
> 
> -- 
> Ville Syrjälä
> Intel