Re: [PATCH v6 16/28] media: rockchip: rga: reuse cmdbuf contents

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Reducing the scope of `rga->ctrl_lock` in `device_run()` introduces a critical data race with concurrent V4L2 control updates, leading to inconsistent hardware command buffers and potential Out-Of-Bounds (OOB) DMA writes.
- [High] The newly introduced `ctx->cmdbuf_dirty` flag is updated in `vidioc_s_selection()` without holding `rga->ctrl_lock`, leading to lost updates and stale hardware configurations.
--

commit 9452d2457b5f041e524a98332bf069eeaf1ac9a6
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: reuse cmdbuf contents

This commit optimizes the command buffer programming by reusing its contents
instead of rewriting it completely for every frame. It introduces a
cmdbuf_dirty flag to track when rewriting is necessary.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -38,6 +38,11 @@ static void device_run(void *prv)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&rga->ctrl_lock, flags);
> +	if (ctx->cmdbuf_dirty) {
> +		ctx->cmdbuf_dirty = false;
> +		rga->hw->setup_cmdbuf(ctx);
> +	}
> +	spin_unlock_irqrestore(&rga->ctrl_lock, flags);
>  
>  	rga->curr = ctx;
>  
> @@ -47,8 +52,6 @@ static void device_run(void *prv)
>  	dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  
>  	rga->hw->start(rga, vb_to_rga(src), vb_to_rga(dst));
> -
> -	spin_unlock_irqrestore(&rga->ctrl_lock, flags);
>  }

Can dropping rga->ctrl_lock before calling rga->hw->start() introduce a
data race with concurrent V4L2 control updates?

If VIDIOC_S_CTRL executes concurrently, rga_s_ctrl() modifies ctx->vflip,
ctx->hflip, and ctx->rotate under ctrl_lock.

Since rga->hw->start() calls rga_cmd_set() which then calls
rga_cmd_set_dst_info(), it reads these same control values to calculate the
destination DMA base address without holding the lock.

Could this result in setup_cmdbuf() programming the core hardware
transformation settings with old control values, while
rga_cmd_set_dst_info() uses new control values? If a flip control is
toggled during this window, might the hardware execute a normal copy but
start from a flipped destination offset, writing outside the allocated
boundaries of the DMA buffer?

[ ... ]

> @@ -564,6 +570,7 @@ static int vidioc_s_selection(struct file *file, void *priv,
>  	}
>  
>  	f->crop = s->r;
> +	ctx->cmdbuf_dirty = true;
>  
>  	return ret;
>  }

Does setting ctx->cmdbuf_dirty without holding rga->ctrl_lock lead to lost
updates?

Unlike vidioc_s_fmt(), vidioc_s_selection() does not appear to check if the
queue is busy, which means it could execute concurrently with active
streaming and the M2M worker thread running device_run().

If vidioc_s_selection() sets the flag at the same moment device_run()
clears it, could the update be lost?

Additionally, if f->crop is updated between device_run() calling
setup_cmdbuf() and rga_cmd_set_dst_info(), could the hardware be
programmed with mismatched active size and DMA destination addresses,
potentially causing out-of-bounds DMA writes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-spu-rga3-v6-0-e547152eb9c9@pengutronix.de?part=16