Re: [PATCH v7 02/13] 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 702cc07abf5c42e8a3c593b53932c55fcc454655
Author: Suraj Kandpal <suraj.kandpal@intel.com>

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

This commit restricts DisplayPort link configurations and prunes modes where
the ceiling of the Link M/N ratio exceeds the hardware limit supported by the
transcoder timing generator DDA.

> 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]
Does calling intel_dp_compute_m_n_ratio_limits() after the
(is_mst || intel_dp->use_max_params) block and before
intel_dp_test_compute_config() in intel_dp_compute_config_limits() cause two
filtering regressions?

First, in drivers/gpu/drm/i915/display/intel_dp.c:
intel_dp_compute_config_limits() narrows limits->link_config_filter to a
single max_config before intel_dp_compute_m_n_ratio_limits() runs:

	if (is_mst || intel_dp->use_max_params) {
		...
		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;

Because 128b/132b symbol clocks (rate * 10 / 32) are lower than 8b/10b symbol
clocks (rate * 10 / 10), max_config can pick a UHBR rate such as UHBR10
(312.5 MHz symbol clock) or UHBR20 (625 MHz symbol clock) that exceeds the
M/N ratio limit for a high pixel clock mode, even when a lower link rate like
HBR3 (810 MHz symbol clock) or HBR2 (540 MHz symbol clock) satisfies the M/N
ratio and passed intel_dp_mode_valid_m_n(). Since
intel_dp_compute_m_n_ratio_limits() only iterates over
limits->link_config_filter, won't it reject the modeset instead of allowing
intel_dp_get_connector_max_link_config() to select the highest M/N-compatible
configuration?

Second, intel_dp_test_compute_config() is called right after
intel_dp_compute_m_n_ratio_limits(). When intel_dp->compliance.test_type is
DP_TEST_LINK_TRAINING, 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 overwrites
limits->link_config_filter with a fresh filter:

static bool set_filter_for_link_config(struct intel_connector *connector,
				       struct intel_dp_link_caps *link_caps,
				       const struct intel_dp_link_config *link_params,
				       struct link_config_limits *limits)
{
	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;
}

Because intel_dp_link_caps_filter_add() checks get_allowed_config_filter()
rather than limits->link_config_filter, won't this discard the M/N ratio
filter applied by intel_dp_compute_m_n_ratio_limits() and allow an
M/N-invalid compliance link configuration?

Should intel_dp_compute_m_n_ratio_limits() be called before the
(is_mst || intel_dp->use_max_params) block, or after
intel_dp_test_compute_config() while also filtering before
intel_dp_get_connector_max_link_config()?

>  
[ ... ]

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