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

[Jackson.lee]

From: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx
Signed-off-by: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
---
 .../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 eae738df270e..e52853e89931 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1088,6 +1088,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;
@@ -1542,7 +1581,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;
@@ -1551,14 +1589,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);
@@ -1575,6 +1605,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;
@@ -1615,16 +1659,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