Re: [PATCH v2 6/9] 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 1 potential
issue to consider:
- [Medium] drm/rockchip: vop2: race in vop2_crtc_handle_post_buf_empty()
  on disable
--

commit 06ecad385f4f1417ba151eac3f81400c9be27660
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/rockchip: vop2: Report POST_BUF_EMPTY at most once per frame

This commit masks the POST_BUF_EMPTY interrupt when it fires and re-arms
it on the next frame start interrupt to avoid interrupt storms during
video port underflows.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 684fa46924e7f..c59e270739b66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -842,6 +842,70 @@ 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.
> + */
> +/* Forget a pending re-arm, called with vp->irq_lock held */
> +static void vop2_crtc_unmask_post_buf_empty(struct vop2_video_port *vp)
> +{
> +	if (vp->post_buf_empty_masked && !vp->vblank_enabled)
> +		vop2_crtc_disable_irq(vp, VP_INT_FS_FIELD);
> +	vp->post_buf_empty_masked = false;
> +}
> +
> +static void vop2_crtc_enable_post_buf_empty_irq(struct vop2_video_port *vp)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vp->irq_lock, flags);
> +	vop2_crtc_unmask_post_buf_empty(vp);
> +	vop2_crtc_enable_irq(vp, VP_INT_POST_BUF_EMPTY);
> +	spin_unlock_irqrestore(&vp->irq_lock, flags);
> +}
> +
> +static void vop2_crtc_disable_post_buf_empty_irq(struct vop2_video_port *vp)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vp->irq_lock, flags);
> +	vop2_crtc_disable_irq(vp, VP_INT_POST_BUF_EMPTY);
> +	vop2_crtc_unmask_post_buf_empty(vp);
> +	spin_unlock_irqrestore(&vp->irq_lock, flags);
> +}
> +
> +static void vop2_crtc_handle_post_buf_empty(struct vop2_video_port *vp)
> +{
> +	spin_lock(&vp->irq_lock);
> +	vop2_crtc_disable_irq(vp, VP_INT_POST_BUF_EMPTY);
> +	vp->post_buf_empty_masked = true;
> +	/*
> +	 * POST_BUF_EMPTY is unmasked again by the FS interrupt at the next
> +	 * frame start, so FS must be enabled. With vblank reporting on it
> +	 * is already, and a pending FS may carry a vblank event. Otherwise
> +	 * enable it here, clearing the FS status of the current frame so
> +	 * that FS fires at the next frame start only.
> +	 */
> +	if (!vp->vblank_enabled)
> +		vop2_crtc_enable_irq(vp, VP_INT_FS_FIELD);
> +	spin_unlock(&vp->irq_lock);
> +}

[Severity: Medium]
Can vop2_crtc_handle_post_buf_empty() run after
vop2_crtc_disable_post_buf_empty_irq() during CRTC disable and re-enable
VP_INT_FS_FIELD while VOP2 clocks are being turned off?

In vop2_crtc_atomic_disable(), VP_INT_POST_BUF_EMPTY stays enabled while
waiting on vp->dsp_hold_completion.  In both rk3576_vp_isr() and
vop2_isr(), VP_INT_STATUS is read once and complete(&vp->dsp_hold_completion)
runs before vop2_crtc_handle_post_buf_empty():

CPU0                                  CPU1 (<hardirq>)
vop2_crtc_atomic_disable()
  wait_for_completion_timeout(...)
                                      vop2_isr() / rk3576_vp_isr()
                                        irqs = vop2_readl(...);
                                        /* DSP_HOLD_VALID | POST_BUF_EMPTY */
                                        complete(&vp->dsp_hold_completion);
  vop2_crtc_disable_post_buf_empty_irq()
    spin_lock_irqsave(&vp->irq_lock)
    /* disables POST_BUF_EMPTY & FS */
    vp->post_buf_empty_masked = false;
    spin_unlock_irqrestore(&vp->irq_lock)
  vop2_disable()
    vop2_core_clks_disable_unprepare()
                                        vop2_crtc_handle_post_buf_empty()
                                          spin_lock(&vp->irq_lock)
                                          vp->post_buf_empty_masked = true;
                                          vop2_crtc_enable_irq(...,
                                                               VP_INT_FS_FIELD);
                                            <-- MMIO write with clocks off

Unlike vop2_crtc_rearm_post_buf_empty(), vop2_crtc_handle_post_buf_empty()
does not check whether POST_BUF_EMPTY reporting is still enabled on the
video port, so it leaves vp->post_buf_empty_masked set and VP_INT_FS_FIELD
enabled on a disabled CRTC, or performs unclocked MMIO writes if the last
CRTC disables VOP2 core clocks concurrently.

Disabling POST_BUF_EMPTY before waiting for dsp_hold_completion, or
tracking under vp->irq_lock whether POST_BUF_EMPTY is enabled so
vop2_crtc_handle_post_buf_empty() becomes a no-op once disabled, would
avoid this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-vop2-fixes-v2-0-8218c1cf8ac8@gmail.com?part=6