[PATCH 0/3] media: verisilicon: Fix error paths in hantro drivers

[PATCH 0/3] media: verisilicon: Fix error paths in hantro drivers

[Sascha Hauer]

This contains the Hantro error path patches previously sent as
part of "media: verisilicon: Add RK3588 VPU720 JPEG decoder" [1] as a
separate series. As the Rockchip JPEG decoder turned out to be no Hantro
compatible hardware at all I'll continue that as another series. This
series now contains the useful stuff from [1] with Nicolas comments to
the first patch addressed.

[1] https://lore.kernel.org/all/20260819-rk3588-jpegdec-v1-0-33d74cdf369c@pengutronix.de/

---
Sascha Hauer (3):
      media: verisilicon: Fix the cleanup when a codec ->run() fails
      media: verisilicon: Complete the request on the ->run() error paths
      media: verisilicon: Allow the EOS event to be subscribed

 drivers/media/platform/verisilicon/hantro_drv.c    | 35 ++++++++++++++++++++--
 .../platform/verisilicon/hantro_g1_h264_dec.c      |  4 ++-
 .../media/platform/verisilicon/hantro_g1_vp8_dec.c |  4 ++-
 .../platform/verisilicon/hantro_g2_hevc_dec.c      |  9 ++++--
 .../media/platform/verisilicon/hantro_g2_vp9_dec.c |  2 +-
 drivers/media/platform/verisilicon/hantro_hw.h     |  1 +
 drivers/media/platform/verisilicon/hantro_v4l2.c   | 13 +++++++-
 .../verisilicon/rockchip_vpu2_hw_h264_dec.c        |  4 ++-
 .../verisilicon/rockchip_vpu2_hw_jpeg_enc.c        |  4 ++-
 .../verisilicon/rockchip_vpu2_hw_vp8_dec.c         |  4 ++-
 .../verisilicon/rockchip_vpu981_hw_av1_dec.c       |  3 +-
 11 files changed, 70 insertions(+), 13 deletions(-)
---
base-commit: 0a0d1d55dad570724bf8c7ea83409639cfb4be9b
change-id: 20260819-rk3588-jpegdec-89e1cf4c8898

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 1/3] media: verisilicon: Fix the cleanup when a codec ->run() fails

[Sascha Hauer]

A codec ->run() operation that fails leaves three things behind, and they
cannot be untangled one at a time, so fix them together.

hantro_start_prepare_run() sets up the controls of the request attached
to the source buffer, hantro_end_prepare_run() completes them again and
arms the watchdog for the job that is about to be started. That pairing
does not survive the error paths. The two ->run() operations that do
reach hantro_end_prepare_run() arm a watchdog for a job that is never
started: device_run() finishes the job synchronously via
hantro_job_finish_no_pm() and nothing cancels the delayed work, so it
expires two seconds later and aborts whatever unrelated job happens to be
running by then.

Split the two jobs hantro_end_prepare_run() would otherwise have to do.
It keeps its meaning for the success path, complete the request and arm
the watchdog, and a new hantro_abort_prepare_run() completes the request
and nothing else for the error paths. Note the resulting invariant: after
hantro_end_prepare_run() the ->run() operation must return 0, as the
watchdog is armed and only the interrupt handler disarms it.

rockchip_vpu981_av1_dec_run() then calls hantro_irq_done() on its error
path and returns the error code to device_run(), which finishes the job a
second time. The buffers have already been given back by then, so the
second attempt trips the WARN_ON(!src) in hantro_job_finish_no_pm() and
bails out. Without the watchdog change this at least reached the first
finish by accident, because the cancel_delayed_work() in
hantro_irq_done() returned true for the watchdog the error path had just
armed. Neither behaviour is something to rely on, so drop the call and
let device_run() clean the job up. No other codec calls hantro_irq_done()
from ->run().

That leaves device_run() itself. It takes a pm_runtime reference and
enables the clocks, then on any subsequent failure jumps to a single
err_cancel_job label that calls hantro_job_finish_no_pm() - which
releases neither. Release the acquired resources there.

This last part is what ties the three together. hantro_irq_done() ends up
in hantro_job_finish(), which already drops the pm reference and disables
the clocks, so as long as the AV1 error path still goes through it,
releasing the same resources in device_run() would trip the

  WARN_ON(core->enable_count == 0)

in clk_core_disable() and underflow dev->power.usage_count. Conversely,
as soon as a failed job no longer arms the watchdog, hantro_irq_done()
stops releasing anything at all and the resources are leaked until
device_run() takes over.

The late_postproc setup is skipped on the error path as well. It is part
of preparing the run, the hardware is not started and the next job
configures it again, so hantro_abort_prepare_run() simply does not have
it. Only the sunxi variant sets late_postproc, and its only decoder is
VP9.

Fixes: 892bb6ecead9 ("media: hantro: do a PM resume earlier")
Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
Fixes: 727a400686a2 ("media: verisilicon: Add Rockchip AV1 decoder")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/media/platform/verisilicon/hantro_drv.c    | 35 ++++++++++++++++++++--
 .../media/platform/verisilicon/hantro_g2_vp9_dec.c |  2 +-
 drivers/media/platform/verisilicon/hantro_hw.h     |  1 +
 .../verisilicon/rockchip_vpu981_hw_av1_dec.c       |  3 +-
 4 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index ad71c0402ef3b..a9ebf856096e6 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -147,6 +147,18 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
 	}
 }
 
+/**
+ * hantro_end_prepare_run() - finish the preparation of a job and arm the
+ *			      watchdog
+ * @ctx:	context the job belongs to
+ *
+ * Complete the controls of the request that hantro_start_prepare_run() set up
+ * and arm the watchdog. The caller must go on and start the hardware, as only
+ * the interrupt handler disarms the watchdog again.
+ *
+ * A ->run() operation that gives up before the hardware is started must call
+ * hantro_abort_prepare_run() instead.
+ */
 void hantro_end_prepare_run(struct hantro_ctx *ctx)
 {
 	struct vb2_v4l2_buffer *src_buf;
@@ -167,6 +179,21 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
 			      msecs_to_jiffies(2000));
 }
 
+/**
+ * hantro_abort_prepare_run() - give up on a job before the hardware is started
+ * @ctx:	context the job belongs to
+ *
+ * Counterpart of hantro_end_prepare_run() for the error paths of ->run().
+ */
+void hantro_abort_prepare_run(struct hantro_ctx *ctx)
+{
+	struct vb2_v4l2_buffer *src_buf;
+
+	src_buf = hantro_get_src_buf(ctx);
+	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
+				   &ctx->ctrl_handler);
+}
+
 static void device_run(void *priv)
 {
 	struct hantro_ctx *ctx = priv;
@@ -182,15 +209,19 @@ static void device_run(void *priv)
 
 	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
 	if (ret)
-		goto err_cancel_job;
+		goto err_pm_put;
 
 	v4l2_m2m_buf_copy_metadata(src, dst);
 
 	if (ctx->codec_ops->run(ctx))
-		goto err_cancel_job;
+		goto err_clk_disable;
 
 	return;
 
+err_clk_disable:
+	clk_bulk_disable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
+err_pm_put:
+	pm_runtime_put_autosuspend(ctx->dev->dev);
 err_cancel_job:
 	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
 }
diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
index 56c79e339030e..78100d1c7e850 100644
--- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
+++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
@@ -895,7 +895,7 @@ int hantro_g2_vp9_dec_run(struct hantro_ctx *ctx)
 
 	ret = start_prepare_run(ctx, &decode_params);
 	if (ret) {
-		hantro_end_prepare_run(ctx);
+		hantro_abort_prepare_run(ctx);
 		return ret;
 	}
 
diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h
index 13e573f1f19de..c6addab4d758b 100644
--- a/drivers/media/platform/verisilicon/hantro_hw.h
+++ b/drivers/media/platform/verisilicon/hantro_hw.h
@@ -431,6 +431,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
 		     enum vb2_buffer_state result);
 void hantro_start_prepare_run(struct hantro_ctx *ctx);
 void hantro_end_prepare_run(struct hantro_ctx *ctx);
+void hantro_abort_prepare_run(struct hantro_ctx *ctx);
 
 irqreturn_t hantro_g1_irq(int irq, void *dev_id);
 void hantro_g1_reset(struct hantro_ctx *ctx);
