Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/i915/ddi: flawed TMDS/FRL identification via port_clock in intel_ddi_level - [Medium] drm/i915: intel_ddi_level misidentifies 300/600MHz TMDS modes as FRL --
commit 0e9b38d21314c92fdb8c138db211c81ea24f573c Author: Suraj Kandpal <[email protected]> drm/i915/hdmi: Introduce intel_hdmi_frl_level() for per-lane TxFFE This commit introduces a per-lane TxFFE level tracking for HDMI FRL and exposes intel_hdmi_frl_level() to retrieve the buffer translation index. It modifies intel_ddi_level() to use this new function for FRL modes while preserving the existing behavior for TMDS. > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > b/drivers/gpu/drm/i915/display/intel_ddi.c > index 1e08ffe19447f..53e889c0d0223 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c [ ... ] > @@ -1518,11 +1518,15 @@ int intel_ddi_level(struct intel_encoder *encoder, > if (drm_WARN_ON_ONCE(display->drm, !trans)) > return 0; > > - if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) > - level = intel_ddi_hdmi_level(encoder, trans); > - else > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > + if (intel_hdmi_is_frl(crtc_state->port_clock)) > + level = intel_hdmi_frl_level(encoder, lane); [Severity: Medium] Does this incorrectly misidentify 300MHz and 600MHz TMDS modes as FRL modes? Because intel_hdmi_is_frl() checks if the clock exactly matches one of the FRL rates (e.g., 300000 kHz or 600000 kHz), it will return true if a custom display mode has a TMDS character clock of exactly 300 MHz or 600 MHz. There is no check in hdmi_port_tmds_clock_valid() preventing TMDS clocks from exactly matching FRL rates. If that happens, this branch will call intel_hdmi_frl_level() (returning index 1) instead of the correct VBT-defined TMDS level via intel_ddi_hdmi_level(). On older PHYs like C10 where FRL is unsupported, returning index 1 applies a random DP preset with pre-emphasis instead of the correct default HDMI level. Could this check use the more reliable crtc_state->frl.enable flag instead? A check forward through the series to commit ede50d511c51 confirms this logic is not fixed. > + else > + level = intel_ddi_hdmi_level(encoder, trans); > + } else { > level = intel_ddi_dp_level(enc_to_intel_dp(encoder), crtc_state, > lane); > + } > > if (drm_WARN_ON_ONCE(display->drm, level >= n_entries)) > level = n_entries - 1; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
