Re: [PATCH v6 11/13] 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:
- [Medium] Common SDP transmission line is incorrectly programmed for MST streams due to missing computation and mismatched transcoder programming scopes.
- [Medium] Integer underflow when computing cmn_sdp_tl for display modes where VRR is disabled or unsupported, leading to spurious warnings and aborted hardware programming.
--

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

drm/i915/dip: Enable Common SDP Transmission line

This commit computes and programs the common base transmission line and
per-SDP stagger values during modesets. It also disables the feature upon
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);
> +
>  	/* 7.a 128b/132b SST. */

[Severity: Medium]
Does hooking this transcoder-specific transmission line programming here
correctly handle MST streams? 

Since mtl_ddi_pre_enable_dp() operates at the link level, it appears this
might only be called once for the first active MST stream. This could leave
subsequent transcoders in the MST topology uninitialized, causing their SDP
packets to use incorrect default transmission lines.

Additionally, should the transmission line computation be added to
mst_stream_compute_config_late() in intel_dp_mst.c? It looks like it is
currently omitted during the MST encoder's late compute configuration.

[ ... ]
> @@ -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);
>  	intel_ddi_disable_fec(encoder, crtc_state);

[Severity: Medium]
Similar to the pre-enable hook, does placing the disable sequence in this
link-level function cause it to only apply to the transcoder of the last
active MST stream?

> diff --git a/drivers/gpu/drm/i915/display/intel_dip.c b/drivers/gpu/drm/i915/display/intel_dip.c
> index f8e8577b27558..f447150ff7bc0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dip.c
> +++ b/drivers/gpu/drm/i915/display/intel_dip.c
[ ... ]
> @@ -54,14 +55,109 @@ void intel_dip_write_emp_as_sdp_tl(const struct intel_crtc_state *crtc_state)
[ ... ]
> +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: Medium]
Can this subtraction underflow if the display mode does not support VRR?

If an interlaced mode is set, for example, intel_vrr_compute_config() can
bail out early and leave vrr.guardband at 0. This would cause the
computation to evaluate to a large unsigned value (or a negative value that
is later evaluated as unsigned).

If this underflow happens, it appears it would cascade into the following
bounds check:

> +	if (drm_WARN_ON(display->drm,
> +			crtc_state->dip.cmn_sdp_tl >=
> +			crtc_state->vrr.guardband + crtc_state->set_context_latency))
> +		return;

This could trigger a spurious kernel warning and unexpectedly abort the
rest of the hardware programming sequence for the transmission line.

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