Re: [PATCH RFC 13/25] drm: Add VRR target frame rate properties

["Michel Dänzer" <michel.daenzer@mailbox.org>]

On 9/25/26 20:42, Leo Li wrote:
> On 2026-09-22 11:26, Nicolas Frattaroli wrote:
>>>> + * VRR Limiter/Target Properties
>>>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> + *
>>>> + * The ``VRR_{MIN,MAX}_{NUMERATOR,DENOMINATOR}`` properties expose a mechanism
>>>> + * through which userspace can control the desired range of refresh rates in
>>>> + * which VRR is allowed to operate. Each rate is expressed as a
>>>> + * numerator/denominator fraction of refresh rates in Hz, allowing for rational
>>>> + * target rates like 24/1.001 Hz with no loss of precision or ambiguity.
>>>> + *
>>>> + * If the minimum and maximum rate are set to the same value (and not 0), they
>>>> + * are understood as a fixed target rate. This is especially useful for media
>>>> + * playback, where the content's frame rate is both constant and known in
>>>> + * advance. In such cases, a refresh rate that is not an integer multiple of the
>>>> + * content's frame rate will introduce judder, since not every frame is
>>>> + * displayed for the same amount of time. A modeset of the display with a
>>>> + * compatible rate may in those cases be either undesirable or impossible, but
>>>> + * the rate can still effectively be reached through VRR.
>>>> + *
>>>> + * .. _VRR-MIN-NUMERATOR:
>>>> + *
>>>> + * "VRR_MIN_NUMERATOR":
>>>> + *	Default &drm_crtc integer property forming the numerator of a
>>>> + *	numerator/denominator pair of a frame rate to set as the minimum VRR
>>>> + *	target rate. Set to 0 to disable.
>>>> + *
>>>> + * "VRR_MIN_DENOMINATOR":
>>>> + *	Default &drm_crtc integer property forming the denominator of a
>>>> + *	numerator/denominator pair of a frame rate to set as the minimum VRR
>>>> + *	target rate. If :ref:`VRR_MIN_NUMERATOR <VRR-MIN-NUMERATOR>` is not
>>>> + *	zero, it must be non-zero.
>>>> + *	Otherwise, must also be zero.
>>>> + *
>>>> + * .. _VRR-MAX-NUMERATOR:
>>>> + *
>>>> + * "VRR_MAX_NUMERATOR":
>>>> + *	Default &drm_crtc integer property forming the numerator of a
>>>> + *	numerator/denominator pair of a frame rate to set as the maximum VRR
>>>> + *	target rate. Set to 0 to disable.
>>>> + *
>>>> + * "VRR_MAX_DENOMINATOR":
>>>> + *	Default &drm_crtc integer property forming the denominator of a
>>>> + *	numerator/denominator pair of a frame rate to set as the maximum VRR
>>>> + *	target rate. If :ref:`VRR_MAX_NUMERATOR <VRR-MAX-NUMERATOR>` is not
>>>> + *	zero, it must be non-zero. Otherwise, must also be zero.
>>>>   */
>>> If VRR_MIN_NUMERATOR == 0 && VRR_MAX_NUMERATOR > 0, do we interpret that as
>>> vrr limiting is disabled?
>> You can picture VRR limiting as always being active, but with a limit rational
>> of 0 it uses the display's limit as per the EDID, which is what unlimited game
>> mode is. So with how it's implemented right now in hdmi_validate_vrr(), your
>> example would set a maximum target, but leave the minimum at whatever the
>> display defaults to.
>>
>> Now that I'm thinking through this, a possible problem is that
>> drm_crtc_helper_vrr_is_fixed_rate() operates on the user supplied limits, but
>> if the display supplied lower limit is equal to the user supplied upper limit,
>> then we have a fixed rate scenario without recognising it as such. I think I
>> need to have a ponder on what the least surprising behaviour for userspace
>> is in that instance. The display limit stuff gets a bit complex due to
>> CinemaVRR and QMS TFRmin/TFRmax.
>>
>> I'll improve the documentation on the next revision to make the meanings more
>> explicit.
> 
> Perhaps a simple way is to require simultaneous setting MIN and MAX pairs?
> IOW, require userspace to set MIN and MAX simultaneously to >0, or =0. For example:
> 
> if ((vrr_min_n == 0 || vrr_min_d == 0 ||
>      vrr_max_n == 0 || vrr_max_d == 0) &&
>     (vrr_min_n > 0 || vrr_max_n > 0))
> 	return -EINVAL;
> That way, it's never ambiguous what userspace has requested for the range.
> They can copy the EDID supported range if they don't care about limiting one side, rather than leaving it at 0.

Determining the actual limits can be non-trivial (though I guess that might be fine as long as libdisplay-info can work them out), if user space gets them wrong, it might accidentally apply a narrower limit than intended.


> It's then also clear if they requested a static Hz.

I do see the benefit of your suggestion for this though.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast