Re: [PATCH v2 36/56] drm/i915/hdmi: Validate FRL modes in mode_valid using the FRL DFM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915: FRL mode valid misses 3D frame packing clock doubling
--

commit 357e437906e7e9f540a08210f7b85cf2f3e109aa
Author: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
drm/i915/hdmi: Validate FRL modes in mode_valid using the FRL DFM

This commit adds a fallback FRL capability check to
intel_hdmi_mode_valid_format() using the non-DSC FRL DFM parameters to
validate modes that exceed TMDS limits. It also introduces
intel_hdmi_can_support_frl() as a placeholder returning false until FRL
support is completed.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 67228b538e81a..b82c636b46f93 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
[ ... ]
> @@ -2021,6 +2022,51 @@ intel_hdmi_tmds_mode_clock_valid(struct drm_connector *_connector, int clock,
>  	return status;
>  }
>  
[ ... ]
> +static enum drm_mode_status
> +intel_hdmi_frl_mode_clock_valid(struct intel_connector *connector,
> +				const struct drm_display_mode *mode,
> +				enum intel_output_format output_format)
> +{
> +	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
> +	struct intel_hdmi_frl_dfm frl_dfm = {};
> +	int max_rate = hdmi->max_frl_rate;
> +	int lanes = max_rate < 24 ? 3 : 4;
> +
> +	frl_dfm.config.pixel_clock_nominal_khz = mode->clock;

[Severity: High]
Does this code miss the 3D frame packing clock doubling?

In intel_hdmi_mode_valid(), the clock variable is doubled for 3D Frame
Packing modes:

	if ((mode->flags & DRM_MODE_FLAG_3D_MASK) == DRM_MODE_FLAG_3D_FRAME_PACKING)
		clock *= 2;

This adjusted clock is passed to intel_hdmi_mode_valid_format() and used by
the TMDS validation path.

However, intel_hdmi_frl_mode_clock_valid() directly reads mode->clock, which
means it uses the unadjusted clock.

Could this cause the FRL Data Flow Management (DFM) capacity check to
underestimate the required bandwidth by a factor of 2 for 3D Frame Packing
modes, erroneously accepting modes that exceed the physical capacity of the
FRL link?

> +	frl_dfm.config.hactive = mode->hdisplay;
> +	frl_dfm.config.hblank = mode->htotal - mode->hdisplay;
> +
[ ... ]
> @@ -2052,6 +2105,7 @@ intel_hdmi_mode_valid_format(struct intel_connector *connector,
>  			     enum intel_output_format sink_format)
>  {
>  	struct intel_display *display = to_intel_display(connector);
> +	struct intel_hdmi *hdmi = intel_attached_hdmi(connector);
>  	enum drm_mode_status status;
>  
>  	status = intel_hdmi_sink_format_valid(connector, mode,
> @@ -2063,7 +2117,12 @@ intel_hdmi_mode_valid_format(struct intel_connector *connector,
>  	if (status != MODE_OK)
>  		return status;
>  
> -	return intel_hdmi_tmds_mode_clock_valid(&connector->base, clock, has_hdmi_sink, sink_format);
> +	status = intel_hdmi_tmds_mode_clock_valid(&connector->base, clock, has_hdmi_sink, sink_format);
> +
> +	if (status != MODE_OK && intel_hdmi_can_support_frl(hdmi))
> +		status = intel_hdmi_frl_mode_clock_valid(connector, mode, sink_format);

[Severity: High]
Since intel_hdmi_frl_mode_clock_valid() is called without the adjusted clock
parameter, it will use the unadjusted mode->clock.

Should we pass the adjusted clock variable to
intel_hdmi_frl_mode_clock_valid() similar to how it is passed to
intel_hdmi_tmds_mode_clock_valid()?

> +
> +	return status;
>  }
>  
>  static enum drm_mode_status

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