diff --git a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
index e4e21ad373233..99ffb4a743764 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
@@ -2192,8 +2192,7 @@ int rockchip_vpu981_av1_dec_run(struct hantro_ctx *ctx)
 	return 0;
 
 prepare_error:
-	hantro_end_prepare_run(ctx);
-	hantro_irq_done(vpu, VB2_BUF_STATE_ERROR);
+	hantro_abort_prepare_run(ctx);
 	return ret;
 }
 

-- 
2.47.3


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 2/3] media: verisilicon: Complete the request on the ->run() error paths

[Sascha Hauer]

Several codec ->run() operations return early without completing the
request, even though hantro_start_prepare_run() has already run and set up
the controls of the request attached to the source buffer.

The buffers are still returned to userspace, device_run() finishes the job
with VB2_BUF_STATE_ERROR, but nothing completes the control handler object
bound to the media request. vb2_buffer_done() only unbinds the request
object owned by videobuf2 itself, so num_incomplete_objects never drops to
zero and the request stays in MEDIA_REQUEST_STATE_QUEUED forever: poll() on
the request file descriptor never returns and MEDIA_REQUEST_IOC_REINIT
fails with -EBUSY. The request is only cleaned up when userspace closes it.

The vb2 buf_request_complete() callback does not help here, it is only
called from __vb2_queue_cancel() for buffers that were never queued to the
driver.

Call hantro_abort_prepare_run() on those paths, which completes the request
and leaves the watchdog alone.

Fixes: 42cb2a8f27d2 ("media: hantro: change hantro_codec_ops run prototype to return errors")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/media/platform/verisilicon/hantro_g1_h264_dec.c        | 4 +++-
 drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c         | 4 +++-
 drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c        | 9 +++++++--
 drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c | 4 +++-
 drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c | 4 +++-
 drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c  | 4 +++-
 6 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
index ad5c1a6634f5c..14ebb25ea24be 100644
--- a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
+++ b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
@@ -255,8 +255,10 @@ int hantro_g1_h264_dec_run(struct hantro_ctx *ctx)
 
 	/* Prepare the H264 decoder context. */
 	ret = hantro_h264_dec_prepare_run(ctx);
-	if (ret)
+	if (ret) {
+		hantro_abort_prepare_run(ctx);
 		return ret;
+	}
 
 	/* Configure hardware registers. */
 	src_buf = hantro_get_src_buf(ctx);
diff --git a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
index 851eb67f19f50..e866ff86019a0 100644
--- a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
+++ b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
@@ -442,8 +442,10 @@ int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
 	hantro_start_prepare_run(ctx);
 
 	hdr = hantro_get_ctrl(ctx, V4L2_CID_STATELESS_VP8_FRAME);
-	if (WARN_ON(!hdr))
+	if (WARN_ON(!hdr)) {
+		hantro_abort_prepare_run(ctx);
 		return -EINVAL;
+	}
 
 	/* Reset segment_map buffer in keyframe */
 	if (V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) && ctx->vp8_dec.segment_map.cpu)
diff --git a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
index e8c2e83379def..5778813a9eb2f 100644
--- a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
+++ b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
@@ -596,7 +596,7 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
 	/* Prepare HEVC decoder context. */
 	ret = hantro_hevc_dec_prepare_run(ctx);
 	if (ret)
-		return ret;
+		goto abort_prepare_run;
 
 	/* Configure hardware registers. */
 	set_params(ctx);
@@ -604,7 +604,7 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
 	/* set reference pictures */
 	ret = set_ref(ctx);
 	if (ret)
-		return ret;
+		goto abort_prepare_run;
 
 	set_buffers(ctx);
 	prepare_tile_info_buffer(ctx);
@@ -634,4 +634,9 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
 	vdpu_write(vpu, G2_REG_INTERRUPT_DEC_E, G2_REG_INTERRUPT);
 
 	return 0;
+
+abort_prepare_run:
+	hantro_abort_prepare_run(ctx);
+
+	return ret;
 }
diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c
index 6da87f5184bcb..d412d5d661226 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c
@@ -473,8 +473,10 @@ int rockchip_vpu2_h264_dec_run(struct hantro_ctx *ctx)
 
 	/* Prepare the H264 decoder context. */
 	ret = hantro_h264_dec_prepare_run(ctx);
-	if (ret)
+	if (ret) {
+		hantro_abort_prepare_run(ctx);
 		return ret;
+	}
 
 	src_buf = hantro_get_src_buf(ctx);
 	set_params(ctx, src_buf);
diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
index 61621b1be8a2f..0ba078b9d1875 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
@@ -143,8 +143,10 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx)
 
 	memset(&jpeg_ctx, 0, sizeof(jpeg_ctx));
 	jpeg_ctx.buffer = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
-	if (!jpeg_ctx.buffer)
+	if (!jpeg_ctx.buffer) {
+		hantro_abort_prepare_run(ctx);
 		return -ENOMEM;
+	}
 
 	jpeg_ctx.width = ctx->dst_fmt.width;
 	jpeg_ctx.height = ctx->dst_fmt.height;
diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c
index d079075448c96..15f4872dd2bdd 100644
--- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c
+++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c
@@ -519,8 +519,10 @@ int rockchip_vpu2_vp8_dec_run(struct hantro_ctx *ctx)
 	hantro_start_prepare_run(ctx);
 
 	hdr = hantro_get_ctrl(ctx, V4L2_CID_STATELESS_VP8_FRAME);
-	if (WARN_ON(!hdr))
+	if (WARN_ON(!hdr)) {
+		hantro_abort_prepare_run(ctx);
 		return -EINVAL;
+	}
 
 	/* Reset segment_map buffer in keyframe */
 	if (V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) && ctx->vp8_dec.segment_map.cpu)

-- 
2.47.3


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 3/3] media: verisilicon: Allow the EOS event to be subscribed

[Sascha Hauer]

hantro queues a V4L2_EVENT_EOS in three places.  vidioc_encoder_cmd()
sends it when a V4L2_ENC_CMD_STOP has drained the encoder:

	if (ec->cmd == V4L2_ENC_CMD_STOP &&
	    v4l2_m2m_has_stopped(ctx->fh.m2m_ctx))
		v4l2_event_queue_fh(&ctx->fh, &hantro_eos_event);

hantro_buf_queue() sends it for a capture buffer queued after the last
one, and hantro_stop_streaming() when the output queue stops while
draining.

No application can ask for any of them.  The ioctl ops offer

	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,

and v4l2_ctrl_subscribe_event() only knows about V4L2_EVENT_CTRL, so
VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_EOS fails with -EINVAL and the
queued events are dropped on the floor.  An application following the
drain sequence in the stateful encoder documentation has to fall back to
V4L2_BUF_FLAG_LAST, which hantro does set, so this has gone unnoticed.

Dispatch on the event type and hand V4L2_EVENT_EOS to
v4l2_event_subscribe(), the way coda-common.c does.  Everything else
keeps going to the control handler.

V4L2_EVENT_SOURCE_CHANGE is deliberately not added, hantro never sends
one.

Fixes: daf3999c12dc ("media: hantro: Implement support for encoder commands")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/media/platform/verisilicon/hantro_v4l2.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
index 83af9fa1ce949..9e19daffe0075 100644
--- a/drivers/media/platform/verisilicon/hantro_v4l2.c
+++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
@@ -765,6 +765,17 @@ static int vidioc_encoder_cmd(struct file *file, void *priv,
 	return 0;
 }
 
+static int hantro_subscribe_event(struct v4l2_fh *fh,
+				  const struct v4l2_event_subscription *sub)
+{
+	switch (sub->type) {
+	case V4L2_EVENT_EOS:
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
+	default:
+		return v4l2_ctrl_subscribe_event(fh, sub);
+	}
+}
+
 const struct v4l2_ioctl_ops hantro_ioctl_ops = {
 	.vidioc_querycap = vidioc_querycap,
 	.vidioc_enum_framesizes = vidioc_enum_framesizes,
@@ -787,7 +798,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
 	.vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
 	.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
 
-	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
+	.vidioc_subscribe_event = hantro_subscribe_event,
 	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 
 	.vidioc_streamon = v4l2_m2m_ioctl_streamon,

-- 
2.47.3


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] media: verisilicon: Fix the cleanup when a codec ->run() fails

