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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 9/8/2026 12:41 PM, sashiko-bot@kernel.org 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 <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.

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

>