[PATCH v2 0/9] fix decoder corruption, stalls and seek issues

[PATCH v2 0/9] fix decoder corruption, stalls and seek issues

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

This series fixes a set of decoder problems in the wave5 driver that show
up as corrupted output, stalled or crashed instances, and lost frames
across seeks. All of them carry a Fixes: tag.

Locking and job ownership
  1. Serialise removal of OUTPUT buffers between finish_decode(),
     streamoff_output() and the start_decode() error path.
  4. Acknowledge the interrupt only after it has been dispatched, so a
     completion the VPU adds meanwhile is no longer dropped and does not
     leave an instance waiting forever.
  5. Finish each m2m job exactly once. A second v4l2_m2m_job_finish() from
     the same context could clear curr_ctx under v4l2_m2m_try_run() and
     oops in the m2m worker.

Decode scheduling
  6. Issue DEC_PIC only when the ring buffer holds bitstream that no queued
     command will consume. The current check compares two unrelated
     counters, which sends commands against an empty ring or duplicates
     one already in flight and corrupts the output. A delayed work restarts
     an instance left idle with data still buffered.

Seek / flush
  2. Drop the consumed-byte tally on OUTPUT streamoff, so it is not
     credited to the first buffers queued after a seek.
  3. Wait between FLUSH retries instead of spending the retry budget in
     microseconds, and ignore dec_info when reading it failed.
  8. Re-establish the display flags from the vb2 buffer states after a
     flush, so buffers held by userspace are not decoded into.
  9. Do not reset the frame buffers on a CAPTURE streamoff that is only
     part of a seek.

Timestamps
  7. Hand OUTPUT timestamps to decoded pictures in decode order. One
     completion can consume several source buffers, and keeping only the
     last timestamp lost the others ("Too old frames, bug in decoder" in
     GStreamer).

Changes in v2
-------------
- 1/9: wave5_handle_src_buffer() peeks the head of the OUTPUT ready queue
  with v4l2_m2m_next_src_buf() instead of walking the queue, which raced
  with VIDIOC_QBUF adding to it under rdy_spinlock only.
- 4/9: Reworded the commit message and the comment: the firmware does not
  update W5_RET_QUEUE_CMD_DONE_INST until the interrupt is acknowledged
  through W5_VPU_VINT_REASON_CLR and W5_VPU_VINT_CLEAR. No code change.
- 5/9: job_abort() no longer ends a job that device_run() is still running
  or about to run, which released v4l2_m2m_cancel_job()'s wait early. A
  per-instance run_lock is held across device_run() and taken in
  job_abort(), which now finishes through the same ownership check.
  start_decode() leaves the finish to device_run() when it fails.
  Wrapped the oops line in the commit message (checkpatch).
- 6/9: release() disables the unstall timer instead of cancelling it, so
  a device_run() still executing at close cannot re-arm it on the freed
  instance.
- 8/9: Wrapped the oops line in the commit message (checkpatch).
- 2/9, 3/9, 7/9 and 9/9 are unchanged.
- Added Brandon's Tested-by to 2/9, 3/9, 4/9, 7/9 and 8/9. It is not
  carried on 5/9 and 6/9, whose code changed since v1.

Link to v1: https://lore.kernel.org/r/20261007015946.53-1-jackson.lee@chipsnmedia.com

Testing
-------
Tested on a TI J721S2 EVM (Wave521C, product code 0x521c, firmware
revision 363254) with GStreamer 1.24.13 and the v4l2h264dec/v4l2h265dec
elements.

fluster (GStreamer-H.264-V4L2-Gst1.0 / GStreamer-H.265-V4L2-Gst1.0):

  JVT-AVC_V1      77/135  (-j1 and -j3, identical per vector)
  JCT-VC-HEVC_V1 133/147  (-j1 and -j3, identical per vector)

The pass/fail result of every vector is the same as with v1. The
remaining AVC failures are interlaced, MBAFF/PAFF, FMO and SP slice
streams. 11 of the 14 HEVC failures are Main10 streams, which this SoC
does not decode ("no support for 10 bit depth"); the others are
PICSIZE_A_Bossen_1, PICSIZE_B_Bossen_1 and DELTAQP_A_BRCM_4.

No oops, call trace, workqueue warning or "invalid display frame index"
message was logged during the runs.

v4l2-compliance 1.28.1-5233, SHA: fc15e229d9d3:

  Total for wave5-dec device /dev/video0: 47, Succeeded: 47, Failed: 0, Warnings: 2
  Total for wave5-enc device /dev/video1: 47, Succeeded: 47, Failed: 0, Warnings: 0

For the job_abort() and release() changes, three concurrent
v4l2h264dec pipelines were also run looping a 1 s clip with flushing
seeks, seeking to random positions every 700 ms, and interrupted or
killed 100 times. No stall, oops or hang was seen.

Brandon Brnich (2):
  media: chips-media: wave5: Ensure Atomic Access to src_buf list
  media: chips-media: wave5: Stop FrameBuf Reset During Seek

Jackson Lee (7):
  media: chips-media: wave5: drop the consumed-byte tally on OUTPUT
    streamoff
  media: chips-media: wave5: wait before retrying a refused flush
  media: chips-media: wave5: ack the interrupt after dispatching it
  media: chips-media: wave5: finish a job only once
  media: chips-media: wave5: decode only when the ring holds unclaimed
    bitstream
  media: chips-media: wave5: stamp decoded pictures from a decode-order
    queue
  media: chips-media: wave5: restore the display flags after a flush

 .../chips-media/wave5/wave5-vpu-dec.c         | 337 +++++++++++++++---
 .../platform/chips-media/wave5/wave5-vpu.c    |  29 +-
 .../platform/chips-media/wave5/wave5-vpuapi.c |  19 +-
 .../platform/chips-media/wave5/wave5-vpuapi.h |  14 +
 4 files changed, 334 insertions(+), 65 deletions(-)