[Benjamin Gaignard]

Le 24/08/2026 à 08:39, Sascha Hauer a écrit :
> A codec ->run() operation that fails leaves three things behind, and they
> cannot be untangled one at a time, so fix them together.
>
> hantro_start_prepare_run() sets up the controls of the request attached
> to the source buffer, hantro_end_prepare_run() completes them again and
> arms the watchdog for the job that is about to be started. That pairing
> does not survive the error paths. The two ->run() operations that do
> reach hantro_end_prepare_run() arm a watchdog for a job that is never
> started: device_run() finishes the job synchronously via
> hantro_job_finish_no_pm() and nothing cancels the delayed work, so it
> expires two seconds later and aborts whatever unrelated job happens to be
> running by then.
>
> Split the two jobs hantro_end_prepare_run() would otherwise have to do.
> It keeps its meaning for the success path, complete the request and arm
> the watchdog, and a new hantro_abort_prepare_run() completes the request
> and nothing else for the error paths. Note the resulting invariant: after
> hantro_end_prepare_run() the ->run() operation must return 0, as the
> watchdog is armed and only the interrupt handler disarms it.
>
> rockchip_vpu981_av1_dec_run() then calls hantro_irq_done() on its error
> path and returns the error code to device_run(), which finishes the job a
> second time. The buffers have already been given back by then, so the
> second attempt trips the WARN_ON(!src) in hantro_job_finish_no_pm() and
> bails out. Without the watchdog change this at least reached the first
> finish by accident, because the cancel_delayed_work() in
> hantro_irq_done() returned true for the watchdog the error path had just
> armed. Neither behaviour is something to rely on, so drop the call and
> let device_run() clean the job up. No other codec calls hantro_irq_done()
> from ->run().
>
> That leaves device_run() itself. It takes a pm_runtime reference and
> enables the clocks, then on any subsequent failure jumps to a single
> err_cancel_job label that calls hantro_job_finish_no_pm() - which
> releases neither. Release the acquired resources there.
>
> This last part is what ties the three together. hantro_irq_done() ends up
> in hantro_job_finish(), which already drops the pm reference and disables
> the clocks, so as long as the AV1 error path still goes through it,
> releasing the same resources in device_run() would trip the
>
>    WARN_ON(core->enable_count == 0)
>
> in clk_core_disable() and underflow dev->power.usage_count. Conversely,
> as soon as a failed job no longer arms the watchdog, hantro_irq_done()
> stops releasing anything at all and the resources are leaked until
> device_run() takes over.
>
> The late_postproc setup is skipped on the error path as well. It is part
> of preparing the run, the hardware is not started and the next job
> configures it again, so hantro_abort_prepare_run() simply does not have
> it. Only the sunxi variant sets late_postproc, and its only decoder is
> VP9.
>
> Fixes: 892bb6ecead9 ("media: hantro: do a PM resume earlier")
> Fixes: e2da465455ce ("media: hantro: Support VP9 on the G2 core")
> Fixes: 727a400686a2 ("media: verisilicon: Add Rockchip AV1 decoder")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by Benjamin Gaignard <benjamin.gaignard@gmail.com>

