Re: [PATCH RFC 11/25] drm/rockchip: vop2: Enable VRR

[sashiko-bot@kernel.org]

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 <nicolas.frattaroli@collabora.com>
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/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=11