base-commit: 58348f64125e9a3e44d3abb275ca7f4e6c9641e5
--
2.43.0

[PATCH v2 1/9] media: chips-media: wave5: Ensure Atomic Access to src_buf list

[Jackson.lee]

From: Brandon Brnich <b-brnich@ti.com>

feed_lock was introduced in decoder performance improvements. This was
used to ensure atomic access to the driver's avail_src_buf list during
fill_ringbuffer, streamoff_output, and buf_queue. While this protected
the drivers available src bufs, the v4l2_m2m_src_bufs were not being
protected at driver level. Three separate threads have access to remove
buffers: start_decode fail path, streamoff_output, and finish_decode.

In streamoff_output(), replace the inst_src_buf_remove() loop with
list_for_each_entry_safe(). inst_src_buf_remove() takes feed_lock
itself, so calling it while streamoff_output() holds feed_lock would
take the same mutex twice and deadlock. Unlink each entry with
list_del_init() directly instead.

In wave5_handle_src_buffer(), feed_lock does not serialize against
v4l2_m2m_buf_queue(), which buf_queue calls after dropping feed_lock and
which adds to the ready queue under rdy_spinlock only. Walking the ready
queue with v4l2_m2m_for_each_src_buf_safe() therefore races with
VIDIOC_QBUF. The loop only ever looks at the head of the queue, so peek
it with v4l2_m2m_next_src_buf(), which takes rdy_spinlock, instead.

Fixes: a176ac5e701f ("media: chips-media: wave5: Improve performance of decoder")
Cc: stable@vger.kernel.org
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../platform/chips-media/wave5/wave5-vpu-dec.c  | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 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 6564cf3ec739..5993102e5afe 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -180,7 +180,7 @@ static bool wave5_last_src_buffer_consumed(struct v4l2_m2m_ctx *m2m_ctx)
 static void wave5_handle_src_buffer(struct vpu_instance *inst, dma_addr_t rd_ptr)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
