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 <[email protected]> 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/[email protected]?part=7
