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 <[email protected]> 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. > + > + intel_de_write(display, CMN_SDP_TL(display, cpu_transcoder), 0); > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
