Re: [PATCH 08/17] drm/i915/ddi: Move platform checks within mtl_ddi_enable/disable_d2d_link()

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

On Wed, Feb 05, 2025 at 02:42:12PM +0200, Jani Nikula wrote:
> On Wed, 29 Jan 2025, Imre Deak <imre.deak@intel.com> wrote:
> > The prefix of the mtl_ddi_enable_d2d() / mtl_ddi_disable_d2d_link()
> > names show already what are the relevant platforms, so the corresponding
> > platform check is a detail that can be hidden in the functions, do so.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> But unrelated bafflement below.
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index d040558b5d029..07188606a0177 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2545,6 +2545,9 @@ mtl_ddi_enable_d2d(struct intel_encoder *encoder)
> >  	i915_reg_t reg;
> >  	u32 set_bits, wait_bits;
> >  
> > +	if (DISPLAY_VER(dev_priv) < 14)
> > +		return;
> > +
> >  	if (DISPLAY_VER(dev_priv) >= 20) {
> >  		reg = DDI_BUF_CTL(port);
> 
> Unrelated to this patch, this seems weird. DDI_BUF_CTL for display
> version 20+? 

Yes, some DDI buffer control/status flags got shuffled around across
different platforms. The D2D enabling flag was originally added on MTL
in the new XELPDP_PORT_BUF_CTL1 register, then on LNL this same D2D
enabling flag was moved (back) to the old DDI_BUF_CTL register. A
similar shuffling happened with the DDI buffer (PHY) enabled _status_
flag (vs. the DDI buffer enabled _control_ flag, which was not
shuffled).

> While XELPDP_PORT_BUF_CTL1() also has a check for >= 20.

Yes, flags other than the D2D enable control/status are in
XELPDP_PORT_BUF_CTL1 reg for LNL+. So both MTL and LNL+ uses this
register, but the mapping of the register for port A/B changed on LNL+.

> 
> >  		set_bits = XE2LPD_DDI_BUF_D2D_LINK_ENABLE;
> > @@ -3035,6 +3038,9 @@ mtl_ddi_disable_d2d_link(struct intel_encoder *encoder)
> >  	i915_reg_t reg;
> >  	u32 clr_bits, wait_bits;
> >  
> > +	if (DISPLAY_VER(dev_priv) < 14)
> > +		return;
> > +
> >  	if (DISPLAY_VER(dev_priv) >= 20) {
> >  		reg = DDI_BUF_CTL(port);
> >  		clr_bits = XE2LPD_DDI_BUF_D2D_LINK_ENABLE;
> > @@ -3411,8 +3417,7 @@ static void intel_ddi_enable_hdmi(struct intel_atomic_state *state,
> >  		hsw_prepare_hdmi_ddi_buffers(encoder, crtc_state);
> >  
> >  	/* e. Enable D2D Link for C10/C20 Phy */
> > -	if (DISPLAY_VER(dev_priv) >= 14)
> > -		mtl_ddi_enable_d2d(encoder);
> > +	mtl_ddi_enable_d2d(encoder);
> 
> Why do we have mtl_ddi_enable_d2d() and mtl_ddi_disable_d2d_link(). It
> just boggles my mind that the names mismatch.

Yes, it's not consistent. I can include the
s/mtl_ddi_disable_d2d_link/mtl_ddi_disable_ddi/
change in this patch.

> >  
> >  	encoder->set_signal_levels(encoder, crtc_state);
> 
> -- 
> Jani Nikula, Intel