[PATCH 0/3] media: verisilicon: fix VEPU1 JPEG encoder writes past the CAPTURE buffer

[PATCH 0/3] media: verisilicon: fix VEPU1 JPEG encoder writes past the CAPTURE buffer

[Sergei Khomenkov via B4 Relay]

The H1 JPEG encoder is given its stream buffer limit in bytes, but the
register counts 64-bit words, so a stream that does not fit the CAPTURE
buffer is written past its end, up to eight times the buffer size. A
quality 100 frame of noise does not fit the default sizeimage, and
userspace can also set a smaller one, so any client of the encoder node
can get there. On an RK3026, which has no IOMMU in front of the VPU, the
encoder wrote 230 KB past a default-sized 640x480 buffer and 1.6 MB
past a 1920x1088 one.

Patch 2 writes the limit in the right unit, rounded down to the 64-byte
write bursts measured on the RK3026. With it, a frame that does not fit
ends with a buffer-full interrupt, which the driver turns into an ERROR
buffer but which leaves the encoder paused, so the following jobs time
out. Patch 1, which goes first, stops the VEPU1 encoder after such an
interrupt. The encoder still writes one burst with a limit of 0, and the
driver writes the 624-byte header itself, so patch 3 makes the smallest
CAPTURE sizeimage the header plus one burst.

