Re: [PATCH v7 11/14] drm/i915/dip: Enable Common SDP Transmission line

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/dip: Missing CMN_SDP_TL programming for DP MST slave transcoders
--

commit de3a67cbeba4a6e70cc767d1dce1950454f58dfa
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
supported platforms. It computes and programs the base transmission line
and stagger values during modeset. It also 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
> index dacb4b7588a02..3cdb06e81130b 100644
> --- 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]
Will placing this transcoder-specific initialization in mtl_ddi_pre_enable_dp()
cause it to be skipped for subsequent MST streams? 

In DP MST configurations, mst_stream_pre_enable() invokes the primary encoder's
pre_enable hook (mtl_ddi_pre_enable_dp()) solely for the first active MST
stream.

Because intel_dip_cmn_sdp_transmission_line_enable() is called here, it
appears it will be skipped for all subsequent streams. This would leave the
CMN_SDP_TL registers uninitialized for slave transcoders, potentially causing
broken or mistimed SDP transmission on secondary MST monitors.

>  	/* 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]
Does placing this disable routine in intel_ddi_buf_disable() prevent it from
executing for slave transcoders during MST teardown? 

Similar to the enable path, intel_ddi_buf_disable() is only called for the
last active stream during teardown, which means the CMN_SDP_TL disable routine
would be skipped for slave streams.

>  	intel_ddi_disable_fec(encoder, crtc_state);
>  
>  	if (DISPLAY_VER(display) < 14)

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