Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/rockchip: vop2: plane left enabled in vop2_plane_atomic_update() - [High] drm/rockchip: vop2: unhandled vblank event in vop2_crtc_atomic_flush() --
commit ad40a8e305ba6e768ae3478a34b5ddce63b79357 Author: Cristian Ciocaltea <[email protected]> drm/rockchip: vop2: Balance state on atomic_enable() error paths This commit cleans up clocks, vop2->enable_count, and PM runtime state on vop2_crtc_atomic_enable() failure paths and introduces a per-video-port enabled flag so atomic commit callbacks skip hardware programming and teardown when a CRTC failed to enable. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index bc14ca9d598dc..8d5613531acb9 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -1197,13 +1203,21 @@ static void vop2_plane_atomic_disable(struct > drm_plane *plane, > struct drm_plane_state *old_pstate = NULL; > struct vop2_win *win = to_vop2_win(plane); > struct vop2 *vop2 = win->vop2; > + struct vop2_video_port *vp; > > drm_dbg(vop2->drm, "%s disable\n", win->data->name); > > if (state) > old_pstate = drm_atomic_get_old_plane_state(state, plane); > - if (old_pstate && !old_pstate->crtc) > - return; > + > + if (old_pstate) { > + if (!old_pstate->crtc) > + return; > + > + vp = to_vop2_video_port(old_pstate->crtc); > + if (!vp->enabled) > + return; > + } > > vop2_win_disable(win); > vop2_win_write(win, VOP2_WIN_YUV_CLIP, 0); > @@ -1298,6 +1312,9 @@ static void vop2_plane_atomic_update(struct drm_plane > *plane, > if (WARN_ON(!crtc)) > return; > > + if (!vp->enabled) > + return; > + > if (!pstate->visible) { > vop2_plane_atomic_disable(plane, state); > return; [Severity: High] What happens if an active plane is migrated from an enabled CRTC to another CRTC whose vop2_crtc_atomic_enable() failed? When a plane moves between CRTCs, drm_atomic_helper_commit_planes() sees new_plane_state->crtc != NULL and plane_crtc_active(new_plane_state) == true, so it calls vop2_plane_atomic_update() rather than vop2_plane_atomic_disable(). Meanwhile, on the source CRTC, vop2_crtc_atomic_begin() -> setup_overlay() only iterates over planes in the source CRTC's new plane_mask and does not disable the migrated window. In vop2_plane_atomic_update(), vp is derived from the destination CRTC (pstate->crtc). Returning early when !vp->enabled skips vop2_plane_atomic_disable(plane, state) and leaves VOP2_WIN_ENABLE set on the source CRTC while drm_atomic_helper_cleanup_planes() unmaps and releases old_plane_state->fb at the end of the commit tail, allowing the hardware window to continue DMA scanout from the old framebuffer. Since vop2_plane_atomic_disable() already checks old_pstate->crtc and whether the old video port is enabled before touching hardware registers, should vop2_plane_atomic_update() call vop2_plane_atomic_disable(plane, state) when !vp->enabled, just as it does when !pstate->visible? [ ... ] > @@ -2034,13 +2066,19 @@ static void vop2_crtc_atomic_flush(struct drm_crtc > *crtc, > struct vop2_video_port *vp = to_vop2_video_port(crtc); > struct vop2 *vop2 = vp->vop2; > > - /* In case of modeset, gamma lut update already happened in atomic > enable */ > - if (!drm_atomic_crtc_needs_modeset(crtc_state) && > crtc_state->color_mgmt_changed) > - vop2_crtc_atomic_try_set_gamma_locked(vop2, vp, crtc, > crtc_state); > + if (vp->enabled) { > + /* > + * In case of modeset, gamma lut update already happened > + * in atomic enable. > + */ > + if (!drm_atomic_crtc_needs_modeset(crtc_state) && > + crtc_state->color_mgmt_changed) > + vop2_crtc_atomic_try_set_gamma_locked(vop2, vp, crtc, > crtc_state); > > - vop2_post_config(crtc, false, crtc_state, old_crtc_state); > + vop2_post_config(crtc, false, crtc_state, old_crtc_state); > > - vop2_cfg_done(vp); > + vop2_cfg_done(vp); > + } > > spin_lock_irq(&crtc->dev->event_lock); [Severity: High] As reported by Sashiko on v2 [1], how is crtc->state->event handled in vop2_crtc_atomic_flush() when vop2_crtc_atomic_enable() fails and leaves vp->enabled as false? When vop2_crtc_atomic_enable() aborts on any error path, it returns before calling drm_crtc_vblank_on(crtc). Later in the same commit tail, drm_atomic_helper_commit_planes() still calls vop2_crtc_atomic_flush(), which runs: spin_lock_irq(&crtc->dev->event_lock); if (crtc->state->event) { WARN_ON(drm_crtc_vblank_get(crtc)); vp->event = crtc->state->event; crtc->state->event = NULL; } spin_unlock_irq(&crtc->dev->event_lock); Because drm_crtc_vblank_on(crtc) was never called, drm_crtc_vblank_get(crtc) fails with -EINVAL and triggers the WARN_ON(). The event is then moved into vp->event, but because the video port is not running and vop2_cfg_done(vp) was skipped, the frame start interrupt never fires to complete vp->event, and vop2_crtc_atomic_disable() only checks crtc->state->event while skipping teardown when !vp->enabled. Does this leave commit->flip_done unsignaled (stalling subsequent commits on this CRTC in drm_atomic_helper_wait_for_dependencies()) and leak vp->event if another commit flushes before the CRTC is enabled, or cause an unbalanced drm_crtc_vblank_put() in the interrupt handler if a later modeset succeeds? When !vp->enabled, should vop2_crtc_atomic_flush() send crtc->state->event immediately via drm_crtc_send_vblank_event() instead of queuing it in vp->event? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
