Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/rockchip: vop2: VRR stretching breaks on commits without events - [High] drm/rockchip: vop2: VRR functionality missing in RK3576 ISR - [High] drm/rockchip: vop2: Cannot disable VRR due to vrr_enabled guard - [High] drm/rockchip: vop2: vop2_vp_set_vrr_vtotal fails to restore base vtotal --
commit dc9e1b8fad941cfe2c30467e7788b4889403d827 Author: Nicolas Frattaroli <[email protected]> Date: Mon Sep 21 15:51:37 2026 +0000 drm/rockchip: vop2: Enable VRR Use the new VRR state tracking to handle VRR front porch adjustment. To avoid indenting too deeply, refactor the vblank handling into its own function while at it, where scope-based cleanup helpers for the lock can be used. > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index bfb3841425687..e79081c044378 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -2011,6 +2012,25 @@ static void vop2_crtc_atomic_begin(struct drm_crtc > *crtc, > vop2->ops->setup_overlay(vp); > } > > +static void vop2_vp_set_vrr_vtotal(struct vop2_video_port *vp, unsigned int > vtotal) > +{ > + const u32 mask = RK3568_VP_DSP_VTOTAL_VS_END__DSP_VTOTAL | > + RK3568_VP_DSP_VTOTAL_VS_END__IMD; > + > + if (!vtotal) { > + vop2_vp_update_bits(vp, RK3568_VP_DSP_VTOTAL_VS_END, > + RK3568_VP_DSP_VTOTAL_VS_END__IMD, 0); > + return; > + } [Severity: High] When VRR is disabled, this early return clears the IMD bit but skips restoring the DSP_VTOTAL bits. Because RK3568_VP_DSP_VTOTAL_VS_END is also used for normal base timing, could leaving the DSP_VTOTAL field at its previous max_vtotal value corrupt the base display timing and leave the refresh rate permanently stretched? [ ... ] > @@ -2029,6 +2049,11 @@ static void vop2_crtc_atomic_flush(struct drm_crtc > *crtc, > > spin_lock_irq(&crtc->dev->event_lock); > > + if (crtc_state->vrr_enabled) { > + if (drm_crtc_helper_vrr_flush(crtc_state)) > + vop2_vp_set_vrr_vtotal(vp, > crtc_state->vrr_state.cur_vtotal); > + } [Severity: High] If userspace disables VRR, crtc_state->vrr_enabled will be false, and this condition will skip calling drm_crtc_helper_vrr_flush(). Since drm_crtc_helper_vrr_flush() is designed to handle the !vrr_enabled case and return a 0 vtotal to disable VRR in hardware, doesn't guarding it here trap the display hardware in VRR mode? > + > if (crtc->state->event) { > WARN_ON(drm_crtc_vblank_get(crtc)); [ ... ] > @@ -2371,6 +2396,30 @@ static irqreturn_t rk3576_vp_isr(int irq, void *data) > return ret; > } > > +static void vop2_vp_handle_vblank(struct vop2_video_port *vp) > +{ > + struct drm_crtc *crtc = &vp->crtc; > + struct vop2 *vop2 = vp->vop2; > + u32 val; > + > + guard(spinlock)(&crtc->dev->event_lock); > + > + if (!vp->event) > + return; [Severity: High] This early return bypasses the VRR vsync adjustment when no event is attached to the commit. If a compositor issues a commit without a vblank event (such as a cursor update), drm_crtc_helper_vrr_flush() shrinks vtotal to base_vtotal, but the subsequent vblank interrupt will hit this return and skip restoring the hardware vtotal to max_vtotal. Could this cause VRR frame stretching to fail for subsequent idle frames? > + > + val = vop2_readl(vop2, RK3568_REG_CFG_DONE); > + > + if (!(val & BIT(vp->id))) { > + drm_crtc_send_vblank_event(crtc, vp->event); > + vp->event = NULL; > + drm_crtc_vblank_put(crtc); > + > + if (crtc->state->vrr_enabled) > + if (drm_crtc_helper_vrr_vsync(crtc->state)) > + vop2_vp_set_vrr_vtotal(vp, > crtc->state->vrr_state.cur_vtotal); [Severity: High] Similar to the logic in atomic_flush, preventing drm_crtc_helper_vrr_vsync() from running when VRR is disabled might miss the transition update. Is it intentional to skip the vsync helper when vrr_enabled is false? > + } > +} > + > static irqreturn_t vop2_isr(int irq, void *data) > { > struct vop2 *vop2 = data; > @@ -2402,18 +2451,7 @@ static irqreturn_t vop2_isr(int irq, void *data) > > if (irqs & VP_INT_FS_FIELD) { > drm_crtc_handle_vblank(crtc); > - spin_lock(&crtc->dev->event_lock); > - if (vp->event) { > - u32 val = vop2_readl(vop2, > RK3568_REG_CFG_DONE); > - > - if (!(val & BIT(vp->id))) { > - > drm_crtc_send_vblank_event(crtc, vp->event); > - vp->event = NULL; > - drm_crtc_vblank_put(crtc); > - } > - } > - spin_unlock(&crtc->dev->event_lock); > - > + vop2_vp_handle_vblank(vp); > ret = IRQ_HANDLED; > } [Severity: High] This refactors vop2_isr() to use the new vop2_vp_handle_vblank() function which includes the VRR updates. Looking at rk3576_vp_isr() earlier in this file, it still uses the old inline vblank event handling and was not updated to use vop2_vp_handle_vblank(). Does this mean VRR functionality is completely missing on RK3576 platforms, as the hardware will never stretch frames without the vsync helper? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
