Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/i915/hdmi: FRL training infinite retry loop on rate change at 9 Gbps - [High] drm/i915/hdmi: Sleeping function read_poll_timeout called in atomic encoder enable callback --
commit 3ebcdfcc6e404185a2b52af7506a3997a1901b79 Author: Ankit Nautiyal <[email protected]> drm/i915/hdmi: Reduce FRL rate cap on rate change during training If the sink requests a rate change during FRL Link training, reduce frl.rate_cap to the next lower FRL rate. The next mode compute then uses this lower rate cap, so we do not attempt the failing rate again. > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index fa4e5f0f567b0..11b4554e87bd6 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c [ ... ] > @@ -3779,6 +3791,7 @@ int intel_hdmi_start_frl(struct intel_encoder *encoder, > int req_rate = crtc_state->frl.required_lanes * > crtc_state->frl.required_rate; > int ffe_level = get_ffe_level(req_rate); > enum frl_lt_status status; > + int next_rate = -EINVAL; > > if (!crtc_state->frl.enable) > return 0; [Severity: High] Does this code call sleeping functions from an atomic context? The DRM subsystem guide explicitly defines encoder atomic enable/disable callbacks as executing in an atomic context where sleeping is forbidden. This patch modifies intel_hdmi_start_frl() which is wired into the atomic encoder enable callback path later in this patch series. intel_hdmi_start_frl() relies on frl_train_complete_ltsp() and intel_hdmi_frl_train_lts3() which both use the read_poll_timeout() macro with a sleep_us parameter of 1000: frl_train_complete_ltsp(): ret = read_poll_timeout(get_frl_update_flags, update_flags, update_flags < 0 || (update_flags & (SCDC_FRL_START | SCDC_FLT_UPDATE)), 1000, FLT_UPDATE_TIMEOUT_MS * USEC_PER_MSEC, false, encoder); intel_hdmi_frl_train_lts3(): read_poll_timeout(intel_hdmi_train_lanes, status, done, 1000, FLT_TIMEOUT_MS * USEC_PER_MSEC, false, encoder, crtc_state, ffe_level); Providing a non-zero sleep parameter causes the macro to use usleep_range() internally, which sleeps. Executing usleep_range() in this atomic context violates subsystem guidelines and causes kernel warnings and potential deadlocks. [ ... ] > @@ -3812,10 +3825,22 @@ int intel_hdmi_start_frl(struct intel_encoder > *encoder, > connector->base.id, connector->name, > intel_hdmi->frl.rate_gbps); > return 0; > - default: > + case FRL_TRAIN_STOP: > + next_rate = 0; > + break; > + case FRL_CHANGE_RATE: > + next_rate = get_next_frl_rate(req_rate); > break; > + case FRL_TRAIN_RETRAIN: > + fallthrough; > + default: > + goto ltsl_tmds_mode; > } > > + if (next_rate >= 0) > + intel_hdmi->frl.rate_cap = next_rate; [Severity: High] Can this result in an infinite retry loop when the rate change happens at 9 Gbps? If the current rate is already the minimum valid FRL rate (9 Gbps), get_next_frl_rate(9) returns -EINVAL. In intel_hdmi_start_frl(), next_rate becomes -EINVAL (< 0), causing the code to skip updating intel_hdmi->frl.rate_cap, leaving it at 9 Gbps. Because the rate cap was not lowered to 0 (which is required to disable FRL and force a TMDS fallback, as correctly handled for FRL_TRAIN_STOP), the next mode compute will see the exact same 9 Gbps rate cap and attempt FRL training at 9 Gbps again, leading to an infinite retry loop. > + > +ltsl_tmds_mode: > if (crtc_state->frl.enable && !intel_hdmi->frl.trained) > drm_err(display->drm, > "[CONNECTOR:%d:%s] FRL Training Failed with rate=%d\n", -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
