Re: [PATCH 3/4] drm/i915/dp: Start eDP link training from VBT fast link train vswing/pre-emphasis levels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/i915/dp: intel_dp_link_training_clock_recovery() assumes
  initial level 0
--

commit 6c36eea023572273fb066c2544c30e572e659dc6
Author: Lee Shawn C <shawn.c.lee@intel.com>

drm/i915/dp: Start eDP link training from VBT fast link train vswing/pre-emphasis levels

This commit initializes intel_dp->train_set from the VBT fast link
training voltage swing and pre-emphasis levels at the start of eDP clock
recovery when link recovery is idle.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index ac1aff6edf052..3de74eb1d3d4a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
[ ... ]
> @@ -1032,6 +1104,10 @@ intel_dp_reset_link_train(struct intel_dp *intel_dp,
>  			  u8 dp_train_pat)
>  {
>  	memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> +
> +	if (intel_dp_use_vbt_fast_link_levels(intel_dp, crtc_state, dp_phy))
> +		intel_dp_init_train_set_from_vbt(intel_dp, crtc_state, dp_phy);
> +
>  	intel_dp_set_signal_levels(intel_dp, crtc_state, dp_phy);
>  	return intel_dp_set_link_train(intel_dp, crtc_state, dp_phy, dp_train_pat);
>  }

[Severity: Medium]
Does starting intel_dp_reset_link_train() at non-zero VBT levels throw off
the retry and max-vswing tracking in
intel_dp_link_training_clock_recovery()?

drivers/gpu/drm/i915/display/intel_dp_link_training.c:intel_dp_link_training_clock_recovery() {
	u8 old_link_status[DP_LINK_STATUS_SIZE] = {};
	int voltage_tries, cr_tries, max_cr_tries;
	u8 link_status[DP_LINK_STATUS_SIZE];
	bool max_vswing_reached = false;
    ...
	if (!intel_dp_reset_link_train(intel_dp, crtc_state, dp_phy,
				       DP_TRAINING_PATTERN_1 |
				       DP_LINK_SCRAMBLING_DISABLE)) {
    ...
	voltage_tries = 1;
	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
    ...
		if (max_vswing_reached) {
			intel_dp_dump_link_status(intel_dp, dp_phy, link_status);
			lt_dbg(intel_dp, dp_phy, "Max Voltage Swing reached\n");
			return false;
		}
    ...
		if (!intel_dp_adjust_request_changed(crtc_state, old_link_status, link_status))
			++voltage_tries;
		else
			voltage_tries = 1;

		memcpy(old_link_status, link_status, sizeof(link_status));

		if (intel_dp_link_max_vswing_reached(intel_dp, crtc_state))
			max_vswing_reached = true;
	}
}

old_link_status starts zero-initialized, representing voltage swing 0 and
pre-emphasis 0.  When intel_dp_reset_link_train() programs non-zero VBT
levels and the first poll does not lock, intel_dp_adjust_request_changed()
compares the sink's first adjust request against (0, 0) instead of the VBT
levels: if the sink requests the same VBT levels, voltage_tries is reset
to 1 instead of incrementing to 2 (trying those levels 6 times instead of
5), whereas if the sink requests (0, 0), voltage_tries increments to 2 on
the first attempt at (0, 0) (aborting after 4 tries instead of 5).

Also, max_vswing_reached is initialized to false before
intel_dp_reset_link_train(), so if the initial VBT levels already hit the
maximum voltage swing, the first loop iteration does not notice that max
vswing was already reached.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110115432.894616-1-shawn.c.lee@intel.com?part=3