Re: [PATCH 5/8] drm/rockchip: vop2: Report POST_BUF_EMPTY at most once per frame

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] drm/rockchip: vop2: stale FS status in
  vop2_crtc_handle_post_buf_empty()
- [Medium] drm/rockchip: vop2: VP_INT_FS_FIELD left enabled across CRTC
  disable
- [Low] drm/rockchip: vop2: vop2_crtc_enable_vblank() clears pending FS
  irq
- [Low] drm/rockchip: vop2: missing Fixes trailer for POST_BUF_EMPTY irq
  storm
--

commit 225fca8d582460d4af5df41d587daa2368d818f1
Author: Arun Iyengar <aiyengar1990@gmail.com>

> drm/rockchip: vop2: Report POST_BUF_EMPTY at most once per frame
>
> POST_BUF_EMPTY is raised for every line on which the post processing
> stage of a video port finds no data. The interrupt handler only
> rate-limits the error message and leaves the interrupt enabled, so an
> underflowing video port keeps the CPU in the interrupt handler 50000 to
> 300000 times per second, for as long as the configuration that causes
> the underflow stays in place (e.g. 120000 interrupts per second for
> 3840x2160@120 with two full screen layers on one AXI port).
>
> Mask POST_BUF_EMPTY when it fires and unmask it at the next frame start.
> The frame start (FS) interrupt is enabled for that frame even when
> vblank reporting is off, and disabled again once POST_BUF_EMPTY is
> re-armed. A new per video port lock serialises the interrupt enable
> bookkeeping between the interrupt handler and the vblank callbacks.
> The error is still reported, now at most once per frame and video port.
>
> Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>

[Severity: Low]
This fixes a per-line interrupt storm on underflowing video ports.  Should
this include:

  Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 9aeb90c6aa4b1..3323e50224c55 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -841,6 +841,48 @@ static void vop2_crtc_disable_irq(struct vop2_video_port *vp, u32 irq)
>  	vop2_writel(vop2, RK3568_VP_INT_EN(vp->id), irq << 16);
>  }
>  
> +/*
> + * POST_BUF_EMPTY is raised for every line the post processing stage finds
> + * no data for, so an underflowing video port can raise it 100000 times per
> + * second. Report it at most once per frame: mask it when it fires and
> + * unmask it at the next frame start, using the FS interrupt even when
> + * vblank reporting is off.
> + */
> +static void vop2_crtc_enable_post_buf_empty_irq(struct vop2_video_port *vp)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vp->irq_lock, flags);
> +	vp->post_buf_empty_masked = false;
> +	vop2_crtc_enable_irq(vp, VP_INT_POST_BUF_EMPTY);
> +	spin_unlock_irqrestore(&vp->irq_lock, flags);
> +}

[Severity: Medium]
Can VP_INT_FS_FIELD stay enabled permanently after a CRTC disable and
re-enable cycle?

If VP_INT_POST_BUF_EMPTY fires during the last frame of a video port
(either before drm_crtc_vblank_off() or while vop2_crtc_atomic_disable()
waits on vp->dsp_hold_completion), vop2_crtc_handle_post_buf_empty() sets
vp->post_buf_empty_masked = true and enables VP_INT_FS_FIELD.
drm_crtc_vblank_off() -> vop2_crtc_disable_vblank() then sees
vp->post_buf_empty_masked == true and leaves VP_INT_FS_FIELD enabled in
RK3568_VP_INT_EN(vp->id) when the video port enters standby.

When the CRTC is later re-enabled (while another active video port kept
VOP2 powered on), vop2_crtc_enable_post_buf_empty_irq() resets
vp->post_buf_empty_masked = false without disabling VP_INT_FS_FIELD.  Once
vblank turns off, the video port runs with both vp->vblank_enabled and
vp->post_buf_empty_masked false while VP_INT_FS_FIELD fires an unhandled
interrupt on every frame.  In addition, vop2_crtc_atomic_disable() and the
err_put_enable_count error paths in vop2_crtc_atomic_enable() never disable
VP_INT_POST_BUF_EMPTY.

> +
> +static void vop2_crtc_handle_post_buf_empty(struct vop2_video_port *vp)
> +{
> +	struct vop2 *vop2 = vp->vop2;
> +
> +	spin_lock(&vp->irq_lock);
> +	vop2_crtc_disable_irq(vp, VP_INT_POST_BUF_EMPTY);
> +	vp->post_buf_empty_masked = true;
> +	/* don't clear a pending FS, it may carry a vblank event */
> +	vop2_writel(vop2, RK3568_VP_INT_EN(vp->id),
> +		    VP_INT_FS_FIELD << 16 | VP_INT_FS_FIELD);
> +	spin_unlock(&vp->irq_lock);
> +}

[Severity: Medium]
When vp->vblank_enabled is false, does unmasking VP_INT_FS_FIELD without
clearing RK3568_VP_INT_CLR(vp->id) immediately trigger a spurious mid-frame
FS interrupt?

VOP2 latches raw interrupt status even while an interrupt is masked, and
vop2_isr() / rk3576_vp_isr() only clear bits returned by the masked status
register RK3568_VP_INT_STATUS(vp->id).  While vblank reporting is disabled,
the raw VP_INT_FS_FIELD bit latched at the start of the current frame stays
set during active scanout.  Writing RK3568_VP_INT_EN(vp->id) here without
clearing RK3568_VP_INT_CLR(vp->id) causes VP_INT_FS_FIELD to fire
immediately on that stale status and call vop2_crtc_rearm_post_buf_empty()
mid-frame, allowing VP_INT_POST_BUF_EMPTY to fire a second time in the same
frame.

When vp->vblank_enabled is true, VP_INT_FS_FIELD is already enabled; when
!vp->vblank_enabled, calling vop2_crtc_enable_irq(vp, VP_INT_FS_FIELD) will
clear the stale raw status before enabling it.

[ ... ]

> @@ -1668,8 +1710,12 @@ static const struct drm_plane_funcs vop2_plane_funcs = {
>  static int vop2_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct vop2_video_port *vp = to_vop2_video_port(crtc);
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&vp->irq_lock, flags);
> +	vp->vblank_enabled = true;
>  	vop2_crtc_enable_irq(vp, VP_INT_FS_FIELD);
> +	spin_unlock_irqrestore(&vp->irq_lock, flags);
>  
>  	return 0;
>  }

[Severity: Low]
Can vop2_crtc_enable_vblank() clear a pending VP_INT_FS_FIELD interrupt if
vp->post_buf_empty_masked is already true?

When vp->post_buf_empty_masked is true, VP_INT_FS_FIELD is already enabled
in hardware.  Calling vop2_crtc_enable_irq(vp, VP_INT_FS_FIELD)
unconditionally writes RK3568_VP_INT_CLR(vp->id), which can drop a pending
frame-start interrupt that raced with vop2_crtc_enable_vblank() and delay
both vblank handling and vop2_crtc_rearm_post_buf_empty() by a frame.
Guard the vop2_crtc_enable_irq() call with if (!vp->post_buf_empty_masked),
matching vop2_crtc_disable_vblank().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-vop2-fixes-v1-0-d336e9dd90d3@gmail.com?part=5