All three were tested on the RK3026 (VEPU1) only; the test paragraph of
each patch has the numbers. v4l2-compliance 1.30.1 on the encoder node
shows no new failures with the series; its one failure (the raw OUTPUT
format's colorspace is not SRGB, v4l2-test-formats.cpp:892) fails
without it as well. The VEPU2 JPEG encoder (rockchip_vpu2_hw_jpeg_enc.c,
rockchip_vpu2_vepu_irq()) has the same code and the same limit in bytes,
and the STM32MP25 VENC runs the H1 code that patch 2 changes but has its
own interrupt handler without the stop of patch 1. I cannot test either,
so this series leaves the VEPU2 and the STM32MP25 handler alone; a check
there with a quality 100 noise frame and a default-sized CAPTURE buffer
would be welcome, and the same change can follow.

Not changed here: after a watchdog timeout, rockchip_vpu1_enc_reset()
sets H1_REG_INTERRUPT_DIS_BIT and the JPEG encoder path never clears
it, so every later job times out until the module is reloaded. That
can be a follow-up.

Before v5.18 (dc8bfe1e482d, "media: hantro: output encoded JPEG
content directly to capture buffers") the stream went to a bounce
buffer; patches 2 and 3 do not apply there and those trees would need a
separate backport.

The bugs were found and the patches drafted with an LLM coding
assistant while porting mainline Linux to an RK3026 board; every claim
in the messages was checked against the code or measured on the board.

---
Sergei Khomenkov (3):
      media: verisilicon: stop the VEPU1 JPEG encoder after an unfinished frame
      media: verisilicon: program the H1 JPEG stream limit in 64-bit words
      media: verisilicon: leave room for the JPEG header and one stream burst

 drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c | 10 +++++++++-
 drivers/media/platform/verisilicon/hantro_v4l2.c        | 10 ++++++++++
 drivers/media/platform/verisilicon/rockchip_vpu_hw.c    |  9 +++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261106-hantro-buflimit-055e8029be23

Best regards,
-- 
Sergei Khomenkov <shomenkow@gmail.com>

[PATCH 1/3] media: verisilicon: stop the VEPU1 JPEG encoder after an unfinished frame

[Sergei Khomenkov via B4 Relay]

From: Sergei Khomenkov <shomenkow@gmail.com>

When a JPEG stream does not fit the CAPTURE buffer, the H1 raises an
interrupt with BUFFER_FULL set and FRAME_RDY clear, and pauses with the
enable bit of H1_REG_ENC_CTRL still set, waiting for more output space.
rockchip_vpu1_vepu_irq() returns the buffer with the ERROR flag but
clears only the interrupt and AXI registers, so the next job is
programmed into the paused encoder: its ENC_CTRL write aborts the paused
frame, which raises a reset interrupt while the new job is being set up,
and that job times out. The watchdog then calls
rockchip_vpu1_enc_reset(), which sets H1_REG_INTERRUPT_DIS_BIT, and the
JPEG encoder path never clears that bit again, so every later job times
out as well. That second problem is not addressed here.

Write 0 to the enable register when an interrupt does not report a
finished frame. The write comes before the interrupt and AXI registers
are cleared, and before the next job is scheduled. On the RK3026 it
raises no interrupt of its own: in the test below the encoder raised one
interrupt per frame and no job timed out.

The encoder rarely gets there today, because the stream limit it is
given is eight times too large; the next patch fixes that, and then
every frame that does not fit ends this way.

rockchip_vpu2_vepu_irq() and stm32mp25_venc_irq() do not stop the
encoder either. I cannot test the VEPU2 or the STM32MP25, so this patch
leaves them alone.

Found with an LLM coding assistant while porting mainline Linux to a
Rockchip RK3026 board (VEPU1); the assistant also drafted the code and
this message. Tested on that board with the stream limit fixed: without
this change the job after a buffer-full interrupt gets a reset interrupt
and every later job times out; with it the frame that does not fit comes
back with the ERROR flag and the following jobs, overflowing or not,
complete as before (10 alternating jobs, and four overflowing frames
queued back to back followed by fitting ones). Counted in
/proc/interrupts over 24 runs of the series (48 frames, 16 of them
ending in a buffer full), every frame raised exactly one interrupt, so
the stop write raises none. Only buffer full was exercised, not bus
errors or the hardware timeout. Not tested on other SoCs.

Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sergei Khomenkov <shomenkow@gmail.com>
---
 drivers/media/platform/verisilicon/rockchip_vpu_hw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
index 02673be9878e..144d320302a1 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu_hw.c
@@ -376,6 +376,15 @@ static irqreturn_t rockchip_vpu1_vepu_irq(int irq, void *dev_id)
 	state = (status & H1_REG_INTERRUPT_FRAME_RDY) ?
 		VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
 
+	/*
+	 * An interrupt without FRAME_RDY does not end the frame: after a
+	 * buffer full the encoder pauses with its enable bit still set,
+	 * waiting for more output space. Stop it, so that the next job is
+	 * not programmed into a paused encoder.
+	 */
+	if (!(status & H1_REG_INTERRUPT_FRAME_RDY))
+		vepu_write(vpu, 0, H1_REG_ENC_CTRL);
+
 	vepu_write(vpu, 0, H1_REG_INTERRUPT);
 	vepu_write(vpu, 0, H1_REG_AXI_CTRL);
 

-- 
2.53.0

[PATCH 2/3] media: verisilicon: program the H1 JPEG stream limit in 64-bit words

[Sergei Khomenkov via B4 Relay]

From: Sergei Khomenkov <shomenkow@gmail.com>

The H1 JPEG encoder takes the stream buffer limit in 64-bit words. The
same register reads back the stream size in bits when the frame is
done, which hantro_h1_jpeg_enc_done() already divides by 8. The driver
writes the room left in the CAPTURE plane in bytes, so the hardware may
write up to eight times the buffer size, and a stream that does not fit
runs past the end of the plane into whatever memory follows it.

Userspace can get there. hantro_try_fmt() keeps any non-zero CAPTURE
sizeimage, and even the default of header_size + width * height * 2 is
not a bound: at quality 100 a high-entropy frame takes up to about 2.75
bytes per pixel. On a 640x480 frame the encoder wrote 230 KB past the
end of a default-sized buffer, on a 1920x1088 frame 1.6 MB. Without an
IOMMU in front of the encoder that is memory owned by someone else.

Write the limit in 64-bit words, rounded down to whole 64-byte bursts:
on the RK3026 the encoder writes the stream in 64-byte bursts and
finishes a burst it has started, so a limit that is not a multiple of 8
words lets it write up to 56 bytes past it. Rockchip's MPP library
programs this register the same way for H.264, in 64-bit words rounded
down to a multiple of 8 (mpp/hal/vpu/h264e/hal_h264e_vepu1_v2.c). The
stream starts header_size (624) bytes into the plane, so its address
stays 8-byte aligned. A stream that does not fit now stops at the limit
with a buffer-full interrupt, and the frame comes back with the ERROR
flag. A plane with less than one burst after the header is left to the
next patch.

The STM32MP25 VENC uses the same code, so it gets this change too. I
cannot test it there, and its interrupt handler does not get the stop
of the previous patch. rockchip_vpu2_jpeg_enc_set_buffers() writes
VEPU_REG_STR_BUF_LIMIT in bytes the same way, and
rockchip_vpu2_jpeg_enc_done() divides the read-back by 8 as well; I
cannot test the VEPU2 either, so this patch leaves it alone.

Found with an LLM coding assistant while porting mainline Linux to a
Rockchip RK3026 board (VEPU1); the assistant also drafted the code and
this message. Tested on that board, with CAPTURE buffers imported
through DMABUF from a larger buffer filled with a pattern: without this
change the encoder wrote the rest of the stream past the end of the
plane in each of 16 overflowing frames (640x480 to 1920x1088, quality
100, 12 of them in default-sized planes). With this and the previous
patch, 14 overflowing frames (640x480 and 1920x1088, four of them queued
back to back) stopped exactly at the limit with BUFFER_FULL in the
status, nothing past the plane changed, and frames that fit are byte for
byte the same as before. With the limit in words but not rounded down to
a burst, the encoder wrote up to 56 bytes past it. Not tested on other
SoCs.

Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sergei Khomenkov <shomenkow@gmail.com>
---
 drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c b/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
index 86cc1a07026f..1e548e91612b 100644
--- a/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
+++ b/drivers/media/platform/verisilicon/hantro_h1_jpeg_enc.c
@@ -55,7 +55,15 @@ static void hantro_h1_jpeg_enc_set_buffers(struct hantro_dev *vpu,
 	vepu_write_relaxed(vpu, vb2_dma_contig_plane_dma_addr(dst_buf, 0) +
 				ctx->vpu_dst_fmt->header_size,
 			   H1_REG_ADDR_OUTPUT_STREAM);
-	vepu_write_relaxed(vpu, size_left, H1_REG_STR_BUF_LIMIT);
+	/*
+	 * The stream buffer limit is counted in 64-bit words, not in bytes
+	 * (the register reads back the stream size in bits once the frame is
+	 * done). On the RK3026 the encoder writes the stream in 64-byte
+	 * bursts and finishes a burst it has started, so give it whole
+	 * bursts only.
+	 */
+	vepu_write_relaxed(vpu, round_down(size_left, 64) / 8,
+			   H1_REG_STR_BUF_LIMIT);
 
 	if (pix_fmt->num_planes == 1) {
 		src[0] = vb2_dma_contig_plane_dma_addr(src_buf, 0);

-- 
2.53.0

[PATCH 3/3] media: verisilicon: leave room for the JPEG header and one stream burst

[Sergei Khomenkov via B4 Relay]

From: Sergei Khomenkov <shomenkow@gmail.com>

hantro_try_fmt() keeps any non-zero sizeimage on the JPEG encoder's
CAPTURE queue, however small, and hantro_buf_prepare() then accepts a
plane of that size. hantro_jpeg_header_assemble() writes the 624-byte
header into the plane with the CPU before the hardware starts, and a
plane smaller than the header also makes the WARN_ON() in
hantro_h1_jpeg_enc_set_buffers() and
rockchip_vpu2_jpeg_enc_set_buffers() fire on every job, which userspace
can trigger at will. After the header the encoder writes its stream in
bursts, one of them even when the stream limit is 0; on the RK3026
VEPU1 a burst is 64 bytes. A plane smaller than the header plus 64 bytes
is therefore written past its end.

Raise a smaller request to header_size + 64, so that every plane the
driver accepts holds the header and at least one such burst.

Found with an LLM coding assistant while porting mainline Linux to a
Rockchip RK3026 board (VEPU1); the assistant also drafted the code and
this message. Tested on that board: a 640x480 CAPTURE format set with
sizeimage 1, 600, 656 and 689 reads back 688, 688, 688 and 689, a
656-byte DMABUF plane is then refused with EINVAL, and frames that do
not fit a 688-byte plane stop exactly at its end and come back with the
ERROR flag. A request of 600, now raised to 688, encodes with no
warning; before this change it was kept, and the encode hit the
WARN_ON() at hantro_h1_jpeg_enc.c:50 (hantro_h1_jpeg_enc_set_buffers(),
reported in hantro_h1_jpeg_enc_run()). Also before it, with 32 and 8
bytes after the header (a stream limit of 0), the encoder wrote one
burst, 32 and 56 bytes past the plane. The burst size is measured there
only; not tested on other SoCs.

Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sergei Khomenkov <shomenkow@gmail.com>
---
 drivers/media/platform/verisilicon/hantro_v4l2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 83af9fa1ce94..cc35beac8338 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -394,6 +394,16 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx,
 		 */
 		pix_mp->plane_fmt[0].sizeimage = fmt->header_size +
 			pix_mp->width * pix_mp->height * fmt->max_depth;
+	} else if (fmt->codec_mode == HANTRO_MODE_JPEG_ENC) {
+		/*
+		 * The JPEG encoder writes the header with the CPU, then the
+		 * stream in bursts, one of them even when the stream limit
+		 * is 0. Leave room for both; a burst is 64 bytes on the
+		 * RK3026 VEPU1.
+		 */
+		pix_mp->plane_fmt[0].sizeimage =
+			max_t(u32, pix_mp->plane_fmt[0].sizeimage,
+			      fmt->header_size + 64);
 	}
 
 	return 0;

-- 
2.53.0