-	struct v4l2_m2m_buffer *buf, *n;
+	struct vb2_v4l2_buffer *src_buf;
 	size_t consumed_bytes = 0;
 
 	if (rd_ptr >= inst->last_rd_ptr) {
@@ -198,8 +198,8 @@ static void wave5_handle_src_buffer(struct vpu_instance *inst, dma_addr_t rd_ptr
 	dev_dbg(inst->dev->dev, "%s: %zu bytes of bitstream was consumed", __func__,
 		consumed_bytes);
 
-	v4l2_m2m_for_each_src_buf_safe(m2m_ctx, buf, n) {
-		struct vb2_v4l2_buffer *src_buf = &buf->vb;
+	mutex_lock(&inst->feed_lock);
+	while ((src_buf = v4l2_m2m_next_src_buf(m2m_ctx))) {
 		size_t src_size = vb2_get_plane_payload(&src_buf->vb2_buf, 0);
 
 		if (src_size > consumed_bytes)
@@ -224,6 +224,7 @@ static void wave5_handle_src_buffer(struct vpu_instance *inst, dma_addr_t rd_ptr
 			break;
 		}
 	}
+	mutex_unlock(&inst->feed_lock);
 
 	inst->remaining_consumed_bytes = consumed_bytes;
 }
@@ -237,9 +238,11 @@ static int start_decode(struct vpu_instance *inst, u32 *fail_res)
 	if (ret) {
 		struct vb2_v4l2_buffer *src_buf;
 
+		mutex_lock(&inst->feed_lock);
 		src_buf = v4l2_m2m_src_buf_remove(m2m_ctx);
 		if (src_buf)
 			v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
+		mutex_unlock(&inst->feed_lock);
 		set_instance_state(inst, VPU_INST_STATE_STOP);
 
 		dev_dbg(inst->dev->dev, "%s: pic run failed / finish job", __func__);
@@ -1453,12 +1456,13 @@ static int streamoff_output(struct vb2_queue *q)
 	dma_addr_t new_rd_ptr;
 	struct dec_output_info dec_info;
 	unsigned int i;
-	struct vpu_src_buffer *vpu_buf;
+	struct vpu_src_buffer *vpu_buf, *tmp;
 
 	inst->retry = false;
 	inst->queuing_num = 0;
-	while ((vpu_buf = inst_src_buf_remove(inst)) != NULL)
-		;
+	mutex_lock(&inst->feed_lock);
+	list_for_each_entry_safe(vpu_buf, tmp, &inst->avail_src_bufs, list)
+		list_del_init(&vpu_buf->list);
 
 	for (i = 0; i < v4l2_m2m_num_dst_bufs_ready(m2m_ctx); i++) {
 		ret = wave5_vpu_dec_set_disp_flag(inst, i);
@@ -1473,6 +1477,7 @@ static int streamoff_output(struct vb2_queue *q)
 			__func__, buf->vb2_buf.type, buf->vb2_buf.index);
 		v4l2_m2m_buf_done(buf, VB2_BUF_STATE_ERROR);
 	}
+	mutex_unlock(&inst->feed_lock);
 
 	while (wave5_vpu_dec_get_output_info(inst, &dec_info) == 0) {
 		if (dec_info.index_frame_display >= 0)

base-commit: 58348f64125e9a3e44d3abb275ca7f4e6c9641e5
-- 
2.43.0

[PATCH v2 2/9] media: chips-media: wave5: drop the consumed-byte tally on OUTPUT streamoff

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

remaining_consumed_bytes is never cleared. On a seek the OUTPUT buffers
it accounts for are returned with VB2_BUF_STATE_ERROR, but the tally
survives and is credited to the first buffers queued afterwards, which
are then completed before the VPU has read them.

Clear it where last_rd_ptr is already being rebased.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Tested-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../media/platform/chips-media/wave5/wave5-vpu-dec.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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 5993102e5afe..a314813a6b4d 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1494,6 +1494,18 @@ static int streamoff_output(struct vb2_queue *q)
 	inst->codec_info->dec_info.stream_rd_ptr = new_rd_ptr;
 	inst->codec_info->dec_info.stream_wr_ptr = new_rd_ptr;
 
+	/*
+	 * remaining_consumed_bytes holds the consumed bytes that no source
+	 * buffer has claimed yet. The buffers it belongs to are returned with
+	 * VB2_BUF_STATE_ERROR just above, so the tally has to go with them --
+	 * last_rd_ptr is being rebased for the same reason.
+	 *
+	 * Left behind it is credit against buffers that no longer exist, and
+	 * wave5_handle_src_buffer() spends it on the first buffers queued after
+	 * the seek, completing them before the VPU has read their payload.
+	 */
+	inst->remaining_consumed_bytes = 0;
+
 	if (v4l2_m2m_has_stopped(m2m_ctx)) {
 		unsigned long flags;
 
-- 
2.43.0

[PATCH v2 3/9] media: chips-media: wave5: wait before retrying a refused flush

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

wave5_vpu_flush_instance() repeats FLUSH until it is accepted but never
waits between attempts, so the whole retry budget is spent in a few hundred
microseconds - far less than a large frame takes to decode. A flush issued
while a picture is in flight is refused every time and gives up:

  Flush of DECODER instance with id: 0 timed out!

streamoff_output() then returns -ETIMEDOUT without resetting the ring
buffer and the instance never decodes again.

Sleep 1-2 ms between attempts, and only use dec_info when the call that
filled it succeeded: otherwise index_frame_display would set a display flag
on an arbitrary frame buffer. With a flushing seek every 500 ms the decoder
stopped after 519 frames before this change and ran past 2072 with it.

Fixes: 45d1a2b93277 ("media: chips-media: wave5: Add vpuapi layer")
Fixes: a2c75e964e51 ("media: chips-media: wave5: Fix a hang after seeking")
Cc: stable@vger.kernel.org
Tested-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../platform/chips-media/wave5/wave5-vpuapi.c  | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
index f77abd5e122a..397da200a9ed 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
@@ -78,13 +78,27 @@ int wave5_vpu_flush_instance(struct vpu_instance *inst)
 			return -ETIMEDOUT;
 		} else if (ret == -EBUSY) {
 			struct dec_output_info dec_info;
+			int info_ret;
 
 			mutex_unlock(&inst->dev->hw_lock);
-			wave5_vpu_dec_get_output_info(inst, &dec_info);
+			info_ret = wave5_vpu_dec_get_output_info(inst, &dec_info);
+			if (info_ret) {
+				/*
+				 * A flush is refused while a picture is still
+				 * being decoded, and there is no result to
+				 * collect yet either. Retrying straight away
+				 * spends the whole retry budget in microseconds,
+				 * far less than a large frame takes to decode, so
+				 * the flush times out and streamoff_output() bails
+				 * out before resetting the ring buffer. Give the
+				 * decode time to land instead.
+				 */
+				usleep_range(1000, 2000);
+			}
 			mutex_ret = mutex_lock_interruptible(&inst->dev->hw_lock);
 			if (mutex_ret)
 				return mutex_ret;
-			if (dec_info.index_frame_display >= 0) {
+			if (!info_ret && dec_info.index_frame_display >= 0) {
 				mutex_unlock(&inst->dev->hw_lock);
 				wave5_vpu_dec_set_disp_flag(inst, dec_info.index_frame_display);
 				mutex_ret = mutex_lock_interruptible(&inst->dev->hw_lock);
-- 
2.43.0

[PATCH v2 4/9] media: chips-media: wave5: ack the interrupt after dispatching it

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

W5_RET_QUEUE_CMD_DONE_INST is written by both the driver and the VPU:
the VPU sets the bit of every instance whose command completed, and the
driver clears the bits it has consumed. The driver does that as a
read-modify-write over a snapshot taken at the top of the handler, so
any bit the VPU adds in between is erased.

The firmware does not update that register, for any reason, until the
host acknowledges the interrupt through W5_VPU_VINT_REASON_CLR and
W5_VPU_VINT_CLEAR, so leaving the interrupt unacknowledged for the whole
dispatch is what keeps the register still. Acknowledging first opened
the race instead: the firmware could rewrite the register, and the
write-back then dropped the instance it had just added. That instance is
never reported again -- its queue count stays non-zero and no further
completion arrives, so it stops decoding for good while the others keep
running.

Move both acknowledge writes past the dispatch loop. The re-read of
W5_VPU_VINT_REASON that tried to narrow the window is no longer needed.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Tested-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../platform/chips-media/wave5/wave5-vpu.c    | 29 ++++++++++++-------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
index 76d57c6b636a..6a5b646ff0f9 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
@@ -51,7 +51,6 @@ static void wave5_vpu_handle_irq(void *dev_id)
 	u32 seq_done;
 	u32 cmd_done;
 	u32 irq_reason;
-	u32 irq_subreason;
 	struct vpu_instance *inst, *tmp;
 	struct vpu_device *dev = dev_id;
 	int val;
@@ -60,8 +59,6 @@ static void wave5_vpu_handle_irq(void *dev_id)
 	irq_reason = wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
 	seq_done = wave5_vdi_read_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO);
 	cmd_done = wave5_vdi_read_register(dev, W5_RET_QUEUE_CMD_DONE_INST);
-	wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_reason);
-	wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
 
 	spin_lock_irqsave(&dev->irq_spinlock, flags);
 	list_for_each_entry_safe(inst, tmp, &dev->instances, list) {
@@ -86,14 +83,10 @@ static void wave5_vpu_handle_irq(void *dev_id)
 		    irq_reason & BIT(INT_WAVE5_ENC_PIC)) {
 			if (cmd_done & BIT(inst->id)) {
 				cmd_done &= ~BIT(inst->id);
-				if (dev->irq >= 0) {
-					irq_subreason =
-						wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
-					if (!(irq_subreason & BIT(INT_WAVE5_DEC_PIC)))
-						wave5_vdi_write_register(dev,
-									 W5_RET_QUEUE_CMD_DONE_INST,
-									 cmd_done);
-				}
+				if (dev->irq >= 0)
+					wave5_vdi_write_register(dev,
+								 W5_RET_QUEUE_CMD_DONE_INST,
+								 cmd_done);
 				val = BIT(INT_WAVE5_DEC_PIC);
 				kfifo_in(&inst->irq_status, &val, sizeof(int));
 			}
@@ -101,6 +94,20 @@ static void wave5_vpu_handle_irq(void *dev_id)
 	}
 	spin_unlock_irqrestore(&dev->irq_spinlock, flags);
 
+	/*
+	 * Acknowledge only now that every instance bit this interrupt carried
+	 * has been consumed. W5_RET_QUEUE_CMD_DONE_INST is written by both
+	 * sides, but the firmware does not update it, for any reason, until
+	 * the interrupt is acknowledged through W5_VPU_VINT_REASON_CLR and
+	 * W5_VPU_VINT_CLEAR. Until then the snapshot taken above stays exact
+	 * and the read-modify-write in the loop cannot lose a bit.
+	 * Acknowledging first would let the firmware add an instance bit
+	 * underneath that snapshot, and the write-back would then drop it --
+	 * an instance whose completion is never reported again.
+	 */
+	wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_reason);
+	wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
+
 	if (dev->irq < 0)
 		up(&dev->irq_sem);
 }
-- 
2.43.0

[PATCH v2 5/9] media: chips-media: wave5: finish a job only once

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

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() must not end a job that device_run() is still running or is
about to run. v4l2_m2m_try_run() sets TRANS_RUNNING before it calls
device_run(), so v4l2_m2m_cancel_job() can get there first, and a finish
from job_abort() then releases cancel_job()'s wait early: try_run()
reads a curr_ctx that is already NULL, or release() frees the instance
under device_run(). Hold a per-instance run_lock across device_run() and
take it in job_abort(), which then finishes through the same ownership
check. The only job left for it to end is one device_run() handed over
to the DEC_PIC completion after EOS; device_run() finishes every other
one itself. job_abort() no longer returns early on a failed state
switch, so that held job is still ended and streamoff cannot hang on it.

start_decode() no longer finishes the job when it fails: device_run()
does that on its way out, as for any run that queued no DEC_PIC.

Fixes: a176ac5e701f ("media: chips-media: wave5: Improve performance of decoder")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../chips-media/wave5/wave5-vpu-dec.c         | 93 ++++++++++++++-----
 .../platform/chips-media/wave5/wave5-vpuapi.c |  1 +
 .../platform/chips-media/wave5/wave5-vpuapi.h |  3 +
 3 files changed, 73 insertions(+), 24 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 a314813a6b4d..3ecf535f5622 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,29 @@ 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(), the completion IRQ and job_abort() all reach for this and any
+ * of them 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
@@ -245,8 +268,7 @@ static int start_decode(struct vpu_instance *inst, u32 *fail_res)
 		mutex_unlock(&inst->feed_lock);
 		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);
+		dev_dbg(inst->dev->dev, "%s: pic run failed", __func__);
 	}
 
 	return ret;
@@ -375,7 +397,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;
 	}
 
@@ -388,7 +410,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;
 	}
 
@@ -469,13 +491,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) {
@@ -1685,6 +1707,9 @@ static void wave5_vpu_dec_device_run(void *priv)
 	int ret = 0;
 	bool cmd_issued = false;
 
+	mutex_lock(&inst->run_lock);
+	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) {
@@ -1809,36 +1834,55 @@ 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);
+	mutex_unlock(&inst->run_lock);
 }
 
 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.
+	 * v4l2_m2m_cancel_job() calls this while the context owns the running
+	 * job and then sleeps until a finish clears TRANS_RUNNING. Ending the
+	 * job from here while device_run() is still executing -- or is about
+	 * to be entered, v4l2_m2m_try_run() having set TRANS_RUNNING already --
+	 * releases that wait early: try_run() then reads a curr_ctx that is
+	 * already NULL, or release() frees the instance under device_run().
+	 * So wait for device_run() to return first.
 	 */
-	pm_runtime_resume_and_get(inst->dev->dev);
+	mutex_lock(&inst->run_lock);
 
-	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 = set_instance_state(inst, VPU_INST_STATE_STOP);
 
-	pm_runtime_put_autosuspend(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);
 
-	v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+		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);
+	}
+
+	/*
+	 * With device_run() out of the way, the job is either already finished,
+	 * still to be run -- device_run() then finds the instance stopped,
+	 * issues nothing and finishes it itself -- or held for a DEC_PIC
+	 * completion after EOS. Only the last one is left for us: end it now
+	 * rather than leave streamoff or release() waiting on that IRQ.
+	 */
+	wave5_dec_finish_job(inst);
+	mutex_unlock(&inst->run_lock);
 }
 
 static int wave5_vpu_dec_job_ready(void *priv)
