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

[Imre Deak]

On Thu, May 23, 2024 at 03:58:30PM +0300, Ville Syrjälä wrote:
> 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 at 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

Ok.

> >  	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.

Yes, mixed SST/MST CRTCs connected to the same encoder is what you
mean I guess. It would have caused a failure elsewhere or make the
commit added here fail during atomic check.

> Reviewed-by: Ville Syrjälä <ville.syrjala at 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