Re: [PATCH v2 18/56] drm/i915/hdmi: Drive per-lane TxFFE during FRL link training

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/i915/hdmi: FRL link training pattern dropped and hardware update inverted on FFE change
- [Medium] drm/i915/hdmi: FFE_Levels unconditionally advertised as 3 for all FRL rates
- [Medium] drm/i915/hdmi: Premature PHY TxFFE0 programming in LTS:2 before FRL is enabled
--

commit 87ef0f5ae2b4f4dd3dfe980d42af7aa5011177a4
Author: Suraj Kandpal <suraj.kandpal@intel.com>

drm/i915/hdmi: Drive per-lane TxFFE during FRL link training

This commit hooks the per-lane TxFFE state into the HDMI FRL Link
Training Sequence. It configures the SCDC FFE_Levels, sets initial
TxFFE0 levels upon FRL enable, and handles sink-requested FFE
changes during LTS:3 lane training.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 0873ecd6ef714..1aa6f71fbfce2 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
[ ... ]
> @@ -3619,7 +3620,8 @@ intel_hdmi_frl_prepare_lts2(struct intel_encoder *encoder,
>  	if (ret & SCDC_FLT_UPDATE)
>  		clear_scdc_update_flags(encoder, SCDC_FLT_UPDATE);
>  
> -	/* #TODO: Source shall program TxFFE = 0 for all active lanes */
> +	/* ffe_level[] was cleared by intel_hdmi_reset_frl_config(). */
> +	encoder->set_signal_levels(encoder, crtc_state);

[Severity: Medium]
The commit message explicitly states: "On LTS:3 entry, after enabling
TRANS_HDMI_FRL_ENABLE, call encoder->set_signal_levels() so the PHY is
programmed to TxFFE0".

However, does this code call encoder->set_signal_levels() prematurely? It
appears to execute inside intel_hdmi_frl_prepare_lts2(), which runs during
the LTS:2 phase before FRL mode is actually enabled via
TRANS_HDMI_FRL_ENABLE in intel_hdmi_frl_train_lts3().

If the underlying PHY requires the transcoder to be in FRL mode to accept
or apply FRL signal levels properly, could this premature sequence cause
hardware misconfiguration?

[ ... ]
> @@ -3684,16 +3688,20 @@ intel_hdmi_train_lanes(struct intel_encoder *encoder,
>  		if (ltp[lane] == SCDC_FRL_LTP3)
>  			continue;
>  
> -		if (ltp[lane] >= SCDC_FRL_LTP1 && ltp[lane] <= SCDC_FRL_LTP8)
> +		if (ltp[lane] >= SCDC_FRL_LTP1 && ltp[lane] <= SCDC_FRL_LTP8) {
>  			write_buf |= TRANS_HDMI_FRL_LTP(ltp[lane], lane);
> -
> -		/* #TODO handle FFE change */
> -		else if (ltp[lane] == SCDC_FRL_CHNG_FFE)
> -			continue;
> +		} else if (ltp[lane] == SCDC_FRL_CHNG_FFE &&
> +			   intel_hdmi->frl.ffe_level[lane] < intel_hdmi->frl.max_ffe_level) {
> +			intel_hdmi->frl.ffe_level[lane]++;
> +			ffe_changed = true;
> +		}
>  	}
>  
>  	intel_de_write(display, TRANS_HDMI_FRL_TRAIN(display, trans), write_buf);
>  
> +	if (ffe_changed)
> +		encoder->set_signal_levels(encoder, crtc_state);

[Severity: High]
Does this implementation inadvertently clear the link training pattern when
the sink requests an FFE change?

When ltp[lane] == SCDC_FRL_CHNG_FFE is evaluated, the FFE level is
incremented, but no pattern bits are preserved in write_buf for that lane.
Since write_buf is initially 0, intel_de_write() will overwrite the
TRANS_HDMI_FRL_TRAIN register with 0 (NO_LTP) for that lane, which could
break the link training sequence.

Additionally, the HDMI 2.1 specification dictates that when a sink requests
an FFE level change (0xE), the source must first update the FFE level on
the lane and then re-transmit the same Link Training Pattern. Here,
intel_de_write() is executed before the PHY FFE level update in
encoder->set_signal_levels(). Does this ordering violate the hardware
sequencing requirement?

[ ... ]
> @@ -3791,9 +3799,9 @@ static int get_next_frl_rate(int curr_rate_gbps)
>  	return -EINVAL;
>  }
>  
> -static int get_ffe_level(int rate_gbps)
> +static int get_max_ffe_level(int rate_gbps)
>  {
> -	return 0;
> +	return 3;
>  }

[Severity: Medium]
Does unconditionally returning 3 here violate the HDMI 2.1 specification
for lower FRL rates?

The specification (Section 10.3.3.4.1) mandates that for FRL rates of 3
Gbps and 6 Gbps (total link rates of 9, 18, 24 Gbps), the source shall set
the FFE_Levels field to 0.

Advertising 3 at these lower rates is a spec violation and might lead to
unexpected sink behavior, false FFE adjustment requests, or compliance test
failures. Should this function evaluate rate_gbps before returning the max
level?

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