@@ -1920,6 +1964,7 @@ static int wave5_vpu_open_dec(struct file *filp)
 
 	spin_lock_init(&inst->state_spinlock);
 	mutex_init(&inst->feed_lock);
+	mutex_init(&inst->run_lock);
 	INIT_LIST_HEAD(&inst->avail_src_bufs);
 
 	inst->codec_info = kzalloc_obj(*inst->codec_info);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
index 397da200a9ed..c7e61e2b78eb 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
@@ -286,6 +286,7 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
 	wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_task);
 
 	mutex_destroy(&inst->feed_lock);
+	mutex_destroy(&inst->run_lock);
 
 unlock_and_return:
 	mutex_unlock(&vpu_dev->hw_lock);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 7b08fef58217..2d515c9028a5 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -826,6 +826,9 @@ 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 mutex run_lock; /* held across device_run(); job_abort() waits on it */
+	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

[PATCH v2 6/9] media: chips-media: wave5: decode only when the ring holds unclaimed bitstream

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

device_run() decides whether to issue a DEC_PIC by comparing the instance
queue count against the number of OUTPUT buffers held. Those two count
unrelated things and diverge routinely with interrupt timing, so commands
go out against an empty ring buffer, or duplicate one already in flight,
corrupting the decoded output and producing display results the driver is
not expecting.

Decide on the real state instead: decode only when the ring still holds
bitstream and no queued command will consume it.

That can leave an instance idle with data still buffered. If the in-flight
command completes without freeing an OUTPUT buffer, nothing re-runs
device_run() and a client that has queued every buffer it owns cannot
restart it. Re-check on a 100 ms timer, which a normal wait outruns.

release() disables that timer rather than cancelling it: a device_run()
still executing at close can re-arm it until v4l2_m2m_ctx_release() has
waited for that run, and a cancelled timer would then fire on the freed
instance.

Fixes: 8c5a74a24cbb ("media: chips-media: wave5: avoid skipping device_run while VPU has work")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../chips-media/wave5/wave5-vpu-dec.c         | 67 +++++++++++++++++--
 .../platform/chips-media/wave5/wave5-vpuapi.h |  1 +
 2 files changed, 64 insertions(+), 4 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 3ecf535f5622..668c923dfa36 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1618,6 +1618,7 @@ static void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
 	else
 		streamoff_capture(q);
 
+	cancel_delayed_work_sync(&inst->unstall_work);
 	inst->empty_queue = false;
 	inst->sent_eos = false;
 	pm_runtime_put_autosuspend(inst->dev->dev);
@@ -1698,6 +1699,31 @@ static bool wave5_is_draining_or_eos(struct vpu_instance *inst)
 	return m2m_ctx->is_draining || inst->eos;
 }
 
