Re: [PATCH v2 16/56] drm/i915/hdmi: Reduce FRL rate cap on rate change during training

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/hdmi: FRL training infinite retry loop on rate change at 9 Gbps
- [High] drm/i915/hdmi: Sleeping function read_poll_timeout called in atomic encoder enable callback
--

commit 3ebcdfcc6e404185a2b52af7506a3997a1901b79
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

drm/i915/hdmi: Reduce FRL rate cap on rate change during training

If the sink requests a rate change during FRL Link training, reduce
frl.rate_cap to the next lower FRL rate. The next mode compute then
uses this lower rate cap, so we do not attempt the failing rate again.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index fa4e5f0f567b0..11b4554e87bd6 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
[ ... ]
> @@ -3779,6 +3791,7 @@ int intel_hdmi_start_frl(struct intel_encoder *encoder,
>  	int req_rate = crtc_state->frl.required_lanes * crtc_state->frl.required_rate;
>  	int ffe_level = get_ffe_level(req_rate);
>  	enum frl_lt_status status;
> +	int next_rate = -EINVAL;
>  
>  	if (!crtc_state->frl.enable)
>  		return 0;

[Severity: High]
Does this code call sleeping functions from an atomic context?

The DRM subsystem guide explicitly defines encoder atomic enable/disable
callbacks as executing in an atomic context where sleeping is forbidden. This
patch modifies intel_hdmi_start_frl() which is wired into the atomic encoder
enable callback path later in this patch series.

intel_hdmi_start_frl() relies on frl_train_complete_ltsp() and
intel_hdmi_frl_train_lts3() which both use the read_poll_timeout() macro
with a sleep_us parameter of 1000:

frl_train_complete_ltsp():
	ret = read_poll_timeout(get_frl_update_flags, update_flags,
				update_flags < 0 ||
				(update_flags & (SCDC_FRL_START | SCDC_FLT_UPDATE)),
				1000, FLT_UPDATE_TIMEOUT_MS * USEC_PER_MSEC,
				false, encoder);

intel_hdmi_frl_train_lts3():
	read_poll_timeout(intel_hdmi_train_lanes, status,
			  done,
			  1000, FLT_TIMEOUT_MS * USEC_PER_MSEC,
			  false, encoder, crtc_state, ffe_level);

Providing a non-zero sleep parameter causes the macro to use usleep_range()
internally, which sleeps. Executing usleep_range() in this atomic context
violates subsystem guidelines and causes kernel warnings and potential
deadlocks.

[ ... ]
> @@ -3812,10 +3825,22 @@ int intel_hdmi_start_frl(struct intel_encoder *encoder,
>  			    connector->base.id, connector->name,
>  			    intel_hdmi->frl.rate_gbps);
>  		return 0;
> -	default:
> +	case FRL_TRAIN_STOP:
> +		next_rate = 0;
> +		break;
> +	case FRL_CHANGE_RATE:
> +		next_rate = get_next_frl_rate(req_rate);
>  		break;
> +	case FRL_TRAIN_RETRAIN:
> +		fallthrough;
> +	default:
> +		goto ltsl_tmds_mode;
>  	}
>  
> +	if (next_rate >= 0)
> +		intel_hdmi->frl.rate_cap = next_rate;

[Severity: High]
Can this result in an infinite retry loop when the rate change happens at
9 Gbps?

If the current rate is already the minimum valid FRL rate (9 Gbps),
get_next_frl_rate(9) returns -EINVAL. In intel_hdmi_start_frl(), next_rate
becomes -EINVAL (< 0), causing the code to skip updating
intel_hdmi->frl.rate_cap, leaving it at 9 Gbps.

Because the rate cap was not lowered to 0 (which is required to disable FRL
and force a TMDS fallback, as correctly handled for FRL_TRAIN_STOP), the next
mode compute will see the exact same 9 Gbps rate cap and attempt FRL
training at 9 Gbps again, leading to an infinite retry loop.

> +
> +ltsl_tmds_mode:
>  	if (crtc_state->frl.enable && !intel_hdmi->frl.trained)
>  		drm_err(display->drm,
>  			"[CONNECTOR:%d:%s] FRL Training Failed with rate=%d\n",

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