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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Friday, 25 September 2026 20:42:05 Central European Summer Time 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. It's then also clear if they requested
> a static Hz.

That's a good thought. It makes a lot of the logic simpler to catch bad
conditions, and it's the kind of thing that can definitely be checked in
connector-agnostic shared CRTC atomic_check code.

Thanks for the suggestion, I'll likely adopt it for the next revision!

Kind regards,
Nicolas Frattaroli

> 
> Thanks,
> Leo
>