Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/i915/hdmi: connector refcount underflow via modeset_retry_work - [Medium] drm/i915/hdmi: intel_hdmi_frl_cfg_write corrupts register by omitting clear mask - [Medium] drm/i915: unconditional FRL configuration and transcoder enablement contrary to commit message --
commit cfc5e8ca998b654bfa60f35c76733a4d8332204f Author: Ankit Nautiyal <[email protected]> Subject: drm/i915/ddi: Wire FRL into the HDMI DDI enable/disable path This commit enables HDMI FRL in the DDI enable and disable sequences for MTL+. It prepares for FRL mode, starts FRL link training, and configures the FRL registers if enabled. On disable, it handles LTS:L teardown and register cleanup. > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > b/drivers/gpu/drm/i915/display/intel_ddi.c > index 53e889c0d0223..22a49eb50b203 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c [ ... ] > @@ -3605,8 +3614,15 @@ static void intel_ddi_enable_hdmi(struct > intel_atomic_state *state, > > intel_ddi_buf_enable(encoder, buf_ctl); > > - if (DISPLAY_VER(display) >= 14) > + if (DISPLAY_VER(display) >= 14) { > + if (crtc_state->frl.enable && > + intel_hdmi_start_frl(encoder, crtc_state) < 0) > + schedule_work(&intel_connector->modeset_retry_work); [Severity: High] Does this code underflow the connector reference count? If intel_hdmi_start_frl() fails, schedule_work() is called directly without taking a reference via drm_connector_get(). If the driver suspends, unbinds, or calls intel_hpd_cancel_work(), it calls intel_connector_cancel_modeset_retry_work(). This generic cancel helper invokes cancel_work_sync() and unconditionally calls drm_connector_put() if the work was pending. Because the work was queued without taking a reference, this drops an unowned reference. > + > + intel_hdmi_frl_cfg_write(crtc_state); [Severity: Medium] Does intel_hdmi_frl_cfg_write() corrupt the register by omitting a clear mask? When this function is called here, it performs a read-modify-write on the TRANS_HDMI_FRL_CFG register: intel_hdmi.c:intel_hdmi_frl_cfg_write() { ... val = TRANS_HDMI_R_B_SCHED_ENABLE(crtc_state->frl.rsrc_sched_en); val |= TRANS_HDMI_ACTIVE_CHAR_BUF_THRESH(crtc_state->frl.active_char_buf_threshold); val |= TRANS_HDMI_MIN_BLANK_CHAR(TRANS_HDMI_MIN_BLANK_CHAR_VAL); intel_de_rmw(display, TRANS_HDMI_FRL_CFG(display, cpu_trans), 0, val); } Because the clear mask is 0, intel_de_rmw() performs a read, leaves all existing bits intact, and then bitwise ORs the new val. This could permanently corrupt the hardware configuration if the previous register value had bits set that should be zero in the new configuration. [Severity: Medium] Does this execute hardware configuration unconditionally despite failures? The commit message explicitly states: "If FRL link training fails, notify userspace. On success, write DFM registers and enable the transcoder." However, the code unconditionally calls intel_hdmi_frl_cfg_write(), intel_hdmi_frl_dfm_write(), and intel_ddi_enable_transcoder_and_vblank() even if intel_hdmi_start_frl() returns an error. While this might be architecturally consistent with how DisplayPort handles link training failures, is there a discrepancy between the documented intent and the actual implementation? > + intel_hdmi_frl_dfm_write(crtc_state); > intel_ddi_enable_transcoder_and_vblank(state, encoder, > crtc_state); > + } > > intel_hdmi_poll_for_scrambling_enable(crtc_state, connector); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=40
