Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Unhandled VBLANK event on `atomic_enable` error paths causes userspace to hang indefinitely and triggers a kernel warning. -- --- Patch [3]: [PATCH v3 03/14] drm/rockchip: vop2: Balance state on atomic_enable() error paths --- commit 852e494693bdb0b841a64b02cd2bceb48e307231 Author: Cristian Ciocaltea <[email protected]> drm/rockchip: vop2: Balance state on atomic_enable() error paths This commit attempts to fix resource leaks in the error paths of vop2_crtc_atomic_enable() by properly decrementing enable_count and unpreparing clocks. It also tracks the per-VP bring-up state to skip teardown in atomic_disable() and hardware programming in atomic_flush() when the enable phase does not complete successfully. 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 e11dc8c7f78ba..271c3cba6fb49 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -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] This is a pre-existing issue, but does this code leave userspace hanging on atomic_enable error paths? Since this patch uses vp->enabled to skip hardware programming on failure, what happens to the pending vblank event? If vop2_crtc_atomic_enable() fails, it returns before calling drm_crtc_vblank_on(). However, immediately following the spin_lock_irq() in vop2_crtc_atomic_flush(), the code unconditionally attempts to arm the event: if (crtc->state->event) { WARN_ON(drm_crtc_vblank_get(crtc) != 0); drm_crtc_arm_vblank_event(crtc, crtc->state->event); crtc->state->event = NULL; } Because vblank wasn't enabled during the failed bring-up, drm_crtc_vblank_get() fails and triggers the WARN_ON. The event is armed but never signaled because the disabled hardware generates no interrupts. Should the driver consume the event directly here (e.g., using drm_crtc_send_vblank_event) if vp->enabled is false to prevent the compositor from blocking indefinitely? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
