Re: [PATCH RFC 07/25] drm/crtc-helper: Add VRR helper functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: data race and atomic state violation in VRR helpers
--

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

drm/crtc-helper: Add VRR helper functions

This commit introduces two VRR helper functions, drm_crtc_helper_vrr_flush
and drm_crtc_helper_vrr_vsync. They modify the vertical front porch timing
to change the duration of a frame and update the internal VRR state.

> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
[ ... ]
> +bool drm_crtc_helper_vrr_flush(struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_crtc_vrr_state *vrr = &crtc_state->vrr_state;
> +
> +	if (!crtc_state->vrr_enabled) {
> +		if (vrr->cur_vtotal) {
> +			vrr->cur_vtotal = 0;
> +			return true;
> +		}
> +		return false;
> +	}
> +
> +	if (vrr->cur_vtotal == vrr->base_vtotal)
> +		return false;
> +
> +	vrr->cur_vtotal = vrr->base_vtotal;

[Severity: High]
Does this code modify the atomic CRTC state from process context?

In DRM, the active crtc->state is strictly immutable after commit.
Modifying it directly contributes to the improper use of atomic state
objects as dynamic hardware trackers, breaking state duplication invariants.

> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_crtc_helper_vrr_flush);
[ ... ]
> +bool drm_crtc_helper_vrr_vsync(struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_crtc_vrr_state *vrr = &crtc_state->vrr_state;
> +	u16 old_vtotal = vrr->cur_vtotal;
> +
> +	if (!crtc_state->vrr_enabled)
> +		vrr->cur_vtotal = 0;
> +	else
> +		vrr->cur_vtotal = vrr->max_vtotal;

[Severity: High]
Does this modify the immutable atomic CRTC state directly from an interrupt
handler?

Modifying it from an IRQ asynchronously without the CRTC mutex creates a data
race when drm_atomic_helper_crtc_duplicate_state() copies the state for a new
concurrent commit. The hardware tracking state (cur_vtotal) can be copied
mid-mutation, replacing the software request state with torn hardware state,
potentially causing torn reads and visual stuttering.

> +
> +	if (abs_diff(vrr->cur_vtotal, old_vtotal) > 1)
> +		vrr->dynamic = true;
> +
> +	return old_vtotal != vrr->cur_vtotal;
> +}

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