+/*
+ * How long an instance may sit idle with bitstream still buffered before it is
+ * assumed stuck. A normal wait -- including one instance queued behind others on
+ * a shared VPU -- clears in a few milliseconds, so this never fires for it.
+ */
+#define WAVE5_UNSTALL_DELAY_MS 100
+
+static void wave5_vpu_dec_unstall_work(struct work_struct *work)
+{
+	struct vpu_instance *inst = container_of(to_delayed_work(work),
+						 struct vpu_instance, unstall_work);
+
+	/*
+	 * Someone already made progress, or the instance is no longer running a
+	 * picture. Poking the scheduler for an instance that is tearing down
+	 * would queue a job against a context that is about to be freed.
+	 */
+	if (!inst->empty_queue || inst->state != VPU_INST_STATE_PIC_RUN)
+		return;
+
+	dev_dbg(inst->dev->dev, "%s: restarting a stalled instance\n", __func__);
+	inst->empty_queue = false;
+	v4l2_m2m_try_schedule(inst->v4l2_fh.m2m_ctx);
+}
+
 static void wave5_vpu_dec_device_run(void *priv)
 {
 	struct vpu_instance *inst = priv;
@@ -1717,14 +1743,32 @@ static void wave5_vpu_dec_device_run(void *priv)
 		if (ret < 0) {
 			dev_warn(inst->dev->dev, "Filling ring buffer failed\n");
 			goto finish_job_and_return;
-		} else if (!inst->eos &&
-				inst->queuing_num == 0 &&
-				inst->state == VPU_INST_STATE_PIC_RUN) {
+		} else if (!inst->eos && inst->state == VPU_INST_STATE_PIC_RUN) {
+			struct dec_info *dec_info = &inst->codec_info->dec_info;
+			bool ring_has_data;
+
 			wave5_vpu_dec_give_command(inst, DEC_GET_QUEUE_STATUS, &q_status);
-			if (q_status.instance_queue_count == v4l2_m2m_num_src_bufs_ready(m2m_ctx)) {
+			ring_has_data = dec_info->stream_rd_ptr != dec_info->stream_wr_ptr;
+
+			/*
+			 * Nothing new to feed, so stop here and wait -- unless the
+			 * ring still holds bitstream that no queued command will
+			 * consume. Decoding an empty ring, or redoing what an
+			 * in-flight command will take, corrupts the output.
+			 */
+			if (q_status.instance_queue_count || !ring_has_data) {
 				dev_dbg(inst->dev->dev, "%s: no bitstream, skip\n",
 					__func__);
 				inst->empty_queue = true;
+				/*
+				 * Stopping with data left over relies on the
+				 * in-flight command to bring us back. If it ends
+				 * without freeing an OUTPUT buffer, a client that
+				 * holds them all cannot queue one to restart us.
+				 */
+				if (ring_has_data)
+					mod_delayed_work(system_percpu_wq, &inst->unstall_work,
+							 msecs_to_jiffies(WAVE5_UNSTALL_DELAY_MS));
 				goto finish_job_and_return;
 			}
 		}
@@ -1966,6 +2010,7 @@ static int wave5_vpu_open_dec(struct file *filp)
 	mutex_init(&inst->feed_lock);
 	mutex_init(&inst->run_lock);
 	INIT_LIST_HEAD(&inst->avail_src_bufs);
+	INIT_DELAYED_WORK(&inst->unstall_work, wave5_vpu_dec_unstall_work);
 
 	inst->codec_info = kzalloc_obj(*inst->codec_info);
 	if (!inst->codec_info) {
@@ -2060,6 +2105,20 @@ static int wave5_vpu_open_dec(struct file *filp)
 
 static int wave5_vpu_dec_release(struct file *filp)
 {
+	struct vpu_instance *inst = file_to_vpu_inst(filp);
+
+	/*
+	 * The unstall timer dereferences both m2m_ctx and inst, which
+	 * wave5_vpu_release_device() frees. stop_streaming() disarms it for a
+	 * streaming instance; make sure nothing is left pending on any other path.
+	 *
+	 * Disable it rather than cancel it: a device_run() still executing
+	 * here can re-arm it until v4l2_m2m_ctx_release() below has waited for
+	 * that run, and a cancelled timer would then fire on the freed
+	 * instance. The instance is never reused, so it is never re-enabled.
+	 */
+	disable_delayed_work_sync(&inst->unstall_work);
+
 	return wave5_vpu_release_device(filp, wave5_vpu_dec_close, "decoder");
 }
 
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 2d515c9028a5..28df7f19c459 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -9,6 +9,7 @@
 #define VPUAPI_H_INCLUDED
 
 #include <linux/kfifo.h>
+#include <linux/workqueue.h>
 #include <linux/idr.h>
 #include <linux/genalloc.h>
 #include <media/v4l2-device.h>
-- 
2.43.0

[PATCH v2 7/9] media: chips-media: wave5: stamp decoded pictures from a decode-order queue

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

The VPU takes more than one picture command at a time, so one completion
can report a read pointer that has already passed several access units.
wave5_handle_src_buffer() then completes several source buffers in a
single call, and inst->timestamp keeps only the last of them, so every
earlier timestamp is overwritten before any picture is stamped with it.

Userspace carries its frame number in that timestamp. A number that
never comes back leaves its frame pending forever; GStreamer reports it
as "Too old frames, bug in decoder" once a hundred have accumulated.

Queue the timestamps in decode order and take one per decoded picture.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Tested-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../chips-media/wave5/wave5-vpu-dec.c         | 69 ++++++++++++++++++-
 .../platform/chips-media/wave5/wave5-vpuapi.h | 10 +++
 2 files changed, 78 insertions(+), 1 deletion(-)

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 668c923dfa36..e351a86528bd 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -200,6 +200,59 @@ static bool wave5_last_src_buffer_consumed(struct v4l2_m2m_ctx *m2m_ctx)
 	return vpu_buf->consumed;
 }
 
+/*
+ * The decode-order timestamp queue.
+ *
+ * GStreamer carries its frame number in tv_sec of the OUTPUT timestamp and
+ * reads it back off the CAPTURE buffer, so every source buffer's timestamp has
+ * to reach exactly one decoded picture.
+ *
+ * A scalar cannot do that. The VPU accepts more than one picture command at a
+ * time, so a single completion can report a read pointer that has already
+ * passed several access units; wave5_handle_src_buffer() then completes
+ * several source buffers in one call, and all but the last timestamp is
+ * overwritten before any picture is stamped with it. That number never comes
+ * back, and the frame it belongs to is left pending in userspace forever --
+ * observed as GStreamer's "Too old frames, bug in decoder" warning once a
+ * hundred of them have piled up.
+ *
+ * Queue them instead. Picture completions arrive in decode order, which is the
+ * order the source buffers were consumed in.
+ */
+static void wave5_ts_reset(struct vpu_instance *inst)
+{
+	inst->time_stamp.head = 0;
+	inst->time_stamp.tail = 0;
+}
+
+static void wave5_ts_push(struct vpu_instance *inst, u64 ts)
+{
+	int next = (inst->time_stamp.head + 1) % MAX_TIMESTAMP_CIR_BUF;
+
+	/*
+	 * Full means pictures are not being reported for the data we feed, so
+	 * the oldest entry has nothing left to claim it. Drop it rather than
+	 * refuse the new one, which would shift every timestamp from here on.
+	 */
+	if (next == inst->time_stamp.tail)
+		inst->time_stamp.tail = (inst->time_stamp.tail + 1) %
+					MAX_TIMESTAMP_CIR_BUF;
+
+	inst->time_stamp.buf[inst->time_stamp.head] = ts;
+	inst->time_stamp.head = next;
+}
+
+static bool wave5_ts_pop(struct vpu_instance *inst, u64 *ts)
+{
+	if (inst->time_stamp.head == inst->time_stamp.tail)
+		return false;
+
+	*ts = inst->time_stamp.buf[inst->time_stamp.tail];
+	inst->time_stamp.tail = (inst->time_stamp.tail + 1) %
+				MAX_TIMESTAMP_CIR_BUF;
+	return true;
+}
+
 static void wave5_handle_src_buffer(struct vpu_instance *inst, dma_addr_t rd_ptr)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
@@ -232,6 +285,7 @@ static void wave5_handle_src_buffer(struct vpu_instance *inst, dma_addr_t rd_ptr
 			__func__, src_buf->vb2_buf.index);
 		src_buf = v4l2_m2m_src_buf_remove(m2m_ctx);
 		inst->timestamp = src_buf->vb2_buf.timestamp;
+		wave5_ts_push(inst, src_buf->vb2_buf.timestamp);
 		v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
 		consumed_bytes -= src_size;
 
@@ -421,8 +475,18 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
 		struct vb2_buffer *vb = vb2_get_buffer(dst_vq,
 						       dec_info.index_frame_decoded);
 		if (vb) {
+			u64 ts;
+
 			dec_buf = to_vb2_v4l2_buffer(vb);
-			dec_buf->vb2_buf.timestamp = inst->timestamp;
+			/*
+			 * Empty means this picture came out of data that was
+			 * already accounted for. Fall back to the last timestamp
+			 * seen rather than leave the buffer unstamped.
+			 */
+			if (!wave5_ts_pop(inst, &ts))
+				ts = inst->timestamp;
+
+			dec_buf->vb2_buf.timestamp = ts;
 		} else {
 			dev_warn(inst->dev->dev, "%s: invalid decoded frame index %i",
 				 __func__, dec_info.index_frame_decoded);
@@ -1528,6 +1592,9 @@ static int streamoff_output(struct vb2_queue *q)
 	 */
 	inst->remaining_consumed_bytes = 0;
 
+	/* The queued timestamps belong to those buffers as well. */
+	wave5_ts_reset(inst);
+
 	if (v4l2_m2m_has_stopped(m2m_ctx)) {
 		unsigned long flags;
 
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 28df7f19c459..d916acce462d 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -781,6 +781,15 @@ struct vpu_instance_ops {
 	void (*finish_process)(struct vpu_instance *inst);
 };
 
+#define MAX_TIMESTAMP_CIR_BUF	30
+
+/* OUTPUT timestamps in decode order, one per consumed source buffer */
+struct timestamp_circ_buf {
+	u64 buf[MAX_TIMESTAMP_CIR_BUF];
+	int head;
+	int tail;
+};
+
 struct vpu_instance {
 	struct list_head list;
 	struct v4l2_fh v4l2_fh;
@@ -817,6 +826,7 @@ struct vpu_instance {
 	struct list_head avail_dst_bufs;
 	struct v4l2_rect conf_win;
 	u64 timestamp;
+	struct timestamp_circ_buf time_stamp;
 	enum frame_buffer_format output_format;
 	bool cbcr_interleave;
 	bool nv21;
-- 
2.43.0

[PATCH v2 8/9] media: chips-media: wave5: restore the display flags after a flush

[Jackson.lee]

From: Jackson Lee <jackson.lee@chipsnmedia.com>

Flushing an instance clears the display flags, so every frame buffer looks
free again -- including the ones userspace is still holding.
streamoff_output() marks only the buffers that happen to be queued when it
runs, and userspace clears those as it re-queues them, so the buffers it
kept end up unmarked.

The next pictures are then decoded straight into buffers the application
owns. finish_decode() cannot find them in the queue and warns:

  wave5_vpu_dec_finish_decode: invalid display frame index N

A trace of the flags across a seek shows it plainly: at streamoff the flags
are 0x73, only the two queued buffers are marked again, userspace re-queues
five of seven buffers and the flags reach 0 -- and the two it kept are
decoded into 8 and 12 ms later.

Re-establish the flags once the flush is done, from the buffers' own
states: mark everything the driver does not own, hand back only what it
does.

Derive that from the vb2 buffers rather than from the m2m ready queue. The
earlier per-buffer loops walked that list with v4l2_m2m_for_each_dst_buf(),
which takes no lock, while the interrupt thread unlinks entries from it in
v4l2_m2m_dst_buf_remove_by_idx(); walking it here oopsed on a poisoned list
pointer:

  Unable to handle kernel paging request at virtual address
  deacfffffffffcd8
  pc : streamoff_output+0xe4/0x294 [wave5]
  Call trace:
   streamoff_output+0xe4/0x294 [wave5]
   wave5_vpu_dec_stop_streaming+0x244/0x2b4 [wave5]
   __vb2_queue_cancel+0x30/0x278 [videobuf2_common]

Indexing the vb2 queue directly avoids the list entirely.

With a flushing seek every 2 s for 180 s through glimagesink, three runs
each: 4, 4, 4 of those warnings before and 0, 0, 0 after, with no oops.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Tested-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../chips-media/wave5/wave5-vpu-dec.c         | 77 +++++++++++++++----
 1 file changed, 60 insertions(+), 17 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 e351a86528bd..102c07979d08 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1087,6 +1087,45 @@ static int wave5_vpu_dec_queue_setup(struct vb2_queue *q, unsigned int *num_buff
 	return 0;
 }
 
+/*
+ * Re-establish the display flags from the buffers' own states: the VPU may only
+ * write into a frame buffer the driver owns. Everything else -- above all the
+ * buffers userspace is holding -- has to stay marked.
+ *
+ * Walk the vb2 buffers rather than the m2m ready queue. That list is modified
+ * from the interrupt thread (v4l2_m2m_dst_buf_remove_by_idx() in
+ * finish_decode()) and v4l2_m2m_for_each_dst_buf() takes no lock, so iterating
+ * it here can land on an entry that was just unlinked.
+ */
+static void wave5_dec_sync_disp_flags(struct vpu_instance *inst, bool mark_all)
+{
+	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
+	struct vb2_queue *dst_vq = v4l2_m2m_get_dst_vq(m2m_ctx);
+	unsigned int i;
+
+	for (i = 0; i < vb2_get_num_buffers(dst_vq); i++) {
+		struct vb2_buffer *vb = vb2_get_buffer(dst_vq, i);
+		bool driver_owns;
+		int ret;
+
+		if (!vb)
+			continue;
+
+		driver_owns = !mark_all &&
+			      (vb->state == VB2_BUF_STATE_QUEUED ||
+			       vb->state == VB2_BUF_STATE_ACTIVE);
+
+		if (driver_owns)
+			ret = wave5_vpu_dec_clr_disp_flag(inst, vb->index);
+		else
+			ret = wave5_vpu_dec_set_disp_flag(inst, vb->index);
+		if (ret)
+			dev_dbg(inst->dev->dev,
+				"%s: %s display flag of buf index: %u, fail: %d\n",
+				__func__, driver_owns ? "Clearing" : "Setting", i, ret);
+	}
+}
+
 static int wave5_prepare_fb(struct vpu_instance *inst)
 {
 	int linear_num;
@@ -1541,7 +1580,6 @@ static int streamoff_output(struct vb2_queue *q)
 	int ret;
 	dma_addr_t new_rd_ptr;
 	struct dec_output_info dec_info;
-	unsigned int i;
 	struct vpu_src_buffer *vpu_buf, *tmp;
 
 	inst->retry = false;
@@ -1550,14 +1588,6 @@ static int streamoff_output(struct vb2_queue *q)
 	list_for_each_entry_safe(vpu_buf, tmp, &inst->avail_src_bufs, list)
 		list_del_init(&vpu_buf->list);
 
-	for (i = 0; i < v4l2_m2m_num_dst_bufs_ready(m2m_ctx); i++) {
-		ret = wave5_vpu_dec_set_disp_flag(inst, i);
-		if (ret)
-			dev_dbg(inst->dev->dev,
-				"%s: Setting display flag of buf index: %u, fail: %d\n",
-				__func__, i, ret);
-	}
-
 	while ((buf = v4l2_m2m_src_buf_remove(m2m_ctx))) {
 		dev_dbg(inst->dev->dev, "%s: (Multiplanar) buf type %4u | index %4u\n",
 			__func__, buf->vb2_buf.type, buf->vb2_buf.index);
@@ -1574,6 +1604,20 @@ static int streamoff_output(struct vb2_queue *q)
 	if (ret)
 		return ret;
 
+	/*
+	 * The flush clears the display flags, so every frame buffer now looks
+	 * free -- including the ones userspace is still holding. Only the
+	 * buffers that happened to be queued when the flush ran were marked
+	 * again, and userspace clears those as it re-queues them, which leaves
+	 * the ones it kept unmarked: the next pictures are decoded straight
+	 * into buffers the application owns and finish_decode() cannot find
+	 * them in the queue any more.
+	 *
+	 * Mark everything, then hand back just what is queued, the same way
+	 * wave5_prepare_fb() establishes the flags in the first place.
+	 */
+	wave5_dec_sync_disp_flags(inst, false);
+
 	/* Reset the ring buffer information */
 	new_rd_ptr = wave5_vpu_dec_get_rd_ptr(inst);
 	inst->last_rd_ptr = new_rd_ptr;
@@ -1614,16 +1658,15 @@ static int streamoff_capture(struct vb2_queue *q)
 	struct vpu_instance *inst = vb2_get_drv_priv(q);
 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
 	struct vb2_v4l2_buffer *buf;
-	unsigned int i;
 	int ret = 0;
 
-	for (i = 0; i < v4l2_m2m_num_dst_bufs_ready(m2m_ctx); i++) {
-		ret = wave5_vpu_dec_set_disp_flag(inst, i);
-		if (ret)
-			dev_dbg(inst->dev->dev,
-				"%s: Setting display flag of buf index: %u, fail: %d\n",
-				__func__, i, ret);
-	}
+	/*
+	 * Every CAPTURE buffer is about to go back to userspace, so none of them
+	 * may be decoded into until it is queued again. wave5_prepare_fb() only
+	 * re-establishes the flags on the INIT_SEQ -> PIC_RUN transition, which a
+	 * plain streamoff/streamon of this queue does not go through.
+	 */
+	wave5_dec_sync_disp_flags(inst, true);
 
 	while ((buf = v4l2_m2m_dst_buf_remove(m2m_ctx))) {
 		u32 plane;
-- 
2.43.0

[PATCH v2 9/9] media: chips-media: wave5: Stop FrameBuf Reset During Seek

[Jackson.lee]

From: Brandon Brnich <b-brnich@ti.com>

A seek drains the capture queue via stop_streaming without stopping the
m2m_ctx. streamoff_capture checks needs_reallocation which will always
be true after the first sequence initialization. This will trigger a
DEC_RESET_FRAMEBUF_INFO which will go through and deallocate all of the
frame and auxiliary buffers.

Video output can have corruption if the firmware is still referencing the
buffers that just got reset. Clear needs_reallocation in wave5_prepare_fb
once the new buffers are registered.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c | 2 ++
 1 file changed, 2 insertions(+)

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 102c07979d08..57511acbb28b 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1250,6 +1250,8 @@ static int wave5_prepare_fb(struct vpu_instance *inst)
 		return ret;
 	}
 
+	inst->needs_reallocation = false;
+
 	/*
 	 * Mark all frame buffers as out of display, to avoid using them before
 	 * the application have them queued.
-- 
2.43.0