Re: [PATCH 07/10] drm/i915/display: Add bits for Wa_14021768792 for linkm/n ratio > 10

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware state readout for TGDDA bypass is gated by software policy checks and strips register fields, hiding invalid hardware state from the DRM state checker.
--

commit 1ce39b1d4686aae2119d0886a91c099e220bd0a0
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Subject: drm/i915/display: Add bits for Wa_14021768792 for linkm/n ratio > 10

This commit adds support for Wa_14021768792 to bypass the hardware limitation
within the Timing Generator DDA (TGDDA) for Link M/N ratios between 10.0 and
15.0. It introduces hardware programming in the LINKM, LINKN, HDMI_EMP_DATA,
and CHICKEN_TRANS registers and tracks the usage with a new
bypass_m_n_ratio_limit member.

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 347892b0c1da7..a5b1d4c4623a3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
[ ... ]
> @@ -3487,16 +3520,64 @@ void intel_get_m_n(struct intel_display *display,
>  	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);
>  
> -	if (DISPLAY_VER(display) >= 14)
> +	if (DISPLAY_VER(display) >= 14) {
> +		u8 link_n_ext = REG_FIELD_GET(PIPE_LINK_N1_EXTENDED_MASK, m_n->link_n);
> +
>  		m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK;

[Severity: Medium]
This unconditionally strips the extended bits from the read hardware state in
intel_get_m_n(). The drm_WARN_ON directly below it only triggers if
link_n_ext is non-zero, which fails to warn if the bits are zero in hardware
but were expected to be set. Should this code retain the extended bits or
unconditionally validate them so the DRM state checker can detect missing
hardware configuration?

> +
> +		if (intel_display_wa(display, INTEL_DISPLAY_WA_14021768792))
> +			drm_WARN_ON(display->drm,
> +				    link_n_ext && link_n_ext != bmg_get_link_n_ext(m_n));
> +	} else {
>  		m_n->link_n &= DATA_LINK_M_N_MASK;
> +	}
[ ... ]
> +static void bmg_enhanced_tgdda_read(struct intel_crtc *crtc,
> +				    enum transcoder transcoder,
> +				    struct intel_link_m_n *m_n)
> +{
> +	struct intel_display *display = to_intel_display(crtc);
> +	u32 link_m_frac;
> +	int m_n_ratio;
> +
> +	if (!m_n->link_n)
> +		return;
> +
> +	m_n_ratio = DIV_ROUND_UP(m_n->link_m, m_n->link_n);
> +
> +	if (!intel_display_can_bypass_m_n_limit(display, m_n_ratio, crtc->pipe))
> +		return;

[Severity: Medium]
This policy check in bmg_enhanced_tgdda_read() aborts the readout of
DP_ENHANCED_TGDDA_ENABLE if the driver considers the configuration unsupported.
If the hardware is misconfigured, this early return leaves
bypass_m_n_ratio_limit as false. Should this check be removed so the true
hardware state is read and exposed to the DRM state checker?

> +
> +	if (!(intel_de_read(display, CHICKEN_TRANS(display, transcoder)) &
> +	      DP_ENHANCED_TGDDA_ENABLE))
> +		return;
> +
> +	link_m_frac = REG_FIELD_GET(HDMI_EMP_DATA_LINK_M_FRAC_MASK,
> +				    intel_de_read(display, HDMI_EMP_DATA(display, transcoder)));
> +
> +	if (link_m_frac == m_n->link_m % m_n->link_n)
> +		m_n->bypass_m_n_ratio_limit = true;

[Severity: Medium]
This exact-match check prevents the readout from recording that the bypass is
enabled if the fractional part in hardware is incorrect. Should
bypass_m_n_ratio_limit be set regardless of the fractional value so the
DRM state checker can catch incorrect hardware configurations?

> +}

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