Re: [PATCH 05/17] drm/i915/ddi: Simplify the port enabling via DDI_BUF_CTL

[Imre Deak <imre.deak@intel.com>]

On Thu, Jan 30, 2025 at 01:55:38PM +0200, Jani Nikula wrote:
> On Wed, 29 Jan 2025, Imre Deak <imre.deak@intel.com> wrote:
> > In the past intel_digital_port::dp.prepare_link_retrain() could be
> > called directly (vs. from a modeset) to retrain an enabled link. In that
> > case the port had to be first disabled and then re-enabled. That changed
> > with commit 2885d283cce5 ("drm/i915/dp: Retrain SST links via a modeset
> > commit"), after which the only way prepare_link_retrain() can be called
> > is from a modeset during link training when the port is still disabled.
> > Simplify things accordingly, assuming the disabled port state.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 23 ++++-------------------
> >  1 file changed, 4 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 6192c0d3c87a5..3138dc4034797 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3743,8 +3743,8 @@ static void mtl_ddi_prepare_link_retrain(struct intel_dp *intel_dp,
> >  	 * necessary disable and enable port
> >  	 */
> >  	dp_tp_ctl = intel_de_read(display, dp_tp_ctl_reg(encoder, crtc_state));
> > -	if (dp_tp_ctl & DP_TP_CTL_ENABLE)
> > -		mtl_disable_ddi_buf(encoder, crtc_state);
> > +
> > +	drm_WARN_ON(display->drm, dp_tp_ctl & DP_TP_CTL_ENABLE);
> >  
> >  	/* 6.d Configure and enable DP_TP_CTL with link training pattern 1 selected */
> >  	dp_tp_ctl = DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_PAT1;
> > @@ -3787,26 +3787,11 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp,
> >  	struct intel_encoder *encoder = &dig_port->base;
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	enum port port = encoder->port;
> > -	u32 dp_tp_ctl, ddi_buf_ctl;
> > -	bool wait = false;
> > +	u32 dp_tp_ctl;
> >  
> >  	dp_tp_ctl = intel_de_read(dev_priv, dp_tp_ctl_reg(encoder, crtc_state));
> >  
> > -	if (dp_tp_ctl & DP_TP_CTL_ENABLE) {
> > -		ddi_buf_ctl = intel_de_read(dev_priv, DDI_BUF_CTL(port));
> > -		if (ddi_buf_ctl & DDI_BUF_CTL_ENABLE) {
> > -			intel_de_write(dev_priv, DDI_BUF_CTL(port),
> > -				       ddi_buf_ctl & ~DDI_BUF_CTL_ENABLE);
> > -			wait = true;
> > -		}
> > -
> > -		dp_tp_ctl &= ~DP_TP_CTL_ENABLE;
> > -		intel_de_write(dev_priv, dp_tp_ctl_reg(encoder, crtc_state), dp_tp_ctl);
> > -		intel_de_posting_read(dev_priv, dp_tp_ctl_reg(encoder, crtc_state));
> > -
> > -		if (wait)
> > -			intel_wait_ddi_buf_idle(dev_priv, port);
> > -	}
> > +	drm_WARN_ON(&dev_priv->drm, dp_tp_ctl & DP_TP_CTL_ENABLE);
> 
> Please don't add new dev_priv uses. Add the local display variable and
> use display->drm.

Ok, will update that.

> With that fixed,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >  
> >  	dp_tp_ctl = DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_PAT1;
> >  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST) ||
> 
> -- 
> Jani Nikula, Intel