[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 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?

> +
>  	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?

>  		/* 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