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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 7/22/2025 2:52 PM, Imre Deak wrote:
> 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?

Yes this should be unadjusted pixel clock. I did not realize this 
earlier, will need to fix patch#4.

Also realized from Wa_14014191401 this need to be only for UHBR rates 
and also for DG2 the ratio is 4.

Regards,

Ankit

>
>> 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
>>>>