> ---
>   drivers/media/platform/verisilicon/hantro_drv.c    | 35 ++++++++++++++++++++--
>   .../media/platform/verisilicon/hantro_g2_vp9_dec.c |  2 +-
>   drivers/media/platform/verisilicon/hantro_hw.h     |  1 +
>   .../verisilicon/rockchip_vpu981_hw_av1_dec.c       |  3 +-
>   4 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index ad71c0402ef3b..a9ebf856096e6 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -147,6 +147,18 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
>   	}
>   }
>   
> +/**
> + * hantro_end_prepare_run() - finish the preparation of a job and arm the
> + *			      watchdog
> + * @ctx:	context the job belongs to
> + *
> + * Complete the controls of the request that hantro_start_prepare_run() set up
> + * and arm the watchdog. The caller must go on and start the hardware, as only
> + * the interrupt handler disarms the watchdog again.
> + *
> + * A ->run() operation that gives up before the hardware is started must call
> + * hantro_abort_prepare_run() instead.
> + */
>   void hantro_end_prepare_run(struct hantro_ctx *ctx)
>   {
>   	struct vb2_v4l2_buffer *src_buf;
> @@ -167,6 +179,21 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>   			      msecs_to_jiffies(2000));
>   }
>   
> +/**
> + * hantro_abort_prepare_run() - give up on a job before the hardware is started
> + * @ctx:	context the job belongs to
> + *
> + * Counterpart of hantro_end_prepare_run() for the error paths of ->run().
> + */
> +void hantro_abort_prepare_run(struct hantro_ctx *ctx)
> +{
> +	struct vb2_v4l2_buffer *src_buf;
> +
> +	src_buf = hantro_get_src_buf(ctx);
> +	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> +				   &ctx->ctrl_handler);
> +}
> +
>   static void device_run(void *priv)
>   {
>   	struct hantro_ctx *ctx = priv;
> @@ -182,15 +209,19 @@ static void device_run(void *priv)
>   
>   	ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
>   	if (ret)
> -		goto err_cancel_job;
> +		goto err_pm_put;
>   
>   	v4l2_m2m_buf_copy_metadata(src, dst);
>   
>   	if (ctx->codec_ops->run(ctx))
> -		goto err_cancel_job;
> +		goto err_clk_disable;
>   
>   	return;
>   
> +err_clk_disable:
> +	clk_bulk_disable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> +err_pm_put:
> +	pm_runtime_put_autosuspend(ctx->dev->dev);
>   err_cancel_job:
>   	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
>   }
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> index 56c79e339030e..78100d1c7e850 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2_vp9_dec.c
> @@ -895,7 +895,7 @@ int hantro_g2_vp9_dec_run(struct hantro_ctx *ctx)
>   
>   	ret = start_prepare_run(ctx, &decode_params);
>   	if (ret) {
> -		hantro_end_prepare_run(ctx);
> +		hantro_abort_prepare_run(ctx);
>   		return ret;
>   	}
>   
> diff --git a/drivers/media/platform/verisilicon/hantro_hw.h b/drivers/media/platform/verisilicon/hantro_hw.h
> index 13e573f1f19de..c6addab4d758b 100644
> --- a/drivers/media/platform/verisilicon/hantro_hw.h
> +++ b/drivers/media/platform/verisilicon/hantro_hw.h
> @@ -431,6 +431,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
>   		     enum vb2_buffer_state result);
>   void hantro_start_prepare_run(struct hantro_ctx *ctx);
>   void hantro_end_prepare_run(struct hantro_ctx *ctx);
> +void hantro_abort_prepare_run(struct hantro_ctx *ctx);
>   
>   irqreturn_t hantro_g1_irq(int irq, void *dev_id);
>   void hantro_g1_reset(struct hantro_ctx *ctx);
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> index e4e21ad373233..99ffb4a743764 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu981_hw_av1_dec.c
> @@ -2192,8 +2192,7 @@ int rockchip_vpu981_av1_dec_run(struct hantro_ctx *ctx)
>   	return 0;
>   
>   prepare_error:
> -	hantro_end_prepare_run(ctx);
> -	hantro_irq_done(vpu, VB2_BUF_STATE_ERROR);
> +	hantro_abort_prepare_run(ctx);
>   	return ret;
>   }
>   
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/3] media: verisilicon: Complete the request on the ->run() error paths

[Benjamin Gaignard]

Le 24/08/2026 à 08:39, Sascha Hauer a écrit :
> Several codec ->run() operations return early without completing the
> request, even though hantro_start_prepare_run() has already run and set up
> the controls of the request attached to the source buffer.
>
> The buffers are still returned to userspace, device_run() finishes the job
> with VB2_BUF_STATE_ERROR, but nothing completes the control handler object
> bound to the media request. vb2_buffer_done() only unbinds the request
> object owned by videobuf2 itself, so num_incomplete_objects never drops to
> zero and the request stays in MEDIA_REQUEST_STATE_QUEUED forever: poll() on
> the request file descriptor never returns and MEDIA_REQUEST_IOC_REINIT
> fails with -EBUSY. The request is only cleaned up when userspace closes it.
>
> The vb2 buf_request_complete() callback does not help here, it is only
> called from __vb2_queue_cancel() for buffers that were never queued to the
> driver.
>
> Call hantro_abort_prepare_run() on those paths, which completes the request
> and leaves the watchdog alone.
>
> Fixes: 42cb2a8f27d2 ("media: hantro: change hantro_codec_ops run prototype to return errors")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by Benjamin Gaignard <benjamin.gaignard@collabora.com>

