Re: [PATCH v2 47/56] drm/i915/hdmi: Compute output format for HDMI FRL mode

[sashiko-bot@kernel.org]

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 <ankit.k.nautiyal@intel.com>

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/20261012035806.3361439-1-ankit.k.nautiyal@intel.com?part=47