[PATCH v1 5/9] media: chips-media: wave5: finish a job only once
From: Jackson.lee
Date: Tue Oct 06 2026 - 22:04:36 EST
From: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
v4l2_m2m_job_finish() checks that the context owns the current job, but
not which job it is, so a second finish from the same context clears
m2m_dev->curr_ctx all the same. v4l2_m2m_try_run() picks the next job
under job_spinlock, drops it, and only then reads curr_ctx again to hand
its priv to device_run(). A stray finish landing in that window makes
that read return NULL and the decoder oopses:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000358
Workqueue: events v4l2_m2m_device_run_work [v4l2_mem2mem]
pc : v4l2_m2m_try_run+0x78/0x140 [v4l2_mem2mem]
The decoder finishes jobs from seven places: device_run() itself, the
completion IRQ, and job_abort(). None of them tracked whether a job was
still outstanding, so the same job was finished twice whenever the IRQ
and device_run() both reached the end. Instrumenting the call sites over
150 s of a three-stream run counted 63 finishes with no job to finish,
two of them a plain double finish.
Take ownership of the slot once per device_run() and let only the winner
call in; whoever loses the race now returns without touching the shared
state. Also compare v4l2_m2m_get_curr_priv() against this instance rather
than testing it for NULL, which let one instance finish another's job.
job_abort() stays unconditional. v4l2_m2m_cancel_job() only calls it
while this context owns the running job and then sleeps until
TRANS_RUNNING clears, which nothing but a finish does, so it has to end
the job even before device_run() has claimed the slot. It no longer
returns early on a failed state switch either, for the same reason.
Fixes: a176ac5e701f ("media: chips-media: wave5: Improve performance of decoder")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
---
.../chips-media/wave5/wave5-vpu-dec.c | 78 +++++++++++++------
.../platform/chips-media/wave5/wave5-vpuapi.h | 2 +
2 files changed, 57 insertions(+), 23 deletions(-)
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index 07dcb20ffdcb..18400af036dc 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -102,6 +102,28 @@ static const struct vpu_format dec_fmt_list[FMT_TYPES][MAX_FMTS] = {
}
};
+/* bit in vpu_instance.job_flags */
+#define WAVE5_JOB_RUNNING 0
+
+/*
+ * Hand the shared job slot back, exactly once per device_run().
+ *
+ * v4l2_m2m_job_finish() only checks that the *context* owns the current job,
+ * not which job it is, so a second finish from the same context clears
+ * m2m_dev->curr_ctx -- and it may do so for the job v4l2_m2m_try_run() has just
+ * put there and is about to hand to device_run(), which then dereferences NULL.
+ *
+ * device_run() and the completion IRQ both reach for this and either may get
+ * there first, so ownership is taken atomically and only the winner calls in.
+ */
+static void wave5_dec_finish_job(struct vpu_instance *inst)
+{
+ if (!test_and_clear_bit(WAVE5_JOB_RUNNING, &inst->job_flags))
+ return;
+
+ v4l2_m2m_job_finish(inst->v4l2_m2m_dev, inst->v4l2_fh.m2m_ctx);
+}
+
/*
* Make sure that the state switch is allowed and add logging for debugging
* purposes
@@ -247,7 +269,7 @@ static int start_decode(struct vpu_instance *inst, u32 *fail_res)
set_instance_state(inst, VPU_INST_STATE_STOP);
dev_dbg(inst->dev->dev, "%s: pic run failed / finish job", __func__);
- v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+ wave5_dec_finish_job(inst);
}
return ret;
@@ -376,7 +398,7 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
ret = wave5_vpu_dec_get_output_info(inst, &dec_info);
if (ret) {
dev_dbg(inst->dev->dev, "%s: could not get output info.", __func__);
- v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+ wave5_dec_finish_job(inst);
return;
}
@@ -389,7 +411,7 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
if (!vb2_is_streaming(dst_vq)) {
dev_dbg(inst->dev->dev, "%s: capture is not streaming..", __func__);
- v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+ wave5_dec_finish_job(inst);
return;
}
@@ -470,13 +492,13 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
}
if (inst->sent_eos &&
- v4l2_m2m_get_curr_priv(inst->v4l2_m2m_dev)) {
+ v4l2_m2m_get_curr_priv(inst->v4l2_m2m_dev) == inst) {
struct queue_status_info q_status;
wave5_vpu_dec_give_command(inst, DEC_GET_QUEUE_STATUS, &q_status);
if (q_status.report_queue_count == 0 &&
q_status.instance_queue_count == 0)
- v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+ wave5_dec_finish_job(inst);
}
if (inst->queuing_fail) {
@@ -1686,6 +1708,8 @@ static void wave5_vpu_dec_device_run(void *priv)
int ret = 0;
bool cmd_issued = false;
+ set_bit(WAVE5_JOB_RUNNING, &inst->job_flags);
+
dev_dbg(inst->dev->dev, "%s: Fill the ring buffer with new bitstream data", __func__);
pm_runtime_resume_and_get(inst->dev->dev);
if (!inst->retry) {
@@ -1810,36 +1834,44 @@ static void wave5_vpu_dec_device_run(void *priv)
* stalling every instance sharing the VPU.
*/
if (!inst->sent_eos || !cmd_issued)
- v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+ wave5_dec_finish_job(inst);
}
static void wave5_vpu_dec_job_abort(void *priv)
{
struct vpu_instance *inst = priv;
- struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
int ret;
ret = set_instance_state(inst, VPU_INST_STATE_STOP);
- if (ret)
- return;
- /*
- * job_abort() runs from the STREAMOFF path and may be called while the
- * device is runtime suspended. Setting the EOS flag talks to the
- * firmware (send_firmware_command() accesses VPU registers), so the
- * device must be resumed first; otherwise the register access faults
- * with an asynchronous SError.
- */
- pm_runtime_resume_and_get(inst->dev->dev);
+ if (!ret) {
+ /*
+ * job_abort() runs from the STREAMOFF path and may be called while the
+ * device is runtime suspended. Setting the EOS flag talks to the
+ * firmware (send_firmware_command() accesses VPU registers), so the
+ * device must be resumed first; otherwise the register access faults
+ * with an asynchronous SError.
+ */
+ pm_runtime_resume_and_get(inst->dev->dev);
- ret = wave5_vpu_dec_set_eos_on_firmware(inst);
- if (ret)
- dev_warn(inst->dev->dev,
- "Setting EOS for the bitstream, fail: %d\n", ret);
+ ret = wave5_vpu_dec_set_eos_on_firmware(inst);
+ if (ret)
+ dev_warn(inst->dev->dev,
+ "Setting EOS for the bitstream, fail: %d\n", ret);
- pm_runtime_put_autosuspend(inst->dev->dev);
+ pm_runtime_put_autosuspend(inst->dev->dev);
+ }
- v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+ /*
+ * The one caller that must always end the job. v4l2_m2m_cancel_job()
+ * only gets here while this context owns the running job, and then
+ * sleeps until TRANS_RUNNING clears -- which only a finish does. Go
+ * straight past the ownership check: device_run() may not have claimed
+ * the slot yet, and skipping the finish would leave streamoff waiting
+ * for a job nobody is going to end.
+ */
+ clear_bit(WAVE5_JOB_RUNNING, &inst->job_flags);
+ v4l2_m2m_job_finish(inst->v4l2_m2m_dev, inst->v4l2_fh.m2m_ctx);
}
static int wave5_vpu_dec_job_ready(void *priv)
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 7b08fef58217..a338e39be1c9 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -826,6 +826,8 @@ struct vpu_instance {
struct mutex feed_lock; /* lock for feeding bitstream buffers */
bool queuing_fail; /* if there is the queuing failure */
bool empty_queue;
+ unsigned long job_flags; /* bit 0: a device_run() job is ours to finish */
+ struct delayed_work unstall_work; /* releases a stale empty_queue park */
struct vpu_buf bitstream_vbuf;
dma_addr_t last_rd_ptr;
size_t remaining_consumed_bytes;
--
2.43.0