Re: [PATCH v2 01/27] drm: Add VRR state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] drm/hdmi: division by zero in hdmi_calculate_vtotal() on zero denom
- [High] drm/hdmi: hdmi_validate_vrr() rejects modes with refresh below vrr_min
- [High] drm/hdmi: invalid VRR error returns and stale state in hdmi_validate_vrr()
- [High] drm/hdmi: bypassed VRR validation and stale vrr_state in hdmi_validate_vrr()
- [Medium] drm/hdmi: u64 quotient truncated to u32 in hdmi_calculate_vtotal()
- [Medium] drm/hdmi: rounded mode_refresh in hdmi_validate_vrr() inflates base_vtotal
- [Medium] drm/hdmi: hdmi_validate_vrr() skipped when CRTC VRR state changes alone
- [Low] drm/hdmi: wrong numerator variable in hdmi_calculate_vtotal() comment
--

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

drm: Add VRR state

This commit adds struct drm_crtc_vrr_state to struct drm_crtc_state and
implements HDMI VRR state validation and vtotal calculation in
drm_hdmi_state_helper.c.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 7cdb7ca3dc12f..c1a84e30441a6 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -887,6 +887,210 @@ hdmi_generate_infoframes(const struct drm_connector *connector,

[ ... ]

> +static int hdmi_calculate_vtotal(const struct drm_display_mode *mode,
> +				 u32 rate_n, u32 rate_d, u16 *out_vtotal,
> +				 u32 *out_denom, u32 *out_frame_err)
> +{
> +	u32 denom, result, err;
> +	u64 numerator;
> +
> +	/*

[ ... ]

> +	 *                        clock
> +	 * Vtotal = ----------------------------------
> +	 *            Htotal * target_n / target_d
> +	 *
> +	 * Some algebra brings us to:
> +	 *
> +	 *                clock * target_n
> +	 * Vtotal = ----------------------------------
> +	 *                Htotal * rate_n

[Severity: Low]
This isn't a bug since the code below multiplies mode->crtc_clock by rate_d,
but shouldn't the numerator in this comment be clock * target_d (or
clock * rate_d) rather than clock * target_n?

> +	 *
> +	 * The amount of timing error we accumulate every frame will be
> +	 * the remainder of that division.

[ ... ]

> +	 */
> +
> +	if (check_mul_overflow(mode->crtc_htotal, rate_n, &denom))
> +		return -ERANGE;
> +
> +	if (check_mul_overflow(mode->crtc_clock * 1000ULL, rate_d, &numerator))
> +		return -ERANGE;
> +
> +	result = div_u64_rem(numerator, denom, &err);
> +	if (result > U16_MAX)
> +		return -ERANGE;

[Severity: High]
Can div_u64_rem() divide by zero here when denom is 0?

check_mul_overflow(mode->crtc_htotal, rate_n, &denom) returns false and sets
denom = 0 whenever mode->crtc_htotal == 0 or rate_n == 0, and div_u64_rem()
is then called without checking !denom.

In drm_atomic_helper_connector_hdmi_check() -> hdmi_validate_vrr(), mode is
taken from &new_crtc_state->mode during connector atomic_check before
mode_fixup() has run. If an in-kernel KMS caller or KUnit test sets a mode on
a CRTC via drm_atomic_set_mode_for_crtc() that was constructed without
drm_mode_set_crtcinfo() (such as via drm_display_mode_from_cea_vic(),
drm_kunit_display_mode_from_cea_vic(), DRM_MODE(), or DRM_MODE_INIT()),
mode->htotal and mode->vtotal are populated so drm_mode_vrefresh(mode)
succeeds, vfront (mode->crtc_vsync_start - mode->crtc_vdisplay) is 0, and
hdmi_calculate_vtotal() is reached with mode->crtc_htotal == 0.

Similarly, drm_mode_vrefresh() returns 0 (never negative) on invalid timings
or overflow, and hdmi_validate_vrr() only rejects mode_refresh < 0 ||
mode_refresh > 1023, so mode_refresh == 0 can also clamp max_vfreq_n to 0 and
pass rate_n == 0 into hdmi_calculate_vtotal().

[Severity: Medium]
Also in hdmi_calculate_vtotal(), since result is declared as u32 while
numerator is u64, doesn't assigning the return value of div_u64_rem() to
result truncate the upper 32 bits before checking result > U16_MAX (and raise
a divide error #DE in do_div() on 32-bit x86 if the quotient exceeds
U32_MAX)?

For example, a custom mode passed via DRM_IOCTL_MODE_ATOMIC with a large
clock and vscan > 1 (which reduces drm_mode_vrefresh(mode) without increasing
mode->crtc_htotal) can keep drm_mode_vrefresh(mode) <= 1023 while producing a
quotient in hdmi_calculate_vtotal() such as 0x10000040dULL, which truncates
to u32 result = 0x40d and bypasses the result > U16_MAX check. Should result
be u64 (or div64_u64_rem())?

> +
> +	if (out_vtotal)
> +		*out_vtotal = result;
> +	if (out_denom)
> +		*out_denom = denom;
> +	if (out_frame_err)
> +		*out_frame_err = err;
> +
> +	return 0;
> +}
> +
> +static int hdmi_validate_vrr(struct drm_connector *connector,
> +			     struct drm_atomic_commit *state)
> +{
> +	struct drm_connector_state *new_conn_state =
> +		drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_crtc_state *new_crtc_state =
> +		drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
> +	struct drm_crtc_vrr_state *vrr_state =
> +		&new_crtc_state->vrr_state;
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct drm_display_mode *mode = &new_crtc_state->mode;
> +	struct drm_device *dev = connector->dev;
> +	int mode_refresh, vfront, ret;
> +	u32 min_vfreq_n = info->hdmi.vrr_cap.vrr_min;
> +	u32 min_vfreq_d = 1;
> +	u32 max_vfreq_n = info->hdmi.vrr_cap.vrr_max;
> +	u32 max_vfreq_d = 1;
> +
> +	if (!new_crtc_state->vrr_enabled)
> +		return 0;
> +
> +	if (!info->hdmi.vrr_cap.supported)
> +		return -EOPNOTSUPP;

[Severity: High]
Does returning -EOPNOTSUPP here (and -EINVAL below when the mode or sink
cannot do HDMI VRR) break the DRM UAPI contract for VRR_ENABLED, and does
returning 0 when !new_crtc_state->vrr_enabled leave stale values in
new_crtc_state->vrr_state?

1. The documentation for drm_crtc_state.vrr_enabled in include/drm/drm_crtc.h
states:

	/*
	 * Indicates if variable refresh rate should be enabled for the CRTC.
	 * Support for the requested vrr state will depend on driver and
	 * hardware capabiltiy - lacking support is not treated as failure.
	 */

Every atomic CRTC attaches VRR_ENABLED in drm_crtc_init_with_planes(), and
drm_atomic_helper_connector_hdmi_check() unconditionally calls
hdmi_validate_vrr(). Because __drm_atomic_helper_set_config() and
drm_client_modeset_commit_atomic() preserve crtc->state->vrr_enabled across
modesets, hotplugging a non-VRR HDMI sink while VRR_ENABLED is set causes
subsequent fbdev/drm_client console restores and legacy SETCRTC calls to fail
with -EOPNOTSUPP. It also rejects commits on HDMI drivers that use
drm_atomic_helper_connector_hdmi_check() without supporting VRR, as well as
connectors set to VRR_DISPLAY_MODE_OFF in later commit "drm/connector: add
vrr_display_mode connector property".

2. When new_crtc_state->vrr_enabled transitions from true to false,
__drm_atomic_helper_crtc_duplicate_state() copies the previous
crtc->state->vrr_state via memcpy(). Returning 0 immediately at
if (!new_crtc_state->vrr_enabled) without clearing new_crtc_state->vrr_state
leaves base_vtotal, max_vtotal, cur_vtotal, and dynamic populated. In later
commit "drm/connector: hdmi: Add VTEM EMP generation",
hdmi_generate_emp_infoframe_vtem() sets vtem->m_const = !vrr->dynamic, so
leaving vrr->dynamic as true transmits M_CONST = 0 when VRR is disabled. In
addition, in later commit "drm/rockchip: vop2: Enable VRR" (with
"drm/crtc-helper: Add VRR helper functions"), vop2_crtc_atomic_flush() only
calls drm_crtc_helper_vrr_flush() when crtc_state->vrr_enabled is true; since
cur_vtotal was not cleared when VRR was disabled, re-enabling VRR at the same
base_vtotal sees cur_vtotal == base_vtotal and skips reprogramming the
hardware VTOTAL register.

> +
> +	mode_refresh = drm_mode_vrefresh(mode);
> +
> +	/* HDMI uses 10 bits to signal the base refresh. */\
> +	if (mode_refresh < 0 || mode_refresh > 1023) {
> +		drm_dbg_kms(dev, "Mode's refresh of %dHz > HDMI VRR maximum (1023)\n",
> +			    mode_refresh);
> +		return -EINVAL;
> +	}
> +
> +	if (!max_vfreq_n || max_vfreq_n > mode_refresh)
> +		max_vfreq_n = mode_refresh;
> +
> +	if (!min_vfreq_n) {
> +		drm_dbg_kms(dev, "Could not get minimum VRR rate from sink\n");
> +		return -EINVAL;
> +	}
> +
> +	vfront = mode->crtc_vsync_start - mode->crtc_vdisplay;
> +	if (vfront < 0 || vfront > U8_MAX) {
> +		drm_dbg_kms(dev, "Vfront of %d would not fit in VTEM packet\n", vfront);
> +		return -EINVAL;
> +	}
> +
> +	if (cmp_fraction(min_vfreq_n, min_vfreq_d, max_vfreq_n, max_vfreq_d) > 0) {
> +		drm_dbg_kms(dev, "Target max (%u/%u) > target min (%u/%u)\n",
> +			    max_vfreq_n, max_vfreq_d, min_vfreq_n, min_vfreq_d);
> +		return -EINVAL;
> +	}

[Severity: High]
Doesn't clamping max_vfreq_n to mode_refresh before this cmp_fraction() check
cause hdmi_validate_vrr() to reject any mode whose refresh rate is below the
sink's vrr_min (such as 24Hz, 25Hz, or 30Hz modes on a 48-120Hz HDMI 2.1 TV)?

Because max_vfreq_n is clamped to at most mode_refresh above, whenever
mode_refresh < min_vfreq_n we have min_vfreq_n > max_vfreq_n, so
cmp_fraction(min_vfreq_n, min_vfreq_d, max_vfreq_n, max_vfreq_d) > 0 is true
and returns -EINVAL (while also logging an inverted debug message claiming
Target max > target min). That also makes the fallback branch further down:

	if (cmp_fraction(min_vfreq_n, min_vfreq_d, mode_refresh, 1) >= 0) {
		/* refresh <= VRR min, don't do VRR vtotal adjustment */
		vrr_state->max_vtotal = mode->crtc_vtotal;
	}

unreachable whenever mode_refresh < min_vfreq_n.

> +
> +	ret = hdmi_calculate_vtotal(mode, max_vfreq_n, max_vfreq_d,
> +				    &vrr_state->base_vtotal, NULL, NULL);
> +	if (ret) {
> +		drm_dbg_kms(dev, "Couldn't calculate base_vtotal: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/*
> +	 * Even at its fastest, it can't go faster than the mode, so
> +	 * clamp to avoid imprecisely rounded mode_refresh values we
> +	 * can't do anything about from ruining our day.
> +	 */
> +	vrr_state->base_vtotal = max(vrr_state->base_vtotal, mode->crtc_vtotal);
> +
> +	if (cmp_fraction(min_vfreq_n, min_vfreq_d, mode_refresh, 1) >= 0) {
> +		/* refresh <= VRR min, don't do VRR vtotal adjustment */
> +		vrr_state->max_vtotal = mode->crtc_vtotal;
> +	} else {

[Severity: Medium]
What happens here when drm_mode_vrefresh(mode) rounds down a mode's
fractional refresh rate?

When !max_vfreq_n || max_vfreq_n > mode_refresh, max_vfreq_n is set to the
integer-rounded mode_refresh from drm_mode_vrefresh(mode) (which uses
DIV_ROUND_CLOSEST_ULL). If the mode's actual refresh rate has a fractional
part in (0, 0.5) Hz (for example, 800x600@60.317Hz with clock = 40000,
htotal = 1056, vtotal = 628 where drm_mode_vrefresh() rounds down to 60, or a
48.4Hz mode with clock = 48400, htotal = 1000, vtotal = 1000 where
drm_mode_vrefresh() rounds down to 48), hdmi_calculate_vtotal() divides by
the smaller rounded rate and computes vrr_state->base_vtotal >
mode->crtc_vtotal (631 > 628, or 1008 > 1000).

Clamping with max(vrr_state->base_vtotal, mode->crtc_vtotal) only guards
against mode_refresh rounding up, not rounding down. Moreover, if
min_vfreq_n == mode_refresh (e.g. vrr_min = 48 on the 48.4Hz mode), the
cmp_fraction(min_vfreq_n, min_vfreq_d, mode_refresh, 1) >= 0 branch sets
vrr_state->max_vtotal = mode->crtc_vtotal (1000), leaving
vrr_state->base_vtotal (1008) > vrr_state->max_vtotal (1000).

When max_vfreq is not capped below the mode's refresh rate, shouldn't
vrr_state->base_vtotal simply be mode->crtc_vtotal?

[ ... ]

> @@ -919,6 +1123,10 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>  
>  	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
>  
> +	ret = hdmi_validate_vrr(connector, state);
> +	if (ret)
> +		return ret;

[Severity: High]
Is calling hdmi_validate_vrr() only from
drm_atomic_helper_connector_hdmi_check() sufficient when userspace updates
CRTC VRR properties without a modeset or connector property change?

VRR_ENABLED (as well as the VRR_{MIN,MAX}_{NUMERATOR,DENOMINATOR} properties
added later in commit "drm: Add VRR target frame rate properties") are CRTC
properties stored on struct drm_crtc_state. In
drm_atomic_helper_check_modeset(), drm_atomic_add_affected_connectors() is
only called when drm_atomic_crtc_needs_modeset() is true. If userspace
submits a DRM_IOCTL_MODE_ATOMIC commit that changes VRR properties on an
active CRTC without changing MODE_ID, ACTIVE, or any connector property, the
connector is not added to state->connectors and
drm_atomic_helper_connector_hdmi_check() -> hdmi_validate_vrr() is never
invoked.

Although later commit "drm/rockchip: Set mode_changed on vrr_enabled change"
sets mode_changed = true in rockchip_drm_atomic_check() when vrr_enabled
toggles, other HDMI drivers using drm_atomic_helper_connector_hdmi_check()
with drm_atomic_helper_check() will skip hdmi_validate_vrr() when VRR_ENABLED
toggles, leaving new_crtc_state->vrr_state uncomputed (or stale). Even on
Rockchip, updating VRR_{MIN,MAX}_{NUMERATOR,DENOMINATOR} while vrr_enabled
remains true does not set mode_changed or add the connector to the atomic
state, bypassing hdmi_validate_vrr() and VTEM infoframe regeneration.

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