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

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

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


Thanks for pointing this out. Makes sense to gate on 
HAS_COMMON_SDP_TL(display) instead of checking 
crtc_state->dip.cmn_sdp_tl in both places, since 0 is a valid 
transmission line value.

Will fix in next version.


Thanks & Regards,

Ankit

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