Re: [PATCH v2 14/56] drm/i915/hdmi: Add functions for FRL training state machine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm: missing read-modify-write in intel_hdmi_train_lanes overwrites hardware state
- [High] drm: usleep_range() called in atomic context via read_poll_timeout during FRL training
- [High] drm: lockless traversal of global encoder_list during FRL polling
- [High] drm/i915/hdmi: FRL link training pattern overwritten with 0 (NO_LTP) on FFE change
- [Low] drm: un-undef'd macro 'done' pollutes file scope in intel_hdmi.c
--

commit 786a188382d5678ef07d3ee398364ffb41065e98
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

drm/i915/hdmi: Add functions for FRL training state machine

This commit adds the FRL link training state machine and helpers to drive it
with an HDMI FRL sink over SCDC. It prepares for LTS:2, trains lanes in
LTS:3, completes training in LTS:P, and falls back to legacy TMDS in LTS:L.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 692480311c745..47d10ae0b07fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
[ ... ]
> @@ -3523,3 +3523,304 @@ void intel_hdmi_prepare_for_frl_mode(const struct intel_crtc_state *crtc_state)
>  
>  	intel_de_write(display, TRANS_HDMI_FRL_TRAIN(display, trans), write_buf);
>  }
> +
> +static int is_flt_ready(struct intel_encoder *encoder)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct i2c_adapter *adapter =
> +		intel_gmbus_get_adapter(display, intel_hdmi_ddc_pin(encoder));

[Severity: High]
Does this code perform an unsafe lockless traversal of the global encoder
list?

The intel_hdmi_ddc_pin() function calls get_encoder_by_ddc_pin(), which
uses for_each_intel_encoder() to iterate over the global encoder list. This
is called repeatedly from the read_poll_timeout() loop in
intel_hdmi_frl_prepare_lts2().

Since this runs during the asynchronous atomic commit tail where the modeset
locks (like connection_mutex) are not held, could this list traversal race
against concurrent MST hotplugs adding or removing encoders? Resolving the
DDC pin adapter once before the polling loop might avoid both the locking
issue and the overhead of an O(N) traversal inside a high-frequency polling
loop.

[ ... ]
> +static bool
> +intel_hdmi_frl_prepare_lts2(struct intel_encoder *encoder,
> +			    int frl_rate, int frl_lanes,
> +			    int ffe_level)
> +{
> +#define TIMEOUT_FLT_READY_MS 250
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct i2c_adapter *adapter =
> +		intel_gmbus_get_adapter(display, intel_hdmi_ddc_pin(encoder));
> +	int flt_ready;
> +	int ret;
> +
> +	if (!frl_rate || !frl_lanes)
> +		return false;
> +
> +	ret = read_poll_timeout(is_flt_ready, flt_ready,
> +				flt_ready, 1000,
> +				TIMEOUT_FLT_READY_MS * USEC_PER_MSEC,
> +				false, encoder);

[Severity: High]
Could this polling loop cause system instability by sleeping in atomic
context?

The read_poll_timeout() macro expands to a loop invoking usleep_range()
because a non-zero sleep duration (1000) is passed. Because
intel_hdmi_start_frl() is called within the encoder atomic enable or
pre_enable path, sleeping is strictly forbidden. Should this use a
non-sleeping delay such as read_poll_timeout_atomic() with udelay instead?

[ ... ]
> +static enum frl_lt_status
> +intel_hdmi_train_lanes(struct intel_encoder *encoder,
> +		       const struct intel_crtc_state *crtc_state,
> +		       int ffe_level)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	enum transcoder trans = crtc_state->cpu_transcoder;
> +	int num_lanes = crtc_state->frl.required_lanes;
> +	enum drm_scdc_frl_ltp ltp[4];
> +	u32 write_buf = 0;
> +	int update_flags;
> +	int lane;
[ ... ]
> +	for (lane = 0; lane < num_lanes; lane++) {
> +		if (ltp[lane] == SCDC_FRL_LTP3)
> +			continue;
> +
> +		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;
> +	}
> +
> +	intel_de_write(display, TRANS_HDMI_FRL_TRAIN(display, trans), write_buf);

[Severity: High]
Will this direct overwrite unintentionally clear the link training pattern for
lanes undergoing an FFE change?

Because write_buf is initialized to 0, if any lane requires an FFE change
(SCDC_FRL_CHNG_FFE) or LTP3, the loop continues without setting bits for
that lane. Using intel_de_write() will then directly overwrite the entire
multi-field TRANS_HDMI_FRL_TRAIN register, forcing 0 (SCDC_FRL_NO_LTP) into
those skipped lanes.

The HDMI specification requires the source to preserve the current link
training pattern while adjusting FFE levels. Does this need a
read-modify-write sequence to ensure the hardware state of untouched lanes
is properly preserved?

[ ... ]
> +static enum frl_lt_status
> +intel_hdmi_frl_train_lts3(struct intel_encoder *encoder,
> +			  const struct intel_crtc_state *crtc_state,
> +			  int ffe_level)
> +{
> +#define FLT_TIMEOUT_MS 200
> +	struct intel_display *display = to_intel_display(encoder);
> +	enum frl_lt_status status;
> +	enum transcoder trans = crtc_state->cpu_transcoder;
> +	u32 buf;
> +
> +	buf = intel_de_read(display, TRANS_HDMI_FRL_CFG(display, trans));
> +	intel_de_write(display, TRANS_HDMI_FRL_CFG(display, trans),
> +		       buf | TRANS_HDMI_FRL_ENABLE);
> +
> +#define done (status != FRL_TRAIN_CONTINUE)
> +	status = FRL_TRAIN_CONTINUE;
> +	read_poll_timeout(intel_hdmi_train_lanes, status,
> +			  done,
> +			  1000, FLT_TIMEOUT_MS * USEC_PER_MSEC,
> +			  false, encoder, crtc_state, ffe_level);

[Severity: Low]
This isn't a severe bug, but does the done macro need to be undefined?

Because C macros do not have block scope, defining done here without a
corresponding #undef pollutes the file scope of intel_hdmi.c. This might
cause confusing compilation errors if subsequent code uses the common word
done as a variable or struct member.

[Severity: High]
As noted above with intel_hdmi_frl_prepare_lts2(), this read_poll_timeout()
call also passes a 1000 microsecond sleep duration. This will invoke
usleep_range() while inside an atomic encoder callback. Does this need to be
changed to a non-sleeping polling function to avoid scheduling while atomic?

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