Re: [PATCH v2 05/10] drm/i915/display: Add bits for link_n_extended for DISPLAY >= 14

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915: Redundant DISPLAY_VER split despite commit message claim
--

commit 42050e03ecee4bb8780c62f3acc4e0b55c85e966
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

drm/i915/display: Add bits for link_n_extended for DISPLAY >= 14

LINK_N register has bits 31:24 for extended link N value used for
HDMI2.1 and for an alternate mode of operation of DP TG DDA. This commit
adds the definition for these extra bits and masks the software link_n
value down to the low 24 bits in intel_set_m_n().

>     v5: Drop the now redundant DISPLAY_VER split, the two masks were
>         identical.

[Severity: Medium]
Is this description accurate? The commit message states the redundant
DISPLAY_VER split was dropped, but it appears to still be present in both
intel_set_m_n() and intel_get_m_n().

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c53163d7c082e..6d2025e109956 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2711,14 +2711,22 @@ void intel_set_m_n(struct intel_display *display,
>  		   intel_reg_t data_m_reg, intel_reg_t data_n_reg,
>  		   intel_reg_t link_m_reg, intel_reg_t link_n_reg)
>  {
> +	u32 link_n = m_n->link_n;
> +
>  	intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m);
>  	intel_de_write(display, data_n_reg, m_n->data_n);
>  	intel_de_write(display, link_m_reg, m_n->link_m);
> +
> +	if (DISPLAY_VER(display) >= 14)
> +		link_n &= ~PIPE_LINK_N1_EXTENDED_MASK;
> +	else
> +		link_n &= DATA_LINK_M_N_MASK;

[Severity: Medium]
Since ~PIPE_LINK_N1_EXTENDED_MASK and DATA_LINK_M_N_MASK both apply a
0x00FFFFFF mask, does this redundant if/else block in intel_set_m_n()
need to be removed to match the commit message?

> +
>  	/*
>  	 * On BDW+ writing LINK_N arms the double buffered update
>  	 * of all the M/N registers, so it must be written last.
>  	 */
> -	intel_de_write(display, link_n_reg, m_n->link_n);
> +	intel_de_write(display, link_n_reg, link_n);
>  }
>  
[ ... ]
> @@ -3475,7 +3483,13 @@ void intel_get_m_n(struct intel_display *display,
>  		   intel_reg_t link_m_reg, intel_reg_t link_n_reg)
>  {
>  	m_n->link_m = intel_de_read(display, link_m_reg) & DATA_LINK_M_N_MASK;
> -	m_n->link_n = intel_de_read(display, link_n_reg) & DATA_LINK_M_N_MASK;
> +	m_n->link_n = intel_de_read(display, link_n_reg);
> +
> +	if (DISPLAY_VER(display) >= 14)
> +		m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK;
> +	else
> +		m_n->link_n &= DATA_LINK_M_N_MASK;

[Severity: Medium]
Similarly, does this split in intel_get_m_n() also need to be removed?

> +
>  	m_n->data_m = intel_de_read(display, data_m_reg) & DATA_LINK_M_N_MASK;
>  	m_n->data_n = intel_de_read(display, data_n_reg) & DATA_LINK_M_N_MASK;
>  	m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916025710.432281-1-suraj.kandpal@intel.com?part=5