> ---
>   drivers/media/platform/verisilicon/hantro_g1_h264_dec.c        | 4 +++-
>   drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c         | 4 +++-
>   drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c        | 9 +++++++--
>   drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c | 4 +++-
>   drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c | 4 +++-
>   drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c  | 4 +++-
>   6 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
> index ad5c1a6634f5c..14ebb25ea24be 100644
> --- a/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g1_h264_dec.c
> @@ -255,8 +255,10 @@ int hantro_g1_h264_dec_run(struct hantro_ctx *ctx)
>   
>   	/* Prepare the H264 decoder context. */
>   	ret = hantro_h264_dec_prepare_run(ctx);
> -	if (ret)
> +	if (ret) {
> +		hantro_abort_prepare_run(ctx);
>   		return ret;
> +	}
>   
>   	/* Configure hardware registers. */
>   	src_buf = hantro_get_src_buf(ctx);
> diff --git a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> index 851eb67f19f50..e866ff86019a0 100644
> --- a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> @@ -442,8 +442,10 @@ int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
>   	hantro_start_prepare_run(ctx);
>   
>   	hdr = hantro_get_ctrl(ctx, V4L2_CID_STATELESS_VP8_FRAME);
> -	if (WARN_ON(!hdr))
> +	if (WARN_ON(!hdr)) {
> +		hantro_abort_prepare_run(ctx);
>   		return -EINVAL;
> +	}
>   
>   	/* Reset segment_map buffer in keyframe */
>   	if (V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) && ctx->vp8_dec.segment_map.cpu)
> diff --git a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> index e8c2e83379def..5778813a9eb2f 100644
> --- a/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> +++ b/drivers/media/platform/verisilicon/hantro_g2_hevc_dec.c
> @@ -596,7 +596,7 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
>   	/* Prepare HEVC decoder context. */
>   	ret = hantro_hevc_dec_prepare_run(ctx);
>   	if (ret)
> -		return ret;
> +		goto abort_prepare_run;
>   
>   	/* Configure hardware registers. */
>   	set_params(ctx);
> @@ -604,7 +604,7 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
>   	/* set reference pictures */
>   	ret = set_ref(ctx);
>   	if (ret)
> -		return ret;
> +		goto abort_prepare_run;
>   
>   	set_buffers(ctx);
>   	prepare_tile_info_buffer(ctx);
> @@ -634,4 +634,9 @@ int hantro_g2_hevc_dec_run(struct hantro_ctx *ctx)
>   	vdpu_write(vpu, G2_REG_INTERRUPT_DEC_E, G2_REG_INTERRUPT);
>   
>   	return 0;
> +
> +abort_prepare_run:
> +	hantro_abort_prepare_run(ctx);
> +
> +	return ret;
>   }
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c
> index 6da87f5184bcb..d412d5d661226 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_h264_dec.c
> @@ -473,8 +473,10 @@ int rockchip_vpu2_h264_dec_run(struct hantro_ctx *ctx)
>   
>   	/* Prepare the H264 decoder context. */
>   	ret = hantro_h264_dec_prepare_run(ctx);
> -	if (ret)
> +	if (ret) {
> +		hantro_abort_prepare_run(ctx);
>   		return ret;
> +	}
>   
>   	src_buf = hantro_get_src_buf(ctx);
>   	set_params(ctx, src_buf);
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> index 61621b1be8a2f..0ba078b9d1875 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_jpeg_enc.c
> @@ -143,8 +143,10 @@ int rockchip_vpu2_jpeg_enc_run(struct hantro_ctx *ctx)
>   
>   	memset(&jpeg_ctx, 0, sizeof(jpeg_ctx));
>   	jpeg_ctx.buffer = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> -	if (!jpeg_ctx.buffer)
> +	if (!jpeg_ctx.buffer) {
> +		hantro_abort_prepare_run(ctx);
>   		return -ENOMEM;
> +	}
>   
>   	jpeg_ctx.width = ctx->dst_fmt.width;
>   	jpeg_ctx.height = ctx->dst_fmt.height;
> diff --git a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c
> index d079075448c96..15f4872dd2bdd 100644
> --- a/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c
> +++ b/drivers/media/platform/verisilicon/rockchip_vpu2_hw_vp8_dec.c
> @@ -519,8 +519,10 @@ int rockchip_vpu2_vp8_dec_run(struct hantro_ctx *ctx)
>   	hantro_start_prepare_run(ctx);
>   
>   	hdr = hantro_get_ctrl(ctx, V4L2_CID_STATELESS_VP8_FRAME);
> -	if (WARN_ON(!hdr))
> +	if (WARN_ON(!hdr)) {
> +		hantro_abort_prepare_run(ctx);
>   		return -EINVAL;
> +	}
>   
>   	/* Reset segment_map buffer in keyframe */
>   	if (V4L2_VP8_FRAME_IS_KEY_FRAME(hdr) && ctx->vp8_dec.segment_map.cpu)
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 3/3] media: verisilicon: Allow the EOS event to be subscribed

