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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm: Missing DRM core validation for VRR target properties
- [Medium] drm: Missing DRM_MODE_PROP_ATOMIC flag on multi-part VRR target properties
- [Low] drm: Redundant bounds check for VRR target properties in drm_atomic_crtc_set_property
--

commit 5c8aa8fe0326bb376d279373eb0661cd2fcc114e
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Subject: drm: Add VRR target frame rate properties

This commit introduces new VRR target frame rate properties, allowing
user-space to set a target refresh rate range. The rates are expressed as
rational fractions using numerators and denominators. It adds the UAPI
properties, validation fields, and attaches them to CRTC objects.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1eebcf6f5e05b..c9eb01d740b3e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -420,6 +420,22 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		return ret;
>  	} else if (property == config->prop_vrr_enabled) {
>  		state->vrr_enabled = val;
> +	} else if (property == config->prop_vrr_min_numerator) {
> +		if (val > U32_MAX)
> +			return -EINVAL;

[Severity: Low]
Is this manual bounds check necessary? Since this is a range property created
with limits of 0 to U32_MAX, the DRM core's drm_property_change_valid_get()
should already validate val before reaching drm_atomic_crtc_set_property(),
making this check redundant.

> +		state->vrr_state.vrr_min_n = val;
> +	} else if (property == config->prop_vrr_min_denominator) {
> +		if (val > U32_MAX)
> +			return -EINVAL;
> +		state->vrr_state.vrr_min_d = val;

[Severity: Medium]
Could this lack of validation allow structurally invalid states? The UAPI
documentation states the denominator must not be zero if the numerator is
non-zero. By accepting the denominator without validating it against the
numerator here, this constraint isn't enforced in the DRM core.

Could this cause drivers implementing VRR to hit divide-by-zero crashes if
they don't manually validate these constraints themselves?

[ ... ]

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

[Severity: Medium]
Should these multi-part properties be created with the DRM_MODE_PROP_ATOMIC
flag? Without it, they are exposed to legacy user-space applications.

Since legacy userspace updates properties one at a time using
drm_mode_obj_set_property_ioctl(), setting a non-zero numerator will
immediately trigger a commit while the denominator is still 0. This
passes an invalid intermediate state to the driver, causing it to reject the
modeset and making it impossible for legacy userspace to configure a target
rate.

> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_vrr_min_numerator = prop;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=13