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. > /* 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? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
