Re: [PATCH v5 10/12] drm/i915/dp: Introduce helpers to enable/disable CMN SDP Transmission line

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using cmn_sdp_tl == 0 as a condition to skip hardware programming incorrectly disables valid configurations where the base transmission line is exactly 0.
--

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

drm/i915/dp: Introduce helpers to enable/disable CMN SDP Transmission line

This commit introduces helper functions to enable and disable the Common SDP
Transmission Line (CMN_SDP_TL) and its stagger registers based on the
configuration stored in crtc_state.

> diff --git a/drivers/gpu/drm/i915/display/intel_dip.c b/drivers/gpu/drm/i915/display/intel_dip.c
> index d1acc7eb5a391..0c80d6305feca 100644
> --- a/drivers/gpu/drm/i915/display/intel_dip.c
> +++ b/drivers/gpu/drm/i915/display/intel_dip.c
[ ... ]
> @@ -61,3 +63,57 @@ void intel_dip_sdp_transmission_line_get_config(struct intel_crtc_state *crtc_st
> +void intel_dip_cmn_sdp_transmission_line_enable(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	int gmp_stagger;
> +	int pps_stagger;
> +	int vsc_ext_stagger;
> +
> +	if (!crtc_state->dip.cmn_sdp_tl)
> +		return;

[Severity: High]
Does this incorrectly skip hardware programming for a valid configuration?

Since the transmission line can be computed as crtc_state->vrr.guardband - 1,
if guardband evaluates to 1, cmn_sdp_tl becomes 0. Since 0 is a
mathematically valid 0-based register index, this early return will
incorrectly treat a valid base transmission line of 0 as disabled, skipping
the register programming entirely.

[ ... ]
> +void intel_dip_cmn_sdp_transmission_line_disable(const struct intel_crtc_state *old_crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(old_crtc_state);
> +	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> +
> +	if (!old_crtc_state->dip.cmn_sdp_tl)
> +		return;

[Severity: High]
Similarly, does this skip disabling the transmission line if the valid
configured base transmission line was exactly 0? 

If the base transmission line was initially programmed as 0, this early
return prevents the register from being cleared on disable.

> +
> +	intel_de_write(display, CMN_SDP_TL(display, cpu_transcoder), 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908062224.2897987-1-ankit.k.nautiyal@intel.com?part=10