Re: [PATCH v2 38/56] drm/i915/hdmi: Add FRL clock computation based on DFM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/hdmi: pipe_bpp corruption in intel_hdmi_compute_frl_clock restricts fallback
--

commit e2a7d5ac5410d9fb50137634f445228551f6d758
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Subject: drm/i915/hdmi: Add FRL clock computation based on DFM

This commit computes the FRL port clock for a mode using the DFM helper.
It starts with the lowest FRL rate meeting the DFM requirements and checks
if FRL can support the mode.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 8e742165a50ad..0391597c9847f 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c

[ ... ]

> +static int
> +intel_hdmi_compute_frl_clock(struct intel_encoder *encoder,
> +			     struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_hdmi *hdmi = enc_to_intel_hdmi(encoder);
> +	int max_bpc = max(crtc_state->pipe_bpp / 3, 8);
> +	int port_clock;
> +	int bpc;
> +
> +	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
> +		int ret;
> +
> +		if (!hdmi_bpc_possible(crtc_state, bpc))
> +			continue;
> +
> +		crtc_state->pipe_bpp = bpc * 3;
> +
> +		ret = intel_hdmi_compute_frl_config(encoder, crtc_state);
> +		if (ret)
> +			continue;
> +
> +		/* Port clock for FRL rates is bitrate in 10Kbps */
> +		port_clock = FRL_GBPS_TO_10KBPS(crtc_state->frl.required_rate);
> +
> +		if (hdmi_port_frl_clock_valid(hdmi, port_clock) == MODE_OK) {
> +			crtc_state->port_clock = port_clock;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;

[Severity: Medium]
Does returning -EINVAL here leave crtc_state->pipe_bpp corrupted?

Because pipe_bpp is modified in-place within the loop, if all FRL
configurations fail, the function exits with pipe_bpp retaining the last
tested value (typically 24).

When this happens, the caller intel_hdmi_compute_formats() will reuse
the exact same crtc_state object for a YCbCr420 fallback attempt:

intel_hdmi_compute_formats() {
	ret = intel_hdmi_compute_output_format(..., INTEL_OUTPUT_FORMAT_RGB, ...);
	
	if (ret && drm_mode_is_420_also(...))
		ret = intel_hdmi_compute_output_format(..., INTEL_OUTPUT_FORMAT_YCBCR420, ...);
}

Since max_bpc is derived from crtc_state->pipe_bpp, the fallback is then
artificially restricted to only testing 8 bpc, missing valid 10/12 bpc
capabilities.

Should pipe_bpp be saved before the loop and restored to its original value 
before returning -EINVAL?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012035806.3361439-1-ankit.k.nautiyal@intel.com?part=38