Re: [PATCH v2 17/56] drm/i915/hdmi: Introduce intel_hdmi_frl_level() for per-lane TxFFE

[sashiko-bot@kernel.org]

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 <suraj.kandpal@intel.com>

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