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

[Ville Syrjälä]

On Tue, Jun 16, 2020 at 06:49:20PM +0300, Imre Deak wrote:
> On Tue, Jun 16, 2020 at 06:39:30PM +0300, Ville Syrjälä wrote:
> > On Tue, Jun 16, 2020 at 06:30:55PM +0300, Imre Deak wrote:
> > > 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.
> > 
> > But then we should have a long hpd and reset_link_params should be set
> > anyway no?
> 
> I meant switching the mode for the same sink as it would change its
> DP_MST_CAP. I'm not sure if a long HPD is required in that case. 

I would expect so. I don't think there's a requirement in the spec to
re-evaluate this sort of stuff for a short hpd.

> Also if
> we had a long HPD after a mode change couldn't a modeset run before the
> next intel_dp_detect() call could reset the link params?

This is detect() we're talking about here, so I guess you mean
detect() running before the hpd gets to flag reset_link_params=true ?
Not sure I'd worry about that, but I guess there should be no harm
in reordering these things a bit.

Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

> 
> > > 
> > > > >  		/* 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel