[Intel-gfx] [PATCH] drm/i915/mst: re-enable link training failure fallback for DP MST

[Ville Syrjälä]

On Mon, Mar 07, 2022 at 09:36:57PM +0200, Jani Nikula wrote:
> Commit 80a8cecf62a5 ("drm/i915/dp_mst: Disable link training fallback on
> MST links") disabled link training failure fallback for DP MST due to
> the MST manager using the DPCD directly, and generally being ignorant
> about the possibility of downgrading link parameters. See the commit for
> further details.
> 
> Since then, the max_lane_count and max_link_rate members have been added
> to struct drm_dp_mst_topology_mgr in commit 98025a62cb00 ("drm/dp_mst:
> Use Extended Base Receiver Capability DPCD space") and refined in
> follow-up work.
> 
> The members perhaps aren't intended for changing the parameters during
> the lifetime of the manager, as they're supposed to be passed to
> drm_dp_mst_topology_mgr_init(). However, the members are only ever used
> in drm_dp_mst_topology_mgr_set_mst(), and there seems to be nothing to
> prevent us from adjusting them *before* enabling MST. The wouldn't have
> an effect if modified while MST is enabled. This is not necessarily
> pretty, though.
> 
> Cc: Nikola Cornij <nikola.cornij at amd.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Uma Shankar <uma.shankar at intel.com>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> 
> ---
> 
> This is *untested*. I don't see why it wouldn't work, though...

I don't think we have the required stuff to force a modeset on all
the streams when the link params change. And the bad link status
property + uevent stuff is only hooked up to the SST connector
AFAICS.

The other major thing missing is a way to reduce the bpp/etc. of
all the streams to make more room on the link if we have
insufficient bandwidth. And the more we start to reduce the bw
the more we're going to hit that and fail the modesets. We already
ran into regressions due to this when I tried to enable deep color
for MST.

> this
> should allow us to downgrade the link to from 128b/132b to 8b/10b if the
> former fails.
> 
> Thoughts? In particular, any objections for messing with the topology
> manager members directly? Any chance it'll make refactoring the MST code
> more difficult?
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 42 ++++++++++-----------
>  drivers/gpu/drm/i915/display/intel_dp_mst.c |  5 ++-
>  2 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 619546441eae..2fad3104b40e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -600,15 +600,6 @@ 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) {
> -		drm_err(&i915->drm, "Link Training Unsuccessful\n");
> -		return -1;
> -	}
> -
>  	if (intel_dp_is_edp(intel_dp) && !intel_dp->use_max_params) {
>  		drm_dbg_kms(&i915->drm,
>  			    "Retrying Link training for eDP with max parameters\n");
> @@ -2785,6 +2776,8 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	struct intel_encoder *encoder =
>  		&dp_to_dig_port(intel_dp)->base;
> +	struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
> +
>  	bool sink_can_mst = drm_dp_read_mst_cap(&intel_dp->aux, intel_dp->dpcd);
>  
>  	drm_dbg_kms(&i915->drm,
> @@ -2800,8 +2793,17 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>  	intel_dp->is_mst = sink_can_mst &&
>  		i915->params.enable_dp_mst;
>  
> -	drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> -					intel_dp->is_mst);
> +	/*
> +	 * Set the source max lane count and link rate using the possibly
> +	 * limited values due to failed link training.
> +	 *
> +	 * This is a bit hackish, as the values are supposed to be passed to
> +	 * drm_dp_mst_topology_mgr_init().
> +	 */
> +	mgr->max_lane_count = intel_dp->max_link_lane_count;
> +	mgr->max_link_rate = intel_dp->max_link_rate;
> +
> +	drm_dp_mst_topology_mgr_set_mst(mgr, intel_dp->is_mst);
>  }
>  
>  static bool
> @@ -4472,23 +4474,19 @@ intel_dp_detect(struct drm_connector *connector,
>  		goto out;
>  	}
>  
> -	/* Read DP Sink DSC Cap DPCD regs for DP v1.4 */
> -	if (DISPLAY_VER(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) {
> +	if (intel_dp->reset_link_params) {
>  		intel_dp_reset_max_link_params(intel_dp);
>  		intel_dp->reset_link_params = false;
>  	}
>  
>  	intel_dp_print_rates(intel_dp);
>  
> +	/* Read DP Sink DSC Cap DPCD regs for DP v1.4 */
> +	if (DISPLAY_VER(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
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index e30e698aa684..442dbd0ed201 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -151,8 +151,9 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  			intel_conn_state->force_audio == HDMI_AUDIO_ON;
>  
>  	/*
> -	 * for MST we always configure max link bw - the spec doesn't
> -	 * seem to suggest we should do otherwise.
> +	 * For MST we always configure max link bw - the spec doesn't seem to
> +	 * suggest we should do otherwise. The values may be reduced due to link
> +	 * training failures, however.
>  	 */
>  	limits.min_rate =
>  	limits.max_rate = intel_dp_max_link_rate(intel_dp);
> -- 
> 2.30.2

-- 
Ville Syrjälä
Intel