Re: [PATCH v6 27/28] media: rockchip: rga: add rga3 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Kernel Panic on Shared IRQ due to missing PM Runtime active check
- [High] DMA API Violation and Out-of-bounds Cache Sync on Coherent Memory
- [High] Silent API Failure of Crop Coordinates
- [Medium] Hardware Error Interrupts Masked
--

commit 71c16e32b64b60b7b83f81a3d0e8a736dfa30182
Author: Sven Püschel <s.pueschel@pengutronix.de>
Subject: media: rockchip: rga: add rga3 support

Add support for the RGA3 unit contained in the RK3588. Only a basic
feature set consisting of scaling and color conversion is implemented.

> diff --git a/drivers/media/platform/rockchip/rga/rga3-hw.c b/drivers/media/platform/rockchip/rga/rga3-hw.c
> new file mode 100644
> index 0000000000000..ca1c268303dd4
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/rga/rga3-hw.c

[ ... ]

> +static void rga3_cmd_set_win0_addr(struct rga_ctx *ctx,
> +				   const struct rga_addrs *addrs)
> +{
> +	u32 *cmd = ctx->cmdbuf_virt;
> +	unsigned int reg;
> +
> +	reg = RGA3_WIN0_Y_BASE - RGA3_FIRST_CMD_REG;
> +	cmd[reg >> 2] = addrs->y_addr;
> +	reg = RGA3_WIN0_U_BASE - RGA3_FIRST_CMD_REG;
> +	cmd[reg >> 2] = addrs->u_addr;
> +}

Does this code ignore the crop coordinates (ctx->in.crop.top and
ctx->in.crop.left)?

In the existing RGA2 implementation, offsets are applied using
rga_get_corner_addrs(). Here, the unadjusted buffer base addresses are
used. If userspace sets a crop selection with non-zero top/left offsets,
the API call will succeed but the hardware will process the image starting
from (0, 0).

[ ... ]

> +static void rga3_hw_start(struct rockchip_rga *rga,
> +			  struct rga_vb_buffer *src, struct rga_vb_buffer *dst)
> +{
> +	struct rga_ctx *ctx = rga->curr;
> +
> +	rga3_cmd_set_win0_addr(ctx, &src->dma_addrs);
> +	rga3_cmd_set_wr_addr(ctx, &dst->dma_addrs);
> +
> +	rga_write(rga, RGA3_CMD_ADDR, ctx->cmdbuf_phy);
> +
> +	/* sync CMD buf for RGA */
> +	dma_sync_single_for_device(rga->dev, ctx->cmdbuf_phy,
> +				   PAGE_SIZE, DMA_BIDIRECTIONAL);

Is it safe to use dma_sync_single_for_device() on ctx->cmdbuf_phy?

The command buffer is allocated via dma_alloc_attrs() with
DMA_ATTR_WRITE_COMBINE, which provides coherent memory. The DMA API strictly
forbids using streaming sync functions on coherent or write-combined memory,
which can cause kernel panics or warnings when CONFIG_DMA_API_DEBUG is enabled.

Additionally, the sync size is hardcoded to PAGE_SIZE (4096 bytes), but
RGA3_CMDBUF_SIZE is only 184 bytes. Could this cause an out-of-bounds cache
sync and corrupt memory if SWIOTLB bounce buffers are used? Should a standard
memory write barrier (wmb()) be used instead?

> +	/* set to master mode and start the conversion */
> +	rga_write(rga, RGA3_SYS_CTRL,
> +		  FIELD_PREP(RGA3_CMD_MODE, RGA3_CMD_MODE_MASTER));
> +	rga_write(rga, RGA3_INT_EN, FIELD_PREP(RGA3_INT_FRM_DONE, 1));

Are hardware error interrupts masked here?

RGA3_INT_EN is programmed with only RGA3_INT_FRM_DONE. If the hardware
encounters an error (like RGA3_INT_WR_BUS_ERR), it will halt but fail to
assert the interrupt. This would leave the V4L2 m2m framework waiting
indefinitely for buffer completion.

> +	rga_write(rga, RGA3_CMD_CTRL,
> +		  FIELD_PREP(RGA3_CMD_LINE_START_PULSE, 1));
> +}
> +
> +static bool rga3_handle_irq(struct rockchip_rga *rga)
> +{
> +	u32 intr;
> +
> +	intr = rga_read(rga, RGA3_INT_RAW);

Is it possible for this register read to occur while the device is
runtime-suspended?

Since RGA3 sets .has_internal_iommu to false, the interrupt is requested with
IRQF_SHARED. If another device on the shared line triggers an interrupt while
the RGA3 clocks are disabled, this register read may return 0xffffffff.

Since FIELD_GET(RGA3_INT_FRM_DONE, 0xffffffff) is true, the ISR would process
a spurious frame. When no job is active, rga->curr is NULL, which could lead
to a crash when dereferencing the context.

To comply with PM Runtime guidelines, should the IRQ handler use
pm_runtime_get_if_active() before accessing registers, and should the driver
call synchronize_irq() in the runtime suspend callback?

> +	/* clear all interrupts */
> +	rga_write(rga, RGA3_INT_CLR, intr);
> +
> +	return FIELD_GET(RGA3_INT_FRM_DONE, intr);
> +}

If error interrupts are enabled to prevent job hangs, would returning false
here cause the core ISR to ignore the error interrupt instead of aborting the
job?

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