Re: [PATCH v6 02/11] drm/i915/dp: Limit M/N ratio for DP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/dp: misordered M/N ratio filter in intel_dp_compute_config_limits()
--

commit 46d07c1bdb22b62954414e63e1d33e15eab315fb
Author: Suraj Kandpal <suraj.kandpal@intel.com>

drm/i915/dp: Limit M/N ratio for DP

This commit restricts DisplayPort link configurations to those whose ceiling
Link M/Link N ratio fits within the transcoder timing generator limit during
link config limit computation, and prunes modes in mode_valid when no link
configuration can satisfy the ratio.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index d8c0d9381471d..7ceb7c02b597b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c

[ ... ]

> @@ -2656,6 +2725,48 @@ intel_dp_dsc_compute_pipe_bpp_limits(struct intel_connector *connector,
>  	return true;
>  }
>  
> +static bool
> +intel_dp_compute_m_n_ratio_limits(struct intel_dp *intel_dp,
> +				  struct drm_connector_state *conn_state,
> +				  const struct intel_crtc_state *crtc_state,
> +				  struct link_config_limits *limits)
> +{
> +	struct intel_display *display = to_intel_display(intel_dp);
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	struct intel_dp_link_caps *link_caps = intel_dp->link.caps;
> +	struct intel_dp_link_caps_order order =
> +		intel_dp_link_caps_connector_compute_order(connector);
> +	struct intel_dp_link_caps_filter new_filter = INTEL_DP_LINK_CAPS_FILTER_NONE;
> +	int pixel_clock = intel_dp_mso_pixel_clock(intel_dp,
> +						  intel_dp_mode_clock(crtc_state, conn_state));
> +	struct intel_dp_link_config link_config;
> +	struct intel_dp_link_caps_iter iter;
> +	bool found = false;
> +
> +	intel_dp_link_caps_iter_start(&iter, link_caps, order, limits->link_config_filter);
> +	for_each_dp_link_config(&iter, &link_config) {
> +		if (!intel_dp_can_support_m_n(display, pixel_clock, link_config.rate))
> +			continue;
> +
> +		intel_dp_link_caps_filter_add(link_caps, &new_filter, &link_config);
> +		found = true;
> +	}
> +	intel_dp_link_caps_iter_end(&iter);
> +
> +	if (!found) {
> +		drm_dbg_kms(display->drm,
> +			    "[CONNECTOR:%d:%s] No link config with a Link M/N ratio <= %d\n",
> +			    connector->base.base.id, connector->base.name,
> +			    intel_dp_get_max_m_n_ratio(display));
> +
> +		return false;
> +	}
> +
> +	limits->link_config_filter = new_filter;
> +
> +	return true;
> +}
> +
>  bool
>  intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>  			       struct drm_connector_state *conn_state,
> @@ -2772,6 +2883,9 @@ intel_dp_compute_config_limits(struct intel_dp *intel_dp,
>  		limits->link_config_filter = new_filter;
>  	}
>  
> +	if (!intel_dp_compute_m_n_ratio_limits(intel_dp, conn_state, crtc_state, limits))
> +		return false;
> +
>  	if (!intel_dp_test_compute_config(connector, crtc_state, limits))
>  		return false;
>  

[Severity: Medium]
Should intel_dp_compute_m_n_ratio_limits() be called earlier in
intel_dp_compute_config_limits() in drivers/gpu/drm/i915/display/intel_dp.c,
and should intel_dp_test_compute_config() also run before or intersect with
that filter?

Right before the new intel_dp_compute_m_n_ratio_limits() call,
intel_dp_compute_config_limits() narrows limits->link_config_filter to a
single max_config when is_mst or intel_dp->use_max_params is set:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_compute_config_limits() {
	...
	if (is_mst || intel_dp->use_max_params) {
		struct intel_dp_link_caps_filter new_filter = INTEL_DP_LINK_CAPS_FILTER_NONE;
		struct intel_dp_link_config max_config;
		...
		if (!intel_dp_get_connector_max_link_config(connector, limits, &max_config))
			return false;

		if (!intel_dp_link_caps_filter_add(link_caps, &new_filter, &max_config))
			return false;

		limits->link_config_filter = new_filter;
	}

	if (!intel_dp_compute_m_n_ratio_limits(intel_dp, conn_state, crtc_state, limits))
		return false;

	if (!intel_dp_test_compute_config(connector, crtc_state, limits))
		return false;
	...
}

Because the link symbol clock is not monotonic with link rate or bandwidth
(e.g., 128b/132b rates divide by 32 while 8b/10b rates divide by 10),
intel_dp_get_connector_max_link_config() can pick a high-rate configuration
with a lower symbol clock that violates intel_dp_can_support_m_n(), even
when a lower link rate in link_caps has a higher symbol clock that satisfies
the ratio.

Since limits->link_config_filter has already been restricted to that single
max_config before intel_dp_compute_m_n_ratio_limits() runs, won't
intel_dp_compute_m_n_ratio_limits() fail the modeset instead of allowing
intel_dp_get_connector_max_link_config() to select the highest configuration
among those that satisfy the ratio?

Conversely, intel_dp_test_compute_config() runs immediately after
intel_dp_compute_m_n_ratio_limits(). When a compliance link training request
is active, intel_dp_test_compute_config() -> set_filter_for_link_params() ->
set_filter_for_link_config() in drivers/gpu/drm/i915/display/intel_dp_test.c
replaces limits->link_config_filter with a fresh filter built from link_caps:

drivers/gpu/drm/i915/display/intel_dp_test.c:set_filter_for_link_config() {
	struct intel_dp_link_caps_filter new_filter = INTEL_DP_LINK_CAPS_FILTER_NONE;

	if (!intel_dp_link_caps_filter_add(link_caps, &new_filter, link_params))
		return false;

	limits->link_config_filter = new_filter;

	return true;
}

Won't that overwrite and discard the filter computed by
intel_dp_compute_m_n_ratio_limits()?

[ ... ]

>
>
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021072438.1164249-1-suraj.kandpal@intel.com?part=2