Re: [PATCH v3 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 2 potential issue(s) to consider:
- [High] drm/i915: CHICKEN_TRANS modified during fastset violates HW sequence
- [Medium] drm/i915: TGDDA bypass readout masks invalid hardware state
--

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

To support a Link M/N ratio between 10.0 and 15.0, for some BMG
ultrajoiner cases we need Wa_14021768792. This commit programs the LINKM and
LINKN registers, along with relevant bits in HDMI_EMP_DATA and CHICKEN_TRANS,
to bypass the hardware limitation within the Timing Generator DDA (TGDDA).

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 6d2025e109956..2b226620de0fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
[ ... ]
> @@ -2738,6 +2749,26 @@ bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display,
>  	return IS_DISPLAY_VER(display, 5, 7) || display->platform.cherryview;
>  }
>  
> +static void bmg_enhanced_tgdda_write(struct intel_crtc *crtc,
> +				     enum transcoder transcoder,
> +				     const struct intel_link_m_n *m_n)
> +{
> +	struct intel_display *display = to_intel_display(crtc);
> +	bool enable = m_n->bypass_m_n_ratio_limit && m_n->link_n;
> +	u32 link_m_frac = enable ? m_n->link_m % m_n->link_n : 0;
> +
> +	if (!intel_display_wa(display, INTEL_DISPLAY_WA_14021768792))
> +		return;
> +
> +	intel_de_rmw(display, HDMI_EMP_DATA(display, transcoder),
> +		     HDMI_EMP_DATA_LINK_M_FRAC_MASK,
> +		     REG_FIELD_PREP(HDMI_EMP_DATA_LINK_M_FRAC_MASK, link_m_frac));
> +
> +	intel_de_rmw(display, CHICKEN_TRANS(display, transcoder),
> +		     DP_ENHANCED_TGDDA_ENABLE,
> +		     enable ? DP_ENHANCED_TGDDA_ENABLE : 0);

[Severity: High]
When reached via intel_update_crtc() during a fastset, can these
non-double-buffered registers be modified while the transcoder is already
enabled?

If userspace requests a modeset that lowers the pixel clock on an eDP panel
(which supports seamless M/N updates) such that the M/N ratio drops below the
workaround threshold, the new state will have bypass_m_n_ratio_limit = false.

Because the M/N struct check is skipped when update_m_n is true:

drivers/gpu/drm/i915/display/intel_display.c:intel_pipe_config_compare() {
    ...
		if (!fastset || !pipe_config->update_m_n)
			PIPE_CONF_CHECK_M_N(dp_m_n);
    ...
}

And because intel_crtc_check_fastset() leaves update_m_n true despite the
bypass_m_n_ratio_limit mismatch:

drivers/gpu/drm/i915/display/intel_display.c:intel_crtc_check_fastset() {
    ...
	if (intel_compare_link_m_n(&old_crtc_state->dp_m_n,
				   &new_crtc_state->dp_m_n))
		new_crtc_state->update_m_n = false;
    ...
}

This authorizes a fastset. During the fastset, bmg_enhanced_tgdda_write() will
then clear the workaround bits in the live transcoder. Modifying CHICKEN_TRANS
and HDMI_EMP_DATA while the transcoder is active violates the explicit hardware
programming sequence. This can lead to transcoder hangs, blank screens, or
display corruption, and is reachable by a local unprivileged user requesting
specific modes/refresh rates on supported eDP displays.

> +}
> +
[ ... ]
> @@ -3495,6 +3528,49 @@ void intel_get_m_n(struct intel_display *display,
>  	m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1;
>  }
>  
> +bool intel_display_can_bypass_m_n_limit(struct intel_display *display,
> +					int m_n_ratio,
> +					enum pipe pipe)
> +{
> +	if (!intel_display_wa(display, INTEL_DISPLAY_WA_14021768792))
> +		return false;
> +
> +	if (m_n_ratio > 15)
> +		return false;
> +
> +	if (pipe != PIPE_A)
> +		return false;
> +
> +	return true;
> +}
> +
> +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]
Does this early return apply software policy during hardware readout, masking
the fact that the hardware workaround is physically enabled on a disallowed
pipe?

If DP_ENHANCED_TGDDA_ENABLE is erroneously left enabled on an invalid pipe
(e.g., PIPE B) or with a corrupted fractional value, bmg_enhanced_tgdda_read()
returns early. The state checker compares this readout (false) against the
computed software state (false) and incorrectly declares a match, blinding the
state checker to the actual hardware configuration. If the hardware is
misprogrammed (e.g., by BIOS), it will not be detected and healed, potentially
causing display corruption or blank screens.

> +
> +	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]
Does requiring a perfect fractional match to report the enable bit mask cases
where the workaround is enabled but incorrectly configured?

If the fractional value is corrupted, bmg_enhanced_tgdda_read() refuses to set
bypass_m_n_ratio_limit = true, causing the same state checker mismatch issue
described above.

> +}
> +

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