Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/i915/hdmi: out-of-spec TMDS fallback takes precedence over valid FRL modes - [High] drm/i915/hdmi: FRL bypassed for modes exceeding sink TMDS limits - [Medium] drm/i915/hdmi: FRL clock computation corrupts crtc_state->pipe_bpp on failure - [Low] drm/i915: unconditional FRL enablement logging on error path --
commit 53b28248132a15ea6972675cb5d3f48a3d13e3b3 Author: Ankit Nautiyal <[email protected]> drm/i915/hdmi: Compute output format for HDMI FRL mode If the source and sink support FRL, then compute output format that can support HDMI FRL mode. > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c > b/drivers/gpu/drm/i915/display/intel_hdmi.c > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c [ ... ] > @@ -2648,13 +2672,23 @@ int intel_hdmi_compute_config(struct intel_encoder > *encoder, > if (!intel_link_bw_compute_pipe_bpp(pipe_config)) > return -EINVAL; > > - /* > - * Try to respect downstream TMDS clock limits first, if > - * that fails assume the user might know something we don't. > - */ > - ret = intel_hdmi_compute_formats(encoder, pipe_config, conn_state, > true); > - if (ret) > - ret = intel_hdmi_compute_formats(encoder, pipe_config, > conn_state, false); > + if (HAS_HDMI_FRL(display) && > + intel_bios_hdmi_max_frl_rate(encoder->devdata) && > + intel_hdmi->has_sink_hdmi_21) > + enable_frl = true; > + > + ret = intel_hdmi_compute_formats(encoder, pipe_config, conn_state, > false); > + > + if (ret && enable_frl) { [Severity: High] Does this sequence bypass FRL completely for displays with low TMDS limits? When connecting an HDMI 2.1 sink that supports FRL but has a lower max TMDS clock than the mode requires, the call to intel_hdmi_compute_formats with enable_frl=false will first fail the strict limit check, but then internally fall back to ignoring downstream limits: intel_hdmi_compute_formats() { ... ret = _intel_hdmi_compute_formats(..., true, enable_frl); if (ret) ret = _intel_hdmi_compute_formats(..., false, enable_frl); ... } If the mode's clock is within the source's max TMDS capability, this fallback succeeds and returns 0, masking the downstream limit failure. Because ret evaluates to 0, this FRL configuration block is skipped, and the driver outputs an out-of-spec TMDS signal instead of attempting FRL. Additionally, in intel_hdmi_compute_frl_config, there is a strict check against max_rate: intel_hdmi_compute_frl_config() { ... for (i = 0; i < ARRAY_SIZE(rate); i++) { if (rate[i] > max_rate) continue; ... } This means the fallback mechanism to ignore downstream limits doesn't actually allow users to override FRL bandwidth limits either. Could we attempt FRL before falling back to out-of-spec TMDS formats? > + ret = intel_hdmi_compute_formats(encoder, pipe_config, > conn_state, true); [Severity: Medium] Can intel_hdmi_compute_formats corrupt crtc_state->pipe_bpp if the FRL computation fails? When FRL computation is attempted, it calls down to intel_hdmi_compute_frl_clock which iterates over bpc: intel_hdmi_compute_frl_clock() { ... for (bpc = max_bpc; bpc >= 8; bpc -= 2) { ... crtc_state->pipe_bpp = bpc * 3; ret = intel_hdmi_compute_frl_config(encoder, crtc_state); if (ret) continue; ... } If the FRL computation fails across all iterations, crtc_state->pipe_bpp remains permanently lowered (e.g., to 24). When _intel_hdmi_compute_formats subsequently falls back to trying the YCBCR420 format, it reads this corrupted pipe_bpp to determine its starting max_bpc. This artificially restricts the fallback to 8 BPC, skipping 10 and 12 BPC which require less bandwidth under YCBCR420 and might have otherwise succeeded. > + drm_dbg_kms(display->drm, > + "Enabling FRL mode with lanes = %d rate = %d\n", > + pipe_config->frl.required_lanes, > + pipe_config->frl.required_rate); > + pipe_config->frl.enable = true; > + lane_count = pipe_config->frl.required_lanes; > + } [Severity: Low] Is it intentional that this block executes unconditionally, even if intel_hdmi_compute_formats fails? If the intel_hdmi_compute_formats call above returns a non-zero error code, the code still executes this drm_dbg_kms claiming FRL is being enabled with uninitialized or stale lane/rate values, and mutates pipe_config->frl.enable. While the atomic check will subsequently reject the mode and discard the state, the debug trace will misleadingly indicate FRL mode was successfully computed before the mode rejection. > + > if (ret) { > drm_dbg_kms(display->drm, > "unsupported HDMI clock (%d kHz), rejecting mode\n", > @@ -2677,7 +2711,7 @@ int intel_hdmi_compute_config(struct intel_encoder > *encoder, > adjusted_mode->picture_aspect_ratio = > conn_state->picture_aspect_ratio; > > - pipe_config->lane_count = 4; > + pipe_config->lane_count = lane_count; > > if (scdc->scrambling.supported && source_supports_scrambling(encoder) && > !pipe_config->frl.enable) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=47
