Re: [PATCH v2 06/27] drm/crtc-helper: Add VRR helper functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/crtc-helper: unsynchronized crtc_state mutation in VRR helpers
- [Medium] drm/crtc-helper: vrr->dynamic set and left true when VRR is disabled
--

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

drm/crtc-helper: Add VRR helper functions

This commit adds drm_crtc_helper_vrr_flush() and drm_crtc_helper_vrr_vsync()
to update the internal VRR state in struct drm_crtc_state when a new frame is
ready or when a vertical blank ends, returning whether cur_vtotal changed.

> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 4a8cfe046e4ab..4ffb3b0b78780 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -838,6 +838,69 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set,
>  }
>  EXPORT_SYMBOL(drm_crtc_helper_set_config);
>  
> +/**
> + * drm_crtc_helper_vrr_flush - Update VRR state on frame ready
> + * @crtc_state: pointer to &struct drm_crtc_state of CRTC that has a new frame ready
> + *
> + * Update the VRR state by communicating that a frame is ready on
> + * @crtc_state.crtc.
> + *
> + * Should be called from the @crtc_state.crtc &drm_crtc_helper_funcs.atomic_flush
> + * hook.
> + *
> + * Returns:
> + * - %true if @crtc_state.vrr_state.cur_vtotal changed
> + * - %false otherwise
> + */
> +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;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_crtc_helper_vrr_flush);

[Severity: High]
As reported by Sashiko on RFC [1], is it safe for drm_crtc_helper_vrr_vsync()
and drm_crtc_helper_vrr_flush() to mutate crtc_state->vrr_state in place
across commits and from the vblank interrupt handler?

Consider a sequence of two atomic commits when VRR is enabled:

1. Commit 1 runs atomic_flush and calls drm_crtc_helper_vrr_flush(state_1),
   which sets state_1->vrr_state.cur_vtotal = base_vtotal and programs the
   hardware vtotal to base_vtotal.

2. Commit 2 starts and calls drm_atomic_get_crtc_state() ->
   __drm_atomic_helper_crtc_duplicate_state(), which copies crtc->state
   (state_1) into state_2 via memcpy(), snapshotting
   state_2->vrr_state.cur_vtotal = base_vtotal.

3. Before commit 2 calls drm_atomic_helper_swap_state() (or while waiting on
   fences / hw_done), a vblank interrupt fires and the driver's vblank
   handler calls drm_crtc_helper_vrr_vsync(crtc->state) on state_1, setting
   state_1->vrr_state.cur_vtotal = max_vtotal and programming the hardware
   vtotal to max_vtotal.

4. Commit 2 runs drm_atomic_helper_swap_state(), replacing crtc->state with
   state_2, which still holds the stale vrr_state.cur_vtotal == base_vtotal
   copied in step 2.

5. When commit 2 reaches atomic_flush and calls
   drm_crtc_helper_vrr_flush(state_2), vrr->cur_vtotal == vrr->base_vtotal
   evaluates to true and the helper returns false, causing the driver to skip
   reprogramming the hardware vtotal back from max_vtotal to base_vtotal for
   the new frame.

In addition, drm_crtc_helper_vrr_vsync() mutates crtc->state->vrr_state from
hardirq context without synchronization against
__drm_atomic_helper_crtc_duplicate_state(), drm_atomic_helper_swap_state(),
drm_crtc_helper_vrr_flush(), or state destruction after
drm_atomic_helper_commit_hw_done(). Should the mutable runtime VRR state live
on struct drm_crtc protected by an irq-safe spinlock rather than in
struct drm_crtc_state?

> +
> +/**
> + * drm_crtc_helper_vrr_vsync - Update VRR state on vblank end
> + * @crtc_state: pointer to &struct drm_crtc_state of CRTC the vblank occurred on
> + *
> + * Update the VRR state by communicating that a vertical blank has ended and a
> + * new frame is about to start.
> + *
> + * Returns:
> + *  - %true if @crtc_state.vrr_state.cur_vtotal changed
> + *  - %false otherwise
> + */
> +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;
> +
> +	if (abs_diff(vrr->cur_vtotal, old_vtotal) > 1)
> +		vrr->dynamic = true;
> +
> +	return old_vtotal != vrr->cur_vtotal;
> +}

[Severity: Medium]
When crtc_state->vrr_enabled is false, drm_crtc_helper_vrr_vsync() sets
vrr->cur_vtotal = 0 and then checks:

	if (abs_diff(vrr->cur_vtotal, old_vtotal) > 1)
		vrr->dynamic = true;

If VRR was previously enabled so old_vtotal > 1 (including fixed-rate VRR
where vrr->dynamic was false), doesn't abs_diff(0, old_vtotal) > 1 evaluate
to true and set vrr->dynamic = true when VRR is disabled?

Neither drm_crtc_helper_vrr_vsync() nor drm_crtc_helper_vrr_flush() resets
vrr->dynamic to false in the !crtc_state->vrr_enabled path either, leaving
vrr->dynamic stuck at true after VRR is turned off. Should both helpers clear
vrr->dynamic = false when !crtc_state->vrr_enabled?

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

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