Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/i915/hdmi: FRL link training pattern dropped and hardware update inverted on FFE change - [Medium] drm/i915/hdmi: FFE_Levels unconditionally advertised as 3 for all FRL rates - [Medium] drm/i915/hdmi: Premature PHY TxFFE0 programming in LTS:2 before FRL is enabled --
commit 87ef0f5ae2b4f4dd3dfe980d42af7aa5011177a4 Author: Suraj Kandpal <[email protected]> drm/i915/hdmi: Drive per-lane TxFFE during FRL link training This commit hooks the per-lane TxFFE state into the HDMI FRL Link Training Sequence. It configures the SCDC FFE_Levels, sets initial TxFFE0 levels upon FRL enable, and handles sink-requested FFE changes during LTS:3 lane training. > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 0873ecd6ef714..1aa6f71fbfce2 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c [ ... ] > @@ -3619,7 +3620,8 @@ intel_hdmi_frl_prepare_lts2(struct intel_encoder > *encoder, > if (ret & SCDC_FLT_UPDATE) > clear_scdc_update_flags(encoder, SCDC_FLT_UPDATE); > > - /* #TODO: Source shall program TxFFE = 0 for all active lanes */ > + /* ffe_level[] was cleared by intel_hdmi_reset_frl_config(). */ > + encoder->set_signal_levels(encoder, crtc_state); [Severity: Medium] The commit message explicitly states: "On LTS:3 entry, after enabling TRANS_HDMI_FRL_ENABLE, call encoder->set_signal_levels() so the PHY is programmed to TxFFE0". However, does this code call encoder->set_signal_levels() prematurely? It appears to execute inside intel_hdmi_frl_prepare_lts2(), which runs during the LTS:2 phase before FRL mode is actually enabled via TRANS_HDMI_FRL_ENABLE in intel_hdmi_frl_train_lts3(). If the underlying PHY requires the transcoder to be in FRL mode to accept or apply FRL signal levels properly, could this premature sequence cause hardware misconfiguration? [ ... ] > @@ -3684,16 +3688,20 @@ intel_hdmi_train_lanes(struct intel_encoder *encoder, > if (ltp[lane] == SCDC_FRL_LTP3) > continue; > > - if (ltp[lane] >= SCDC_FRL_LTP1 && ltp[lane] <= SCDC_FRL_LTP8) > + 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; > + } else if (ltp[lane] == SCDC_FRL_CHNG_FFE && > + intel_hdmi->frl.ffe_level[lane] < > intel_hdmi->frl.max_ffe_level) { > + intel_hdmi->frl.ffe_level[lane]++; > + ffe_changed = true; > + } > } > > intel_de_write(display, TRANS_HDMI_FRL_TRAIN(display, trans), > write_buf); > > + if (ffe_changed) > + encoder->set_signal_levels(encoder, crtc_state); [Severity: High] Does this implementation inadvertently clear the link training pattern when the sink requests an FFE change? When ltp[lane] == SCDC_FRL_CHNG_FFE is evaluated, the FFE level is incremented, but no pattern bits are preserved in write_buf for that lane. Since write_buf is initially 0, intel_de_write() will overwrite the TRANS_HDMI_FRL_TRAIN register with 0 (NO_LTP) for that lane, which could break the link training sequence. Additionally, the HDMI 2.1 specification dictates that when a sink requests an FFE level change (0xE), the source must first update the FFE level on the lane and then re-transmit the same Link Training Pattern. Here, intel_de_write() is executed before the PHY FFE level update in encoder->set_signal_levels(). Does this ordering violate the hardware sequencing requirement? [ ... ] > @@ -3791,9 +3799,9 @@ static int get_next_frl_rate(int curr_rate_gbps) > return -EINVAL; > } > > -static int get_ffe_level(int rate_gbps) > +static int get_max_ffe_level(int rate_gbps) > { > - return 0; > + return 3; > } [Severity: Medium] Does unconditionally returning 3 here violate the HDMI 2.1 specification for lower FRL rates? The specification (Section 10.3.3.4.1) mandates that for FRL rates of 3 Gbps and 6 Gbps (total link rates of 9, 18, 24 Gbps), the source shall set the FFE_Levels field to 0. Advertising 3 at these lower rates is a spec violation and might lead to unexpected sink behavior, false FFE adjustment requests, or compliance test failures. Should this function evaluate rate_gbps before returning the max level? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
