On Thu Jul 16, 2026 at 5:21 PM CST, sashiko-bot wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] Race condition in atomic commit causing display hang in oneshot mode > due to premature hardware start. > -- > > --- Patch [3]: [PATCH v2 3/8] drm/mcde: replace struct > drm_simple_display_pipe with regular atomic helpers --- > commit 24a767bb6a2c4adfefe9e0a5de7e9c8bbe7b2a64 > Author: Ze Huang <[email protected]> > > drm/mcde: replace struct drm_simple_display_pipe with regular atomic helpers > > Convert MCDE to explicit plane, CRTC and encoder objects. Handle pending > page-flip events from the CRTC atomic flush path using the commit-local > CRTC state. > >> diff --git a/drivers/gpu/drm/mcde/mcde_display.c >> b/drivers/gpu/drm/mcde/mcde_display.c >> index 257a6e84dd583..86edf94c52372 100644 >> --- a/drivers/gpu/drm/mcde/mcde_display.c >> +++ b/drivers/gpu/drm/mcde/mcde_display.c > [ ... ] >> @@ -1381,44 +1398,13 @@ static void mcde_set_extsrc(struct mcde *mcde, u32 >> buffer_address) >> -static void mcde_display_update(struct drm_simple_display_pipe *pipe, >> - struct drm_plane_state *old_pstate) >> +static void mcde_plane_helper_atomic_update(struct drm_plane *plane, >> + struct drm_atomic_commit *commit) >> { >> - struct drm_crtc *crtc = &pipe->crtc; >> - struct drm_device *drm = crtc->dev; >> - struct mcde *mcde = to_mcde(drm); >> - struct drm_pending_vblank_event *event = crtc->state->event; >> - struct drm_plane *plane = &pipe->plane; >> - struct drm_plane_state *pstate = plane->state; >> + struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, >> plane); >> + struct mcde *mcde = to_mcde(plane->dev); >> struct drm_framebuffer *fb = pstate->fb; >> >> - /* >> - * Handle any pending event first, we need to arm the vblank >> - * interrupt before sending any update to the display so we don't >> - * miss the interrupt. >> - */ >> - if (event) { > [ ... ] >> - spin_unlock_irq(&crtc->dev->event_lock); >> - } >> - >> /* >> * We do not start sending framebuffer updates before the >> * display is enabled. Update events will however be dispatched > [ ... ] >> -static int mcde_display_enable_vblank(struct drm_simple_display_pipe *pipe) >> +static void mcde_crtc_helper_atomic_flush(struct drm_crtc *crtc, >> + struct drm_atomic_commit *commit) >> +{ >> + struct drm_crtc_state *crtc_state = >> drm_atomic_get_new_crtc_state(commit, crtc); >> + struct drm_pending_vblank_event *event = crtc_state->event; >> + >> + /* >> + * Handle any pending event first, we need to arm the vblank >> + * interrupt before sending any update to the display so we don't >> + * miss the interrupt. >> + */ >> + if (!event) >> + return; > > [Severity: High] > Does this split introduce a race condition that could hang the pipeline in > oneshot mode? > > Because the DRM core executes the plane's atomic_update callback before the > CRTC's atomic_flush callback, the hardware flow is now started before the > vblank event is armed. > > If mcde_start_flow() is triggered during mcde_plane_helper_atomic_update() > (which happens just below the quoted block), it could complete the hardware > transfer and fire its interrupt before the vblank event gets armed later in > mcde_crtc_helper_atomic_flush(). > > The relocated comment in atomic_flush still notes this exact requirement: > "we need to arm the vblank interrupt before sending any update to the > display so we don't miss the interrupt." > > If the interrupt fires early in oneshot mode, it will be missed, and the > atomic commit could wait indefinitely for an event that will never complete.
Hi, Thomas Would it make more sense to move this pending vblank event handling to the CRTC's atomic_begin(), or should it simply go back to update()? >From the comment in the original code, the event needs to be armed before the update is triggered, otherwise the completion interrupt could be missed in oneshot mode. Thanks, Ze
