On Wed, Feb 05, 2025 at 02:42:12PM +0200, Jani Nikula wrote:
> On Wed, 29 Jan 2025, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> 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