[Benjamin Gaignard]

Le 24/08/2026 à 08:39, Sascha Hauer a écrit :
> hantro queues a V4L2_EVENT_EOS in three places.  vidioc_encoder_cmd()
> sends it when a V4L2_ENC_CMD_STOP has drained the encoder:
>
> 	if (ec->cmd == V4L2_ENC_CMD_STOP &&
> 	    v4l2_m2m_has_stopped(ctx->fh.m2m_ctx))
> 		v4l2_event_queue_fh(&ctx->fh, &hantro_eos_event);
>
> hantro_buf_queue() sends it for a capture buffer queued after the last
> one, and hantro_stop_streaming() when the output queue stops while
> draining.
>
> No application can ask for any of them.  The ioctl ops offer
>
> 	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
>
> and v4l2_ctrl_subscribe_event() only knows about V4L2_EVENT_CTRL, so
> VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_EOS fails with -EINVAL and the
> queued events are dropped on the floor.  An application following the
> drain sequence in the stateful encoder documentation has to fall back to
> V4L2_BUF_FLAG_LAST, which hantro does set, so this has gone unnoticed.
>
> Dispatch on the event type and hand V4L2_EVENT_EOS to
> v4l2_event_subscribe(), the way coda-common.c does.  Everything else
> keeps going to the control handler.
>
> V4L2_EVENT_SOURCE_CHANGE is deliberately not added, hantro never sends
> one.
>
> Fixes: daf3999c12dc ("media: hantro: Implement support for encoder commands")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by Benjamin Gaignard <benjamin.gaignard@collabora.com>

> ---
>   drivers/media/platform/verisilicon/hantro_v4l2.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 83af9fa1ce949..9e19daffe0075 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -765,6 +765,17 @@ static int vidioc_encoder_cmd(struct file *file, void *priv,
>   	return 0;
>   }
>   
> +static int hantro_subscribe_event(struct v4l2_fh *fh,
> +				  const struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_EOS:
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
> +	default:
> +		return v4l2_ctrl_subscribe_event(fh, sub);
> +	}
> +}
> +
>   const struct v4l2_ioctl_ops hantro_ioctl_ops = {
>   	.vidioc_querycap = vidioc_querycap,
>   	.vidioc_enum_framesizes = vidioc_enum_framesizes,
> @@ -787,7 +798,7 @@ const struct v4l2_ioctl_ops hantro_ioctl_ops = {
>   	.vidioc_remove_bufs = v4l2_m2m_ioctl_remove_bufs,
>   	.vidioc_expbuf = v4l2_m2m_ioctl_expbuf,
>   
> -	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> +	.vidioc_subscribe_event = hantro_subscribe_event,
>   	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>   
>   	.vidioc_streamon = v4l2_m2m_ioctl_streamon,
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip