On 9/8/2026 12:41 PM, [email protected] wrote:
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 <[email protected]>
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.
VRR is not set for MST yet so we are covered.
/* 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?
guardband can't be 0 or 1 here. DP always computes it via the optimized
guardband path (intel_vrr_compute_optimized_guardband()), which calls
intel_dp_sdp_min_guardband(crtc_state, true) with assume_all_enabled = true.
That guarantees a floor of at least 8 (GMP's contribution), regardless
of which SDPs are actually active, so no underflow is possible.
That said, this does need to change once CMN_SDP_TL is set: with
CMN_SDP_TL enabled, the per-packet minimum SDP guardband requirement no
longer applies.
I'll set a 5-line floor for the guardband in that case, 4 lines to cover
the packet types plus 1 extra.
Thanks & Regards,
Ankit
