Re: [PATCH 5/7] media: verisilicon: Add Rockchip VPU720 JPEG decoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] DMA API violation and cache coherency bug when filling chroma plane for grayscale JPEGs.
- [High] Lockless Read-Modify-Write on VDPU720_REG_INT causes destructive race between hardware IRQ and watchdog soft reset.
- [Medium] Valid JPEG frames are erroneously rejected due to a strict 64-byte tail search for the EOI marker.
- [Medium] Missing endianness conversions when writing 16-bit table entries to the DMA side buffer.
--

--- Patch [5]: [PATCH 5/7] media: verisilicon: Add Rockchip VPU720 JPEG decoder ---
commit 8ca41ed2bd2cec7b6e2c467549362041d9356521
Author: Lucas Sinn <lucas.sinn@wolfvision.net>

media: verisilicon: Add Rockchip VPU720 JPEG decoder

This commit adds support for the Rockchip VPU720 JPEG hardware decoder on
the RK3588 SoC to the verisilicon hantro driver. It introduces the
HANTRO_JPEG_DECODER codec and mode, implementing header parsing, Q/H table
side buffer generation, and handling of VPU720 specific configuration limits.

> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu720_hw_jpeg_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu720_hw_jpeg_dec.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu720_hw_jpeg_dec.c
[ ... ]
> +static int vdpu720_write_qtbl(struct hantro_ctx *ctx,
> +			      const struct v4l2_jpeg_header *hdr)
> +{
[ ... ]
> +		/*
> +		 * Reorder zigzag → raster scan.
> +		 * v4l2_jpeg_zigzag_scan_index[z] = raster position of zigzag
> +		 * element z.  JPEG Q-tables are stored in zigzag order; the
> +		 * hardware expects them in natural raster (row-major) order.
> +		 */
> +		for (i = 0; i < VDPU720_QTBL_ENTRIES; i++)
> +			dst[v4l2_jpeg_zigzag_scan_index[i]] = (u16)qtbl[i];

[Severity: Medium]
Will this write the 16-bit table entries correctly on a Big Endian kernel?

It looks like the Q-table elements are written using native CPU endianness.
Since the VPU720 hardware expects the DMA buffer to be in Little Endian format,
could this result in decoding failures and visual corruption on Big Endian
systems? Using explicit endian conversions like cpu_to_le16() might be
necessary here.

[ ... ]
> +static int vdpu720_write_htbl(struct hantro_ctx *ctx,
> +			      const struct v4l2_jpeg_header *hdr)
> +{
[ ... ]
> +		vdpu720_compute_mincode(dc_bits, min_dc, acc_dc);
> +		vdpu720_compute_mincode(ac_bits, min_ac, acc_ac);
> +
> +		for (i = 0; i < 16; i++)
> +			*p_mincode++ = min_dc[i];

[Severity: Medium]
Similarly, the Huffman mincode table entries appear to be written with
native CPU endianness. Does this also require cpu_to_le16() for the same
Little Endian hardware requirement?

[ ... ]
> +static int vdpu720_fill_chroma(struct hantro_ctx *ctx,
> +			       struct vb2_v4l2_buffer *dst_buf)
> +{
[ ... ]
> +	/* Available because variant->dst_needs_kmap */
> +	dst_cpu = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> +	if (!dst_cpu) {
> +		dev_err(vpu->dev,
> +			"JPEG capture buffer has no kernel mapping\n");
> +		return -EINVAL;
> +	}
> +
> +	memset(dst_cpu + y_size, 0x80, size - y_size);

[Severity: High]
Can this memset result in data loss or corruption for the UV plane?

This memset modifies the CPU cache for a device-owned DMA buffer. Because it
is called from rockchip_vpu720_jpeg_dec_run() after the buffer has been
queued and synced for the device, it dirties the CPU cache without explicit
cache synchronization.

When the hardware completes its operations and the V4L2 core calls
dma_sync_sg_for_cpu() with DMA_FROM_DEVICE during DQBUF, the CPU cache is
unconditionally invalidated. Won't this destroy the CPU's memset writes and
leave the UV plane with uninitialized memory or stale data if non-coherent
DMABUFs are used?

[ ... ]
> +int rockchip_vpu720_jpeg_dec_run(struct hantro_ctx *ctx)
> +{
[ ... ]
> +	tail_len = min_t(u32, src_len, sizeof(tail));
> +	memcpy(tail, src_cpu + src_len - tail_len, tail_len);
> +
> +	for (i = 0; i + 1 < tail_len; i++)
> +		if (tail[i] == 0xff && tail[i + 1] == 0xd9)
> +			break;
> +
> +	if (i + 1 >= tail_len) {
> +		dev_err_ratelimited(vpu->dev,
> +				    "truncated JPEG, no EOI at the end of the %u byte payload (buffer too small?)\n",
> +				    src_len);
> +		ret = -EINVAL;
> +		goto err;
> +	}

[Severity: Medium]
Could this strict 64-byte tail search reject valid padded JPEG frames?

If a userspace application or upstream pipeline pads the JPEG payload
(for example, with page-aligned or container-aligned chunks) and includes
the padding in bytesused, the padding might easily exceed 62 bytes.

If the 0xff 0xd9 EOI marker falls outside the strictly checked 64-byte tail
buffer, it looks like valid frames will be rejected with -EINVAL. Could this
be made more robust by scanning backwards dynamically or parsing the stream
length instead?

[ ... ]
> +static int vdpu720_soft_reset(struct hantro_dev *vpu)
> +{
> +	u32 status;
> +	int ret;
> +
> +	/*
> +	 * If the decoder is idle (DEC_E=0), set FORCE_SOFTRESET_VALID
> +	 * before triggering the soft reset, per downstream BSP behaviour.
> +	 */
> +	status = vdpu_read(vpu, VDPU720_REG_INT);
> +	if (!(status & VDPU720_DEC_E))
> +		vdpu_write(vpu, VDPU720_FORCE_SOFTRST, VDPU720_REG_SYS);
> +
> +	vdpu_write(vpu, status | VDPU720_SOFT_RST_EN, VDPU720_REG_INT);

[Severity: High]
Is there a race condition here between the watchdog soft reset and the
hardware IRQ?

vdpu720_soft_reset() is executed from the hantro_watchdog() workqueue in
process context and performs an unprotected Read-Modify-Write (RMW) on
VDPU720_REG_INT. Concurrently, rockchip_vpu720_irq() can preempt this in
hardirq context and write to the same register:

rockchip_vpu720_irq() {
    ...
    vdpu_write(vpu, clr_mask, VDPU720_REG_INT);
    ...
    vdpu_write(vpu, 0, VDPU720_REG_INT);
}

If an untimely IRQ fires, it could clear VDPU720_SOFT_RST_EN or
VDPU720_SOFT_RST_RDY midway through the reset sequence. Won't this cause
readl_relaxed_poll_timeout() to fail and force an unnecessary hard reset,
or corrupt the hardware state? Should this sequence acquire vpu->irqlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rk3588-jpegdec-v1-0-33d74cdf369c@pengutronix.de?part=5