Re: [PATCH v2 09/27] drm: Add VRR target frame rate properties

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm: missing DRM_MODE_PROP_ATOMIC flag on VRR limiter CRTC properties
- [Medium] drm: missing min <= max VRR rate check in drm_atomic_crtc_check()
--

commit 221e142e46603679a11c0d1b71599449f1d7a39c
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm: Add VRR target frame rate properties

This commit introduces four CRTC properties (VRR_MIN_NUMERATOR,
VRR_MIN_DENOMINATOR, VRR_MAX_NUMERATOR, and VRR_MAX_DENOMINATOR) to allow
userspace to configure a target frame rate range for Variable Refresh Rate
as numerator/denominator fractions.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9bb761eda59f4..da5c6925c349f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -489,6 +489,7 @@ EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  static int drm_atomic_crtc_check(const struct drm_crtc_state *old_crtc_state,
>  				 const struct drm_crtc_state *new_crtc_state)
>  {
> +	const struct drm_crtc_vrr_state *vrr = &new_crtc_state->vrr_state;
>  	struct drm_crtc *crtc = new_crtc_state->crtc;
>  
>  	/* NOTE: we explicitly don't enforce constraints such as primary
> @@ -544,6 +545,19 @@ static int drm_atomic_crtc_check(const struct drm_crtc_state *old_crtc_state,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Reject VRR limiter ranges that only specify one end of the limit, or
> +	 * specify a numerator > 0 with a denominator that is 0.
> +	 */
> +	if ((!vrr->vrr_min_n || !vrr->vrr_min_d ||
> +	     !vrr->vrr_max_n || !vrr->vrr_max_d) &&
> +	    (vrr->vrr_min_n > 0 || vrr->vrr_max_n > 0)) {
> +		drm_dbg_atomic(crtc->dev,
> +			       "[CRTC:%d:%s] incomplete VRR limiter range\n",
> +			       crtc->base.id, crtc->name);
> +		return -EINVAL;
> +	}

[Severity: Medium]
As reported by Sashiko on RFC [1], should drm_atomic_crtc_check() also reject
inverted VRR target frame rate ranges where the minimum rate fraction exceeds
the maximum rate fraction?

In include/drm/drm_mode_config.h, the kerneldoc for struct drm_mode_config
documents that prop_vrr_max_numerator divided by prop_vrr_max_denominator must
result in a frame rate equal to or above prop_vrr_min_numerator divided by
prop_vrr_min_denominator.

However, drm_atomic_crtc_check() only checks that all four values are non-zero
when either numerator is positive. If userspace submits an atomic commit via
DRM_IOCTL_MODE_ATOMIC with non-zero limiter values where vrr_min_n / vrr_min_d
is greater than vrr_max_n / vrr_max_d (for example while vrr_enabled is false,
or on a non-HDMI connector), the check succeeds and commits the inverted range
into drm_crtc_state.

Would it make sense to check:

    mul_u32_u32(vrr->vrr_min_n, vrr->vrr_max_d) >
    mul_u32_u32(vrr->vrr_max_n, vrr->vrr_min_d)

when the limiter range is non-zero in drm_atomic_crtc_check()?

[ ... ]

> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 48a56f23dadbb..da967d60f832a 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -535,6 +535,26 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.prop_vrr_enabled = prop;
>  
> +	prop = drm_property_create_range(dev, 0, "VRR_MIN_NUMERATOR", 0, U32_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_vrr_min_numerator = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "VRR_MIN_DENOMINATOR", 0, U32_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_vrr_min_denominator = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "VRR_MAX_NUMERATOR", 0, U32_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_vrr_max_numerator = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "VRR_MAX_DENOMINATOR", 0, U32_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_vrr_max_denominator = prop;

[Severity: Medium]
As reported by Sashiko on RFC [2], should these four properties be created
with the DRM_MODE_PROP_ATOMIC flag in drm_mode_create_standard_properties()?

Because flags is passed as 0 to drm_property_create_range(),
drm_mode_object_get_properties() exposes these properties to legacy non-atomic
userspace clients. Non-atomic clients update properties one at a time via
DRM_IOCTL_MODE_OBJ_SETPROPERTY:

    drm_mode_obj_set_property_ioctl()
      -> set_property_atomic()
        -> drm_atomic_commit()
          -> drm_atomic_check_only()
            -> drm_atomic_crtc_check()

Since drm_atomic_crtc_check() rejects any state where only part of the
limiter range is non-zero with -EINVAL, a legacy client cannot transition
from (0, 0, 0, 0) to an active range or clear an active range back to
(0, 0, 0, 0) one property at a time without failing on the intermediate
commits.

[1] https://lore.kernel.org/r/20260921-vrr-limiter-uapi-v1-13-2fcd7d011646@collabora.com
[2] https://lore.kernel.org/r/20260921-vrr-limiter-uapi-v1-13-2fcd7d011646@collabora.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-vrr-limiter-uapi-v2-0-55f4215be2c2@collabora.com?part=9