Re: [PATCH 2/9] drm/i915/dp: Limit m/n ratio to 10 for DP SST

[Imre Deak <imre.deak@intel.com>]

On Tue, Jul 22, 2025 at 11:25:32AM +0530, Nautiyal, Ankit K wrote:
> 
> On 7/21/2025 5:11 PM, Imre Deak wrote:
> > On Mon, Jul 21, 2025 at 02:45:22PM +0530, Ankit Nautiyal wrote:
> > > The hardware cannot support DisplayPort configurations where the
> > > ceiling of the Link M/Link N ratio exceeds 10. This limitation has
> > > always existed, but it typically wasn't encountered without the use of
> > > joiners and DSC.
> > > 
> > > With higher resolutions and combinations involving joiners and DSC,
> > > this constraint can now be hit in certain scenarios.
> > > 
> > > Introduce a check during link rate configuration to ensure
> > > the computed M/N ratio does not exceed the hardware limit. If no valid
> > > link rate satisfies this constraint, the mode will be rejected.
> > > 
> > > Note: This change applies the check only for SST. Support for MST will
> > > be added in a subsequent commit.
> > > 
> > > v2:
> > > -Move the M/N ratio check to the link rate configuration phase instead
> > > of during M/N computation. (Ville)
> > > -Prune modes that cannot be supported even with highest link rate due to
> > > M/N ratio restriction.
> > > 
> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_display.c |  1 -
> > >   drivers/gpu/drm/i915/display/intel_display.h |  3 ++
> > >   drivers/gpu/drm/i915/display/intel_dp.c      | 33 +++++++++++++++++++-
> > >   3 files changed, 35 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 671491a2a3b6..f32a4956c926 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2506,7 +2506,6 @@ static void compute_m_n(u32 *ret_m, u32 *ret_n,
> > >   	intel_reduce_m_n_ratio(ret_m, ret_n);
> > >   }
> > > -static
> > >   void intel_display_get_link_m_n(u32 *link_m, u32 *link_n,
> > >   				u32 pixel_clock,
> > >   				u32 link_clock)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > > index 37e2ab301a80..bfa3db219b9c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -559,5 +559,8 @@ bool assert_port_valid(struct intel_display *display, enum port port);
> > >   bool intel_scanout_needs_vtd_wa(struct intel_display *display);
> > >   int intel_crtc_num_joined_pipes(const struct intel_crtc_state *crtc_state);
> > > +void intel_display_get_link_m_n(u32 *link_m, u32 *link_n,
> > > +				u32 pixel_clock,
> > > +				u32 link_clock);
> > >   #endif
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 54d88f24b689..4245dd65b2af 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1407,6 +1407,28 @@ bool intel_dp_has_dsc(const struct intel_connector *connector)
> > >   	return true;
> > >   }
> > > +static int
> > > +intel_dp_get_max_m_n_ratio(void)
> > > +{
> > > +	return 10;
> > > +}
> > > +
> > > +static bool
> > > +intel_dp_can_support_m_n(int pixel_clock,
> > > +			 int link_rate)
> > > +{
> > > +	int max_m_n_ratio = intel_dp_get_max_m_n_ratio();
> > > +	u32 link_m, link_n;
> > > +	int m_n_ratio;
> > > +
> > > +	intel_display_get_link_m_n(&link_m, &link_n,
> > > +				   pixel_clock, link_rate);
> > > +
> > > +	m_n_ratio = DIV_ROUND_UP(link_m, link_n);
> > > +
> > > +	return m_n_ratio <= max_m_n_ratio;
> > > +}
> > > +
> > >   static enum drm_mode_status
> > >   intel_dp_mode_valid(struct drm_connector *_connector,
> > >   		    const struct drm_display_mode *mode)
> > > @@ -1518,6 +1540,9 @@ intel_dp_mode_valid(struct drm_connector *_connector,
> > >   	if (status != MODE_OK)
> > >   		return status;
> > > +	if (!intel_dp_can_support_m_n(target_clock, max_rate))
> > > +		return MODE_CLOCK_HIGH;
> > > +
> > >   	return intel_mode_valid_max_plane_size(display, mode, num_joined_pipes);
> > >   }
> > > @@ -1789,6 +1814,9 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> > >   			    link_rate > limits->max_rate)
> > >   				continue;
> > > +			if (!intel_dp_can_support_m_n(clock, link_rate))
> > > +				continue;
> > Could this limit be set in limits->min_rate, computed already in
> > intel_dp_compute_config_limits()? (And fail already there if this is
> > bigger than limits->max_rate.)
> 
> Yes right this can be done in intel_dp_compute_config_limit.
> 
> We can iterate over the array of common_rates and find the rate for which
> the ratio is within limits and bail out early if we cant find such a link
> rate.
> 
> Or otherwise instead of iterating over rates, I guess theoretically we can
> calculate the minimum link rate for given pixelclock and ratio and set it to
> limits->min_rate and bail out early if this is more than the
> limits->max_rate.

Right, the first option to just iterate over common_rates sounds better.

limits->min_rate should be set to a validate rate, so it will need to be
looked up from common_rates anyway. There's also
intel_dp_rate_limit_len()/intel_dp_common_rate(), but the former returns
the index for a rate which is <= than the limit passed to it (while >=
would be needed here), so can't be used as-is.

I noticed that the mode's pixel clock is adjusted later based on the MSO
link count (in intel_dp_compute_config()) and the number of joined pipes
(in intel_joiner_adjust_timings()), but I suppose this doesn't matter,
since the link m/n values are calculated with the unadjusted pixel
clock. Could you confirm this nevertheless?

> However this might be bit tricky as symbol size is different for uhbr.
> 
> I will check this out, and move this in intel_dp_compute_config_limits.
> 
> Thanks & Regards,
> 
> Ankit
> 
> 
> > 
> > > +
> > >   			for (lane_count = limits->min_lane_count;
> > >   			     lane_count <= limits->max_lane_count;
> > >   			     lane_count <<= 1) {
> > > @@ -1796,7 +1824,6 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> > >   									 link_rate,
> > >   									 lane_count);
> > > -
> > >   				if (mode_rate <= link_avail) {
> > >   					pipe_config->lane_count = lane_count;
> > >   					pipe_config->pipe_bpp = bpp;
> > > @@ -1983,6 +2010,10 @@ static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > >   		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > >   			continue;
> > > +		if (!intel_dp_can_support_m_n(adjusted_mode->clock,
> > > +					      link_rate))
> > > +			continue;
> > > +
> > >   		for (lane_count = limits->min_lane_count;
> > >   		     lane_count <= limits->max_lane_count;
> > >   		     lane_count <<= 1) {
> > > -- 
> > > 2.45.2
> > >