Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm: missing read-modify-write in intel_hdmi_train_lanes overwrites hardware state - [High] drm: usleep_range() called in atomic context via read_poll_timeout during FRL training - [High] drm: lockless traversal of global encoder_list during FRL polling - [High] drm/i915/hdmi: FRL link training pattern overwritten with 0 (NO_LTP) on FFE change - [Low] drm: un-undef'd macro 'done' pollutes file scope in intel_hdmi.c --
commit 786a188382d5678ef07d3ee398364ffb41065e98 Author: Ankit Nautiyal <[email protected]> drm/i915/hdmi: Add functions for FRL training state machine This commit adds the FRL link training state machine and helpers to drive it with an HDMI FRL sink over SCDC. It prepares for LTS:2, trains lanes in LTS:3, completes training in LTS:P, and falls back to legacy TMDS in LTS:L. > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 692480311c745..47d10ae0b07fd 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c [ ... ] > @@ -3523,3 +3523,304 @@ void intel_hdmi_prepare_for_frl_mode(const struct > intel_crtc_state *crtc_state) > > intel_de_write(display, TRANS_HDMI_FRL_TRAIN(display, trans), > write_buf); > } > + > +static int is_flt_ready(struct intel_encoder *encoder) > +{ > + struct intel_display *display = to_intel_display(encoder); > + struct i2c_adapter *adapter = > + intel_gmbus_get_adapter(display, intel_hdmi_ddc_pin(encoder)); [Severity: High] Does this code perform an unsafe lockless traversal of the global encoder list? The intel_hdmi_ddc_pin() function calls get_encoder_by_ddc_pin(), which uses for_each_intel_encoder() to iterate over the global encoder list. This is called repeatedly from the read_poll_timeout() loop in intel_hdmi_frl_prepare_lts2(). Since this runs during the asynchronous atomic commit tail where the modeset locks (like connection_mutex) are not held, could this list traversal race against concurrent MST hotplugs adding or removing encoders? Resolving the DDC pin adapter once before the polling loop might avoid both the locking issue and the overhead of an O(N) traversal inside a high-frequency polling loop. [ ... ] > +static bool > +intel_hdmi_frl_prepare_lts2(struct intel_encoder *encoder, > + int frl_rate, int frl_lanes, > + int ffe_level) > +{ > +#define TIMEOUT_FLT_READY_MS 250 > + struct intel_display *display = to_intel_display(encoder); > + struct i2c_adapter *adapter = > + intel_gmbus_get_adapter(display, intel_hdmi_ddc_pin(encoder)); > + int flt_ready; > + int ret; > + > + if (!frl_rate || !frl_lanes) > + return false; > + > + ret = read_poll_timeout(is_flt_ready, flt_ready, > + flt_ready, 1000, > + TIMEOUT_FLT_READY_MS * USEC_PER_MSEC, > + false, encoder); [Severity: High] Could this polling loop cause system instability by sleeping in atomic context? The read_poll_timeout() macro expands to a loop invoking usleep_range() because a non-zero sleep duration (1000) is passed. Because intel_hdmi_start_frl() is called within the encoder atomic enable or pre_enable path, sleeping is strictly forbidden. Should this use a non-sleeping delay such as read_poll_timeout_atomic() with udelay instead? [ ... ] > +static enum frl_lt_status > +intel_hdmi_train_lanes(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + int ffe_level) > +{ > + struct intel_display *display = to_intel_display(encoder); > + enum transcoder trans = crtc_state->cpu_transcoder; > + int num_lanes = crtc_state->frl.required_lanes; > + enum drm_scdc_frl_ltp ltp[4]; > + u32 write_buf = 0; > + int update_flags; > + int lane; [ ... ] > + for (lane = 0; lane < num_lanes; lane++) { > + if (ltp[lane] == SCDC_FRL_LTP3) > + continue; > + > + if (ltp[lane] >= SCDC_FRL_LTP1 && ltp[lane] <= SCDC_FRL_LTP8) > + write_buf |= TRANS_HDMI_FRL_LTP(ltp[lane], lane); > + > + /* #TODO handle FFE change */ > + else if (ltp[lane] == SCDC_FRL_CHNG_FFE) > + continue; > + } > + > + intel_de_write(display, TRANS_HDMI_FRL_TRAIN(display, trans), > write_buf); [Severity: High] Will this direct overwrite unintentionally clear the link training pattern for lanes undergoing an FFE change? Because write_buf is initialized to 0, if any lane requires an FFE change (SCDC_FRL_CHNG_FFE) or LTP3, the loop continues without setting bits for that lane. Using intel_de_write() will then directly overwrite the entire multi-field TRANS_HDMI_FRL_TRAIN register, forcing 0 (SCDC_FRL_NO_LTP) into those skipped lanes. The HDMI specification requires the source to preserve the current link training pattern while adjusting FFE levels. Does this need a read-modify-write sequence to ensure the hardware state of untouched lanes is properly preserved? [ ... ] > +static enum frl_lt_status > +intel_hdmi_frl_train_lts3(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + int ffe_level) > +{ > +#define FLT_TIMEOUT_MS 200 > + struct intel_display *display = to_intel_display(encoder); > + enum frl_lt_status status; > + enum transcoder trans = crtc_state->cpu_transcoder; > + u32 buf; > + > + buf = intel_de_read(display, TRANS_HDMI_FRL_CFG(display, trans)); > + intel_de_write(display, TRANS_HDMI_FRL_CFG(display, trans), > + buf | TRANS_HDMI_FRL_ENABLE); > + > +#define done (status != FRL_TRAIN_CONTINUE) > + status = FRL_TRAIN_CONTINUE; > + read_poll_timeout(intel_hdmi_train_lanes, status, > + done, > + 1000, FLT_TIMEOUT_MS * USEC_PER_MSEC, > + false, encoder, crtc_state, ffe_level); [Severity: Low] This isn't a severe bug, but does the done macro need to be undefined? Because C macros do not have block scope, defining done here without a corresponding #undef pollutes the file scope of intel_hdmi.c. This might cause confusing compilation errors if subsequent code uses the common word done as a variable or struct member. [Severity: High] As noted above with intel_hdmi_frl_prepare_lts2(), this read_poll_timeout() call also passes a 1000 microsecond sleep duration. This will invoke usleep_range() while inside an atomic encoder callback. Does this need to be changed to a non-sleeping polling function to avoid scheduling while atomic? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
