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

[Jani Nikula <jani.nikula@linux.intel.com>]

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+? While XELPDP_PORT_BUF_CTL1() also has a check for >= 20.

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


>  
>  	encoder->set_signal_levels(encoder, crtc_state);

-- 
Jani Nikula, Intel