Re: [PATCH v2 06/21] drm/i915/dp: Use a commit modeset for link retraining MST links

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, May 20, 2024 at 09:58:04PM +0300, Imre Deak wrote:
> Instead of direct calls to the link train functions, retrain the link
> via a commit modeset. The direct call means that the output port will be
> disabled/re-enabled while the rest of the pipeline (transcoder) is
> active, which doesn't seem to work on MST at least. It leads to
> underruns and black screen, presumedly because the transcoder is not
> disabled/re-enabled along the port.
> 
> Leave switching to a commit modeset on SST for a later patchset, as that
> seems to work ok currently (though better to using a commit there too,
> due to the suppressed underruns).
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 81e620dd33bb7..120f7b420807b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5147,6 +5147,7 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct intel_crtc *crtc;
>  	u8 pipe_mask;
> +	bool mst_output = false;

nit: maybe move that up one line to maintain a bit more of a steady slope

>  	int ret;
>  
>  	if (!intel_dp_is_connected(intel_dp))
> @@ -5177,6 +5178,11 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
>  		const struct intel_crtc_state *crtc_state =
>  			to_intel_crtc_state(crtc->base.state);
>  
> +		if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)) {
> +			mst_output = true;
> +			break;
> +		}

I was pondering if we need a bit more care to make sure all
the pipes agree, but I suppose if that wasn't the case
check_digital_port_conflicts() would have a failed at its
job. So this seems fine.

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

> +
>  		/* Suppress underruns caused by re-training */
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
>  		if (crtc_state->has_pch_encoder)
> @@ -5184,16 +5190,23 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
>  							      intel_crtc_pch_transcoder(crtc), false);
>  	}
>  
> +	/* TODO: use a modeset for SST as well. */
> +	if (mst_output) {
> +		ret = intel_modeset_commit_pipes(dev_priv, pipe_mask, ctx);
> +
> +		if (ret && ret != -EDEADLK)
> +			drm_dbg_kms(&dev_priv->drm,
> +				    "[ENCODER:%d:%s] link retraining failed: %pe\n",
> +				    encoder->base.base.id, encoder->base.name,
> +				    ERR_PTR(ret));
> +
> +		return ret;
> +	}
> +
>  	for_each_intel_crtc_in_pipe_mask(&dev_priv->drm, crtc, pipe_mask) {
>  		const struct intel_crtc_state *crtc_state =
>  			to_intel_crtc_state(crtc->base.state);
>  
> -		/* retrain on the MST master transcoder */
> -		if (DISPLAY_VER(dev_priv) >= 12 &&
> -		    intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) &&
> -		    !intel_dp_mst_is_master_trans(crtc_state))
> -			continue;
> -
>  		intel_dp_check_frl_training(intel_dp);
>  		intel_dp_pcon_dsc_configure(intel_dp, crtc_state);
>  		intel_dp_start_link_train(intel_dp, crtc_state);
> -- 
> 2.43.3

-- 
Ville Syrjälä
Intel