Re: [PATCH v5 11/12] drm/i915/dip: Enable Common SDP Transmission line

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Integer underflow when computing cmn_sdp_tl with guardband=0 causes a kernel warning, and a valid transmission line of 0 is incorrectly treated as 'disabled'.
- [Medium] Per-transcoder SDP transmission line programming is incorrectly placed in port-level enable/disable paths, breaking DP MST secondary streams.
--

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

drm/i915/dip: Enable Common SDP Transmission line

This commit enables programming of the common SDP transmission line on
platforms that support it. It computes and programs the common base
transmission line and per-SDP stagger values from the crtc state during
modeset, and disables the feature on pipe disable.

> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2737,6 +2737,8 @@ static void mtl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	/* 6.o Configure and enable FEC if needed */
>  	intel_ddi_enable_fec(encoder, crtc_state);
>  
> +	intel_dip_cmn_sdp_transmission_line_enable(crtc_state);
> +

[Severity: Medium]
Since mtl_ddi_pre_enable_dp() acts at the port level and only executes for
the first stream in a DP MST topology, will secondary streams' transcoders
miss having their SDP transmission line initialized?

It seems MST secondary streams will bypass this hook entirely. Furthermore,
mst_stream_compute_config_late() in intel_dp_mst.c doesn't appear to call
intel_dip_cmn_sdp_tl_compute_config_late(), meaning the values aren't
computed for MST streams.

>  	/* 7.a 128b/132b SST. */
>  	if (!is_mst && intel_dp_is_uhbr(crtc_state)) {
>  		/* VCPID 1, start slot 0 for 128b/132b, tu slots */
[ ... ]
> @@ -3124,6 +3126,7 @@ static void intel_ddi_buf_disable(struct intel_encoder *encoder,
>  			     DP_TP_CTL_ENABLE, 0);
>  	}
>  
> +	intel_dip_cmn_sdp_transmission_line_disable(crtc_state);

[Severity: Medium]
Similarly, since this port-level disable hook only executes when the last
stream is disabled, does this leave the transmission line registers active
for other transcoders in an MST setup when they are individually disabled?

>  	intel_ddi_disable_fec(encoder, crtc_state);
>  
>  	if (DISPLAY_VER(display) < 14)
> diff --git a/drivers/gpu/drm/i915/display/intel_dip.c b/drivers/gpu/drm/i915/display/intel_dip.c
> --- a/drivers/gpu/drm/i915/display/intel_dip.c
> +++ b/drivers/gpu/drm/i915/display/intel_dip.c
[ ... ]
> +static
> +void intel_dip_cmn_sdp_tl_compute_config_late(struct intel_crtc_state *crtc_state)
> +{
[ ... ]
> +	if (as_sdp)
> +		crtc_state->dip.cmn_sdp_tl = crtc_state->dip.emp_as_sdp_tl;
> +	else
> +		crtc_state->dip.cmn_sdp_tl = crtc_state->vrr.guardband - 1;

[Severity: High]
If crtc_state->vrr.guardband is 0 (for example, when VRR is not used), will
this subtraction underflow and set the u16 cmn_sdp_tl to 65535?

If it underflows to 65535, pps_stagger evaluates to a negative value 
(0 - 65535) in intel_dip_cmn_sdp_transmission_line_enable(). Will this trigger
the drm_WARN_ON(display->drm, pps_stagger < 0) and abort enabling the
transmission line?

Also, if guardband evaluates to 1, cmn_sdp_tl will be 0. Does the
!crtc_state->dip.cmn_sdp_tl check in intel_dip_cmn_sdp_transmission_line_enable()
and disable() incorrectly treat a valid transmission line value of 0 as a
sentinel for 'feature disabled', silently skipping the hardware enablement?

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