Re: [PATCH v6 12/13] drm/i915/dp: Adjust SDP guardband requirement for CMN_SDP_TL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] intel_dp_get_lines_for_cmn_sdp_tl() fails to handle DP_SDP_VSC, returning 0 instead of the documented 3 setup lines.

Pre-existing issues:
- [High] intel_dp_sdp_min_guardband() fails to query guardband requirements for DP_SDP_VSC and DP_SDP_VSC_EXT_VESA/CEA.
--

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

    drm/i915/dp: Adjust SDP guardband requirement for CMN_SDP_TL

This commit adjusts the SDP guardband requirements when CMN_SDP_TL is enabled.
It introduces intel_dp_get_lines_for_cmn_sdp_tl() to anchor SDPs like GMP, PPS,
VSC, and VSC_EXT relative to CMN_SDP_TL instead of the old per-packet sizing.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8c3a324e164ec..02e73ae9e34a0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
[ ... ]
> @@ -7348,9 +7349,55 @@ int intel_dp_compute_config_late(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> +static
> +int intel_dp_get_lines_for_cmn_sdp_tl(u32 type)
> +{
> +	u32 stagger_val;
> +
> +	/*
> +	 * Since we are using default stagger values similar to the case
> +	 * where CMN SDP TL is not set, the different SDP transmission
> +	 * lines are:
> +	 * base : 2nd line of delayed vblank:
> +	 * GMP : 2 + GMP_STAGGER
> +	 * VSC_EXT: 2 + VSC_EXT_STAGGER
> +	 * VSC : 2
> +	 * PPS : 2 + PPS_STAGGER
> +	 *
> +	 * SDP Setup = 1 + MAX(GMP, VSC_EXT, VSC, PPS setup lines)
> +	 *
> +	 * For EMP_AS_SDP_TL guardband should be more than vrr.vsync_start.
> +	 */
> +
> +	switch (type) {
> +	case DP_SDP_VSC_EXT_VESA:
> +	case DP_SDP_VSC_EXT_CEA:
> +		stagger_val = VSC_EXT_STAGGER_DEFAULT;
> +		break;
> +	case HDMI_PACKET_TYPE_GAMUT_METADATA:
> +		stagger_val = GMP_STAGGER_DEFAULT;
> +		break;
> +	case DP_SDP_PPS:
> +		stagger_val = PPS_STAGGER_DEFAULT;
> +		break;
> +	default:
> +		return 0;
> +	}

[Severity: Medium]
Does this switch statement intentionally omit a case for DP_SDP_VSC?

The comment block above explicitly lists VSC as requiring 2 lines, but
because it is missing from the switch cases, this function falls through to
the default case and returns 0. Returning 0 instead of the documented 3
setup lines (1 + 2 + 0 stagger) for the DP_SDP_VSC guardband requirement
leads to an incorrectly calculated guardband when VSC SDPs are evaluated,
which could potentially cause missing SDPs or display corruption.

> +
> +	return 1 + 2 + stagger_val;
> +}
> +
>  static
>  int intel_dp_get_lines_for_sdp(const struct intel_crtc_state *crtc_state, u32 type)
>  {
> +	struct intel_display *display = to_intel_display(crtc_state);
> +
> +	if (type == DP_SDP_ADAPTIVE_SYNC)
> +		return crtc_state->vrr.vsync_start + 1;
> +
> +	if (HAS_COMMON_SDP_TL(display))
> +		return intel_dp_get_lines_for_cmn_sdp_tl(type);
> +
>  	switch (type) {
>  	case DP_SDP_VSC_EXT_VESA:
>  	case DP_SDP_VSC_EXT_CEA:
> @@ -7359,8 +7406,6 @@ int intel_dp_get_lines_for_sdp(const struct intel_crtc_state *crtc_state, u32 ty
>  		return 8;
>  	case DP_SDP_PPS:
>  		return 7;

[Severity: High]
This is a pre-existing issue, but I noticed that intel_dp_sdp_min_guardband()
never actually queries DP_SDP_VSC or DP_SDP_VSC_EXT_VESA/CEA.

While this patch explicitly adds logic to handle VSC_EXT and VSC sizes (both
in the new CMN mode and legacy paths), intel_dp_sdp_min_guardband() only
queries GMP, PPS, and AS SDPs when calculating the maximum guardband:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_sdp_min_guardband() {
	...
	if (assume_all_enabled ||
	    crtc_state->infoframes.enable &
	    intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GAMUT_METADATA))
		sdp_guardband = max(sdp_guardband,
				    intel_dp_get_lines_for_sdp(crtc_state,
							       HDMI_PACKET_TYPE_GAMUT_METADATA));

	if (assume_all_enabled ||
	    crtc_state->dsc.compression_enable)
		sdp_guardband = max(sdp_guardband,
				    intel_dp_get_lines_for_sdp(crtc_state, DP_SDP_PPS));
	...
}

Does this leave the VSC_EXT switch cases in intel_dp_get_lines_for_sdp() and
intel_dp_get_lines_for_cmn_sdp_tl() as dead code? Since the guardband
calculation completely ignores the setup line requirements for VSC and
VSC_EXT packets, could programming a guardband smaller than the required
SDP transmission time lead to hardware failures or corrupted VBLANK timing?

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