[PATCH v4 0/7] media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail

[PATCH v4 0/7] media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail

[Yunfei Dong]

v4l2_m2m_buf_done is called in lat work queue, v4l2_ctrl_request_complete
is called in core queue. The request status of output queue will be set to
MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called, leading to
output queue request complete fail. Must move v4l2_ctrl_request_complete
in front of v4l2_m2m_buf_done.

Patch 1 setting request complete before buffer done
Patch 2 change flush decode order when stream off
Patch 3 flush decoder before stream off
Patch 4 using input information to get vb2 buffer
Patch 5 store source vb2 buffer
Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
Patch 7 remove media request checking

---
compared with v3:
- fix flush decoder issue when userspace stream off capture queue firstly
- fluster test result same with v3

compared with v2:
- add patch 5/6/7 to fix decode again issue
- add fluster test result with mt8195 platform(same with no changed):
  1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
     VP8-TEST-VECTORS 59/61
  2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
     VP9-TEST-VECTORS 276/305
  3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
     AV1-TEST-VECTORS 237/239
  4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
     JVT-AVC_V1       95/135
  5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
     JCT-VC-HEVC_V1   142/147

compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (7):
  media: mediatek: vcodec: setting request complete before buffer done
  media: mediatek: vcodec: change flush decode order when stream off
  media: mediatek: vcodec: flush decoder before stream off
  media: mediatek: vcodec: using input information to get vb2 buffer
  media: mediatek: vcodec: store source vb2 buffer
  media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
    v4l2_m2m_src_buf_remove
  media: mediatek: vcodec: remove media request checking

 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 44 ++++++++---------
 .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  4 +-
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-----
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
 .../decoder/vdec/vdec_h264_req_multi_if.c     |  4 +-
 .../decoder/vdec/vdec_hevc_req_multi_if.c     |  4 +-
 .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
 .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  4 +-
 8 files changed, 85 insertions(+), 60 deletions(-)

-- 
2.46.0

[PATCH v4 1/7] media: mediatek: vcodec: setting request complete before buffer done

[Yunfei Dong]

The request status of output queue is set to MEDIA_REQUEST_STATE_COMPLETE
when user space dequeue output buffer. Will get below warning if the
driver calling v4l2_ctrl_request_complete to set media request complete,
must to change the function order, calling v4l2_ctrl_request_complete
before v4l2_m2m_buf_done.

Workqueue: core-decoder vdec_msg_queue_core_work [mtk_vcodec_dec]
pstate: 80c00089 (Nzcv daIf +PAN +UAO -TCO BTYPE=--)
pc : media_request_object_bind+0xa8/0x124
lr : media_request_object_bind+0x50/0x124
sp : ffffffc011393be0
x29: ffffffc011393be0 x28: 0000000000000000
x27: ffffff890c280248 x26: ffffffe21a71ab88
x25: 0000000000000000 x24: ffffff890c280280
x23: ffffff890c280280 x22: 00000000fffffff0
x21: 0000000000000000 x20: ffffff890260d280
x19: ffffff890260d2e8 x18: 0000000000001000
x17: 0000000000000400 x16: ffffffe21a4584a0
x15: 000000000053361d x14: 0000000000000018
x13: 0000000000000004 x12: ffffffa82427d000
x11: ffffffe21ac3fce0 x10: 0000000000000001
x9 : 0000000000000000 x8 : 0000000000000003
x7 : 0000000000000000 x6 : 000000000000003f
x5 : 0000000000000040 x4 : ffffff89052e7b98
x3 : 0000000000000000 x2 : 0000000000000001
x1 : 0000000000000000 x0 : 0000000000000000
Call trace:
 media_request_object_bind+0xa8/0x124
 v4l2_ctrl_request_bind+0xc4/0x168
 v4l2_ctrl_request_complete+0x198/0x1f4
 mtk_vdec_stateless_cap_to_disp+0x58/0x8c [mtk_vcodec_dec 245a7c1e48ff1b2451a50e1dfcb174262b6b462c]
 vdec_vp9_slice_core_decode+0x1c0/0x268 [mtk_vcodec_dec 245a7c1e48ff1b2451a50e1dfcb174262b6b462c]
 vdec_msg_queue_core_work+0x60/0x11c [mtk_vcodec_dec 245a7c1e48ff1b2451a50e1dfcb174262b6b462c]
 process_one_work+0x140/0x480
 worker_thread+0x12c/0x2f8
 kthread+0x13c/0x1d8
 ret_from_fork+0x10/0x30

Fixes: 7b182b8d9c852 ("media: mediatek: vcodec: Refactor get and put capture buffer flow")
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c        |  4 ++--
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h    |  2 +-
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c       | 13 ++++++++-----
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c       |  7 ++++---
 .../vcodec/decoder/vdec/vdec_h264_req_multi_if.c    |  4 ++--
 .../vcodec/decoder/vdec/vdec_hevc_req_multi_if.c    |  4 ++--
 .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c       |  6 +++---
 .../mediatek/vcodec/decoder/vdec_msg_queue.h        |  4 ++--
 8 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
index 98838217b97d..2b787e60a1f9 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -887,10 +887,10 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 			if (src_buf != &ctx->empty_flush_buf.vb) {
 				struct media_request *req =
 					src_buf->vb2_buf.req_obj.req;
-				v4l2_m2m_buf_done(src_buf,
-						VB2_BUF_STATE_ERROR);
+
 				if (req)
 					v4l2_ctrl_request_complete(req, &ctx->ctrl_hdl);
+				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
 			}
 		}
 		return;
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
index ac568ed14fa2..1fabe8c5b7a4 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
@@ -111,7 +111,7 @@ struct mtk_vcodec_dec_pdata {
 	int (*flush_decoder)(struct mtk_vcodec_dec_ctx *ctx);
 	struct vdec_fb *(*get_cap_buffer)(struct mtk_vcodec_dec_ctx *ctx);
 	void (*cap_to_disp)(struct mtk_vcodec_dec_ctx *ctx, int error,
-			    struct media_request *src_buf_req);
+			    struct vb2_v4l2_buffer *vb2_v4l2_src);
 
 	const struct vb2_ops *vdec_vb2_ops;
 
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index b903e39fee89..2a7e4fe24ed3 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -245,10 +245,11 @@ static const struct v4l2_frmsize_stepwise stepwise_fhd = {
 };
 
 static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int error,
-					   struct media_request *src_buf_req)
+					   struct vb2_v4l2_buffer *vb2_v4l2_src)
 {
 	struct vb2_v4l2_buffer *vb2_dst;
 	enum vb2_buffer_state state;
+	struct media_request *src_buf_req;
 
 	if (error)
 		state = VB2_BUF_STATE_ERROR;
@@ -264,8 +265,12 @@ static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int e
 		mtk_v4l2_vdec_err(ctx, "dst buffer is NULL");
 	}
 
+	src_buf_req = vb2_v4l2_src->vb2_buf.req_obj.req;
 	if (src_buf_req)
 		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+
+	if (vb2_v4l2_src)
+		v4l2_m2m_buf_done(vb2_v4l2_src, state);
 }
 
 static struct vdec_fb *vdec_get_cap_buffer(struct mtk_vcodec_dec_ctx *ctx)
@@ -374,14 +379,12 @@ static void mtk_vdec_worker(struct work_struct *work)
 	state = ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE;
 	if (!IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch) ||
 	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
-		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
 		if (src_buf_req)
 			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
 	} else {
-		if (ret != -EAGAIN) {
+		if (ret != -EAGAIN)
 			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
-			v4l2_m2m_buf_done(vb2_v4l2_src, state);
-		}
 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 	}
 }
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
index bf21f2467a0f..90217cc8e242 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
@@ -1071,7 +1071,8 @@ static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_instance
 	if (!src)
 		return -EINVAL;
 
-	lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
+	lat_buf->vb2_v4l2_src = src;
+
 	dst = &lat_buf->ts_info;
 	v4l2_m2m_buf_copy_metadata(src, dst, true);
 	vsi->frame.cur_ts = dst->vb2_buf.timestamp;
@@ -2195,7 +2196,7 @@ static int vdec_av1_slice_core_decode(struct vdec_lat_buf *lat_buf)
 		       &instance->core_vsi->trans.dma_addr_end);
 	vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, instance->core_vsi->trans.dma_addr_end);
 
-	ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf->src_buf_req);
+	ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf->vb2_v4l2_src);
 
 	return 0;
 
@@ -2204,7 +2205,7 @@ static int vdec_av1_slice_core_decode(struct vdec_lat_buf *lat_buf)
 	vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr_end);
 
 	if (fb)
-		ctx->dev->vdec_pdata->cap_to_disp(ctx, 1, lat_buf->src_buf_req);
+		ctx->dev->vdec_pdata->cap_to_disp(ctx, 1, lat_buf->vb2_v4l2_src);
 
 	return ret;
 }
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
index 2d4611e7fa0b..2aac6167f893 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
@@ -533,7 +533,7 @@ static int vdec_h264_slice_core_decode(struct vdec_lat_buf *lat_buf)
 
 vdec_dec_end:
 	vdec_msg_queue_update_ube_rptr(&lat_buf->ctx->msg_queue, share_info->trans_end);
-	ctx->dev->vdec_pdata->cap_to_disp(ctx, !!err, lat_buf->src_buf_req);
+	ctx->dev->vdec_pdata->cap_to_disp(ctx, !!err, lat_buf->vb2_v4l2_src);
 	mtk_vdec_debug(ctx, "core decode done err=%d", err);
 	ctx->decoded_frame_cnt++;
 	return 0;
@@ -605,7 +605,7 @@ static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
 	}
 
 	inst->vsi->dec.nal_info = buf[nal_start_idx];
-	lat_buf->src_buf_req = src_buf_info->m2m_buf.vb.vb2_buf.req_obj.req;
+	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
 	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
 
 	err = vdec_h264_slice_fill_decode_parameters(inst, share_info);
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c
index aa721cc43647..f6f9f7de0005 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_hevc_req_multi_if.c
@@ -741,7 +741,7 @@ static int vdec_hevc_slice_setup_lat_buffer(struct vdec_hevc_slice_inst *inst,
 	inst->vsi->bs.size = bs->size;
 
 	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
-	lat_buf->src_buf_req = src_buf_info->m2m_buf.vb.vb2_buf.req_obj.req;
+	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
 	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
 
 	*res_chg = inst->resolution_changed;
@@ -961,7 +961,7 @@ static int vdec_hevc_slice_core_decode(struct vdec_lat_buf *lat_buf)
 
 vdec_dec_end:
 	vdec_msg_queue_update_ube_rptr(&lat_buf->ctx->msg_queue, share_info->trans.dma_addr_end);
-	ctx->dev->vdec_pdata->cap_to_disp(ctx, !!err, lat_buf->src_buf_req);
+	ctx->dev->vdec_pdata->cap_to_disp(ctx, !!err, lat_buf->vb2_v4l2_src);
 	mtk_vdec_debug(ctx, "core decode done err=%d", err);
 	ctx->decoded_frame_cnt++;
 	return 0;
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
index eea709d93820..3dceb668ba1c 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
@@ -721,7 +721,7 @@ static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_vp9_slice_instance
 	if (!src)
 		return -EINVAL;
 
-	lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
+	lat_buf->vb2_v4l2_src = src;
 
 	dst = &lat_buf->ts_info;
 	v4l2_m2m_buf_copy_metadata(src, dst, true);
@@ -2187,7 +2187,7 @@ static int vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf)
 	mtk_vdec_debug(ctx, "core dma_addr_end 0x%lx\n",
 		       (unsigned long)pfc->vsi.trans.dma_addr_end);
 	vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr_end);
-	ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf->src_buf_req);
+	ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf->vb2_v4l2_src);
 
 	return 0;
 
@@ -2197,7 +2197,7 @@ static int vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf)
 		vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr_end);
 
 		if (fb)
-			ctx->dev->vdec_pdata->cap_to_disp(ctx, 1, lat_buf->src_buf_req);
+			ctx->dev->vdec_pdata->cap_to_disp(ctx, 1, lat_buf->vb2_v4l2_src);
 	}
 	return ret;
 }
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.h b/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.h
index b0f576867f4b..9781de35df4b 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.h
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.h
@@ -55,7 +55,7 @@ struct vdec_msg_queue_ctx {
  * @rd_mv_addr:	mv addr for av1 lat hardware output, core hardware input
  * @tile_addr:	tile buffer for av1 core input
  * @ts_info: need to set timestamp from output to capture
- * @src_buf_req: output buffer media request object
+ * @vb2_v4l2_src: vb2 buffer of output queue
  *
  * @private_data: shared information used to lat and core hardware
  * @ctx: mtk vcodec context information
@@ -71,7 +71,7 @@ struct vdec_lat_buf {
 	struct mtk_vcodec_mem rd_mv_addr;
 	struct mtk_vcodec_mem tile_addr;
 	struct vb2_v4l2_buffer ts_info;
-	struct media_request *src_buf_req;
+	struct vb2_v4l2_buffer *vb2_v4l2_src;
 
 	void *private_data;
 	struct mtk_vcodec_dec_ctx *ctx;
-- 
2.46.0

[PATCH v4 2/7] media: mediatek: vcodec: change flush decode order when stream off

[Yunfei Dong]

The buffer remove and buffer done of output queue is separated into
two works, the value of owned_by_drv_count isn't zero when output
queue stream off before flush decode.
Changing the flush decode from capture to output when stream off to
make sure all the output queue buffers are set to done list.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 45 +++++++++----------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
index 2b787e60a1f9..7080ca3e18b0 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -893,32 +893,31 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
 			}
 		}
-		return;
-	}
-
-	if (ctx->state >= MTK_STATE_HEADER) {
-
-		/* Until STREAMOFF is called on the CAPTURE queue
-		 * (acknowledging the event), the driver operates
-		 * as if the resolution hasn't changed yet, i.e.
-		 * VIDIOC_G_FMT< etc. return previous resolution.
-		 * So we update picinfo here
-		 */
-		ctx->picinfo = ctx->last_decoded_picinfo;
 
-		mtk_v4l2_vdec_dbg(2, ctx,
-				  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
-				  ctx->id, ctx->last_decoded_picinfo.pic_w,
-				  ctx->last_decoded_picinfo.pic_h,
-				  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
-				  ctx->last_decoded_picinfo.buf_w,
-				  ctx->last_decoded_picinfo.buf_h);
+		if (ctx->state >= MTK_STATE_HEADER) {
+			/*
+			 * The resolution hasn't been changed when STREAMOFF is called.
+			 * Update the picinfo here with previous resolution if VIDIOC_G_FMT
+			 * is called.
+			 */
+			ctx->picinfo = ctx->last_decoded_picinfo;
+
+			mtk_v4l2_vdec_dbg(2, ctx,
+					  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
+					  ctx->id, ctx->last_decoded_picinfo.pic_w,
+					  ctx->last_decoded_picinfo.pic_h,
+					  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
+					  ctx->last_decoded_picinfo.buf_w,
+					  ctx->last_decoded_picinfo.buf_h);
+
+			ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
+			if (ret)
+				mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
+		}
 
-		ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
-		if (ret)
-			mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
+		ctx->state = MTK_STATE_FLUSH;
+		return;
 	}
-	ctx->state = MTK_STATE_FLUSH;
 
 	while ((dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx))) {
 		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
-- 
2.46.0

[PATCH v4 3/7] media: mediatek: vcodec: flush decoder before stream off

[Yunfei Dong]

Flush decoder will reset the driver to flush status. If lat or core
work queue in active before flush when stream off, will lead to get
dst buffer NULL or buff done with one non-existent source buffer.

Flush decoder when stream off no matter output or capture queue
calling stream off firstly.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 45 ++++++++++---------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
index 7080ca3e18b0..fc4ee1fb7cd1 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -882,6 +882,29 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 	mtk_v4l2_vdec_dbg(3, ctx, "[%d] (%d) state=(%x) ctx->decoded_frame_cnt=%d",
 			  ctx->id, q->type, ctx->state, ctx->decoded_frame_cnt);
 
+	if (ctx->state >= MTK_STATE_HEADER && ctx->state != MTK_STATE_FLUSH) {
+		/*
+		 * The resolution hasn't been changed when STREAMOFF is called.
+		 * Update the picinfo here with previous resolution if VIDIOC_G_FMT
+		 * is called.
+		 */
+		ctx->picinfo = ctx->last_decoded_picinfo;
+
+		mtk_v4l2_vdec_dbg(2, ctx,
+				  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
+				  ctx->id, ctx->last_decoded_picinfo.pic_w,
+				  ctx->last_decoded_picinfo.pic_h,
+				  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
+				  ctx->last_decoded_picinfo.buf_w,
+				  ctx->last_decoded_picinfo.buf_h);
+
+		ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
+		if (ret)
+			mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
+
+		ctx->state = MTK_STATE_FLUSH;
+	}
+
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx))) {
 			if (src_buf != &ctx->empty_flush_buf.vb) {
@@ -894,28 +917,6 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 			}
 		}
 
-		if (ctx->state >= MTK_STATE_HEADER) {
-			/*
-			 * The resolution hasn't been changed when STREAMOFF is called.
-			 * Update the picinfo here with previous resolution if VIDIOC_G_FMT
-			 * is called.
-			 */
-			ctx->picinfo = ctx->last_decoded_picinfo;
-
-			mtk_v4l2_vdec_dbg(2, ctx,
-					  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
-					  ctx->id, ctx->last_decoded_picinfo.pic_w,
-					  ctx->last_decoded_picinfo.pic_h,
-					  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
-					  ctx->last_decoded_picinfo.buf_w,
-					  ctx->last_decoded_picinfo.buf_h);
-
-			ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
-			if (ret)
-				mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
-		}
-
-		ctx->state = MTK_STATE_FLUSH;
 		return;
 	}
 
-- 
2.46.0

[PATCH v4 4/7] media: mediatek: vcodec: using input information to get vb2 buffer

[Yunfei Dong]

vb2 buffer may be removed from ready list when lat try to get next
src buffer, leading to vb2 buffer not the current one. Need to get
vb2 buffer according to current input memory information.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c     | 13 +++++++------
 .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c     | 15 +++++++--------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
index 90217cc8e242..a744740ba5f1 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
@@ -1062,19 +1062,20 @@ static inline void vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
 
 static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_instance *instance,
 						 struct vdec_av1_slice_vsi *vsi,
+						 struct mtk_vcodec_mem *bs,
 						 struct vdec_lat_buf *lat_buf)
 {
-	struct vb2_v4l2_buffer *src;
+	struct mtk_video_dec_buf *src_buf_info;
 	struct vb2_v4l2_buffer *dst;
 
-	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
-	if (!src)
+	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
+	if (!src_buf_info)
 		return -EINVAL;
 
-	lat_buf->vb2_v4l2_src = src;
+	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
 
 	dst = &lat_buf->ts_info;
-	v4l2_m2m_buf_copy_metadata(src, dst, true);
+	v4l2_m2m_buf_copy_metadata(lat_buf->vb2_v4l2_src, dst, true);
 	vsi->frame.cur_ts = dst->vb2_buf.timestamp;
 
 	return 0;
@@ -1724,7 +1725,7 @@ static int vdec_av1_slice_setup_lat(struct vdec_av1_slice_instance *instance,
 	struct vdec_av1_slice_vsi *vsi = &pfc->vsi;
 	int ret;
 
-	ret = vdec_av1_slice_setup_lat_from_src_buf(instance, vsi, lat_buf);
+	ret = vdec_av1_slice_setup_lat_from_src_buf(instance, vsi, bs, lat_buf);
 	if (ret)
 		return ret;
 
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
index 3dceb668ba1c..c50a454ab4f7 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
@@ -712,19 +712,18 @@ int vdec_vp9_slice_setup_single_from_src_to_dst(struct vdec_vp9_slice_instance *
 }
 
 static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_vp9_slice_instance *instance,
+						 struct mtk_vcodec_mem *bs,
 						 struct vdec_lat_buf *lat_buf)
 {
-	struct vb2_v4l2_buffer *src;
-	struct vb2_v4l2_buffer *dst;
+	struct mtk_video_dec_buf *src_buf_info;
 
-	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
-	if (!src)
+	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
+	if (!src_buf_info)
 		return -EINVAL;
 
-	lat_buf->vb2_v4l2_src = src;
+	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
 
-	dst = &lat_buf->ts_info;
-	v4l2_m2m_buf_copy_metadata(src, dst, true);
+	v4l2_m2m_buf_copy_metadata(lat_buf->vb2_v4l2_src, &lat_buf->ts_info, true);
 	return 0;
 }
 
@@ -1154,7 +1153,7 @@ static int vdec_vp9_slice_setup_lat(struct vdec_vp9_slice_instance *instance,
 	struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
 	int ret;
 
-	ret = vdec_vp9_slice_setup_lat_from_src_buf(instance, lat_buf);
+	ret = vdec_vp9_slice_setup_lat_from_src_buf(instance, bs, lat_buf);
 	if (ret)
 		goto err;
 
-- 
2.46.0

[PATCH v4 5/7] media: mediatek: vcodec: store source vb2 buffer

[Yunfei Dong]

Store the current vb2 buffer when lat need to decode again.
Then lat work can get the same vb2 buffer directly next time.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h      |  2 ++
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c         | 11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
index 1fabe8c5b7a4..0817be73f8e0 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
@@ -155,6 +155,7 @@ struct mtk_vcodec_dec_pdata {
  * @last_decoded_picinfo: pic information get from latest decode
  * @empty_flush_buf: a fake size-0 capture buffer that indicates flush. Used
  *		     for stateful decoder.
+ * @last_vb2_v4l2_src: the backup of current source buffer.
  * @is_flushing: set to true if flushing is in progress.
  *
  * @current_codec: current set input codec, in V4L2 pixel format
@@ -201,6 +202,7 @@ struct mtk_vcodec_dec_ctx {
 	struct work_struct decode_work;
 	struct vdec_pic_info last_decoded_picinfo;
 	struct v4l2_m2m_buffer empty_flush_buf;
+	struct vb2_v4l2_buffer *last_vb2_v4l2_src;
 	bool is_flushing;
 
 	u32 current_codec;
diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index 2a7e4fe24ed3..8aa379872ddc 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -320,7 +320,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 	struct mtk_vcodec_dec_ctx *ctx =
 		container_of(work, struct mtk_vcodec_dec_ctx, decode_work);
 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
-	struct vb2_v4l2_buffer *vb2_v4l2_src;
+	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;
 	struct vb2_buffer *vb2_src;
 	struct mtk_vcodec_mem *bs_src;
 	struct mtk_video_dec_buf *dec_buf_src;
@@ -329,7 +329,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 	bool res_chg = false;
 	int ret;
 
-	vb2_v4l2_src = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
+	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx);
 	if (!vb2_v4l2_src) {
 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source buffer", ctx->id);
@@ -383,8 +383,13 @@ static void mtk_vdec_worker(struct work_struct *work)
 			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
 		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
 	} else {
-		if (ret != -EAGAIN)
+		if (ret != -EAGAIN) {
 			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
+			ctx->last_vb2_v4l2_src = NULL;
+		} else {
+			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
+		}
+
 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 	}
 }
-- 
2.46.0

[PATCH v4 6/7] media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

[Yunfei Dong]

There isn't lock to protect source buffer when get next src buffer,
if the source buffer is removed for some unknown reason before lat
work queue execute done, will lead to remove source buffer or buffer
done error.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index 8aa379872ddc..3dba3549000a 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 		container_of(work, struct mtk_vcodec_dec_ctx, decode_work);
 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
 	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;
+	struct vb2_v4l2_buffer *vb2_v4l2_dst;
 	struct vb2_buffer *vb2_src;
 	struct mtk_vcodec_mem *bs_src;
 	struct mtk_video_dec_buf *dec_buf_src;
@@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 	bool res_chg = false;
 	int ret;
 
-	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx);
+	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
 	if (!vb2_v4l2_src) {
 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source buffer", ctx->id);
@@ -381,17 +382,28 @@ static void mtk_vdec_worker(struct work_struct *work)
 	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
 		if (src_buf_req)
 			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
-		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
-	} else {
-		if (ret != -EAGAIN) {
-			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
-			ctx->last_vb2_v4l2_src = NULL;
-		} else {
-			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
-		}
+		vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
+		v4l2_m2m_buf_done(vb2_v4l2_dst, state);
+		v4l2_m2m_buf_done(vb2_v4l2_src, state);
 
 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+		return;
 	}
+
+	/* If each codec return -EAGAIN to decode again, need to backup current source
+	 * buffer, then the driver will get this buffer next time.
+	 *
+	 * If each codec decode error, must to set buffer done with error status for
+	 * this buffer have been removed from ready list.
+	 */
+	ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src;
+	if (ret && ret != -EAGAIN) {
+		if (src_buf_req)
+			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+		v4l2_m2m_buf_done(vb2_v4l2_src, state);
+	}
+
+	v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
 }
 
 static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
-- 
2.46.0

[PATCH v4 7/7] media: mediatek: vcodec: remove media request checking

[Yunfei Dong]

Need to set the buffer status to error if the media request of
each source buffer is NULL, then schedule the work to process
again in case of access NULL pointer.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
index 3dba3549000a..43af18df03ea 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
@@ -359,10 +359,14 @@ static void mtk_vdec_worker(struct work_struct *work)
 			  ctx->id, bs_src->va, &bs_src->dma_addr, bs_src->size, vb2_src);
 	/* Apply request controls. */
 	src_buf_req = vb2_src->req_obj.req;
-	if (src_buf_req)
+	if (src_buf_req) {
 		v4l2_ctrl_request_setup(src_buf_req, &ctx->ctrl_hdl);
-	else
+	} else {
 		mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
+		v4l2_m2m_buf_done(vb2_v4l2_src, VB2_BUF_STATE_ERROR);
+		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+		return;
+	}
 
 	ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);
 	if (ret && ret != -EAGAIN) {
@@ -380,8 +384,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 	state = ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE;
 	if (!IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch) ||
 	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
-		if (src_buf_req)
-			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
 		vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
 		v4l2_m2m_buf_done(vb2_v4l2_dst, state);
 		v4l2_m2m_buf_done(vb2_v4l2_src, state);
@@ -398,8 +401,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 	 */
 	ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src;
 	if (ret && ret != -EAGAIN) {
-		if (src_buf_req)
-			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
+		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
 		v4l2_m2m_buf_done(vb2_v4l2_src, state);
 	}
 
-- 
2.46.0

Re: [PATCH v4 0/7] media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail

[Nicolas Dufresne]

Hi Yunfei,

Le mercredi 07 août 2024 à 16:24 +0800, Yunfei Dong a écrit :
> v4l2_m2m_buf_done is called in lat work queue, v4l2_ctrl_request_complete
> is called in core queue. The request status of output queue will be set to
> MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called, leading to
> output queue request complete fail. Must move v4l2_ctrl_request_complete
> in front of v4l2_m2m_buf_done.

Sebastian and I have analyzed further the issue and the description here does
not seem to match. What happens is that in Stateless decoding, you have to set
header controls out-of-request to negotiate the format at first.

With VP9 notably, the header control is the only control there is. Chromium will
optimize out this and only attach a bitstream buffer to the request. So when the
buffer is mark to done, it is the last object in the request, which implicitly
mark the request as complete.

When v4l2_ctrl_request_complete() is later called, the control code detect that
there is no controls in the request. It then creates an empty control, but
attaching an object to a completed request is not allowed.

> 
> Patch 1 setting request complete before buffer done
> Patch 2 change flush decode order when stream off
> Patch 3 flush decoder before stream off
> Patch 4 using input information to get vb2 buffer
> Patch 5 store source vb2 buffer
> Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
> Patch 7 remove media request checking

I will give a some testing soon. Can you clarify on if the LAT and the CORE
still runs in parallel after this change ?

Nicolas

> 
> ---
> compared with v3:
> - fix flush decoder issue when userspace stream off capture queue firstly
> - fluster test result same with v3
> 
> compared with v2:
> - add patch 5/6/7 to fix decode again issue
> - add fluster test result with mt8195 platform(same with no changed):
>   1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
>      VP8-TEST-VECTORS 59/61
>   2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
>      VP9-TEST-VECTORS 276/305
>   3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
>      AV1-TEST-VECTORS 237/239
>   4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
>      JVT-AVC_V1       95/135
>   5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
>      JCT-VC-HEVC_V1   142/147
> 
> compared with v1:
> - add patch 2/3/4 to fix timing issue.
> ---
> Yunfei Dong (7):
>   media: mediatek: vcodec: setting request complete before buffer done
>   media: mediatek: vcodec: change flush decode order when stream off
>   media: mediatek: vcodec: flush decoder before stream off
>   media: mediatek: vcodec: using input information to get vb2 buffer
>   media: mediatek: vcodec: store source vb2 buffer
>   media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
>     v4l2_m2m_src_buf_remove
>   media: mediatek: vcodec: remove media request checking
> 
>  .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 44 ++++++++---------
>  .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  4 +-
>  .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-----
>  .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
>  .../decoder/vdec/vdec_h264_req_multi_if.c     |  4 +-
>  .../decoder/vdec/vdec_hevc_req_multi_if.c     |  4 +-
>  .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
>  .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  4 +-
>  8 files changed, 85 insertions(+), 60 deletions(-)
> 

Re: [PATCH v4 0/7] media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail

[Yunfei Dong (董云飞)]

Hi Nicolas,

Thanks for your advice.

On Wed, 2024-08-07 at 09:08 -0400, Nicolas Dufresne wrote:
> > 

> Hi Yunfei,
> 
> Le mercredi 07 août 2024 à 16:24 +0800, Yunfei Dong a écrit :
> > v4l2_m2m_buf_done is called in lat work queue,
> > v4l2_ctrl_request_complete
> > is called in core queue. The request status of output queue will be
> > set to
> > MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called,
> > leading to
> > output queue request complete fail. Must move
> > v4l2_ctrl_request_complete
> > in front of v4l2_m2m_buf_done.
> 
> Sebastian and I have analyzed further the issue and the description
> here does
> not seem to match. What happens is that in Stateless decoding, you
> have to set
> header controls out-of-request to negotiate the format at first.
> 
> With VP9 notably, the header control is the only control there is.
> Chromium will
> optimize out this and only attach a bitstream buffer to the request.
> So when the
> buffer is mark to done, it is the last object in the request, which
> implicitly
> mark the request as complete.
> 
> When v4l2_ctrl_request_complete() is later called, the control code
> detect that
> there is no controls in the request. It then creates an empty
> control, but
> attaching an object to a completed request is not allowed.
> 

Whether I can write the commit message as below:

"User space will attach the syntaxes and bit-stream buffer to a media
request for stateless decoding, and the syntax controls are the only
v4l2 control request. The request will be marked to complete status
when the buffer is set to done, then request object will be cleaned
from media request.
 
When v4l2_ctrl_request_complete() is later called, the control request
detect that there is no controls in the request object. It then creates
an empty control request object, but attaching an object to a completed
request is not allowed."

> > Patch 1 setting request complete before buffer done
> > Patch 2 change flush decode order when stream off
> > Patch 3 flush decoder before stream off
> > Patch 4 using input information to get vb2 buffer
> > Patch 5 store source vb2 buffer
> > Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
> > Patch 7 remove media request checking
> 
> I will give a some testing soon. Can you clarify on if the LAT and
> the CORE
Have you already helped to do the fluster test?

> still runs in parallel after this change ?
> 
Yes, the driver can work in parallel.

Could you please help to review other patches?

> Nicolas
> 
Best Regards,
Yunfei Dong
> > 
> > ---
> > compared with v3:
> > - fix flush decoder issue when userspace stream off capture queue
> > firstly
> > - fluster test result same with v3
> > 
> > compared with v2:
> > - add patch 5/6/7 to fix decode again issue
> > - add fluster test result with mt8195 platform(same with no
> > changed):
> >   1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
> >      VP8-TEST-VECTORS 59/61
> >   2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
> >      VP9-TEST-VECTORS 276/305
> >   3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
> >      AV1-TEST-VECTORS 237/239
> >   4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
> >      JVT-AVC_V1       95/135
> >   5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
> >      JCT-VC-HEVC_V1   142/147
> > 
> > compared with v1:
> > - add patch 2/3/4 to fix timing issue.
> > ---
> > Yunfei Dong (7):
> >   media: mediatek: vcodec: setting request complete before buffer
> > done
> >   media: mediatek: vcodec: change flush decode order when stream
> > off
> >   media: mediatek: vcodec: flush decoder before stream off
> >   media: mediatek: vcodec: using input information to get vb2
> > buffer
> >   media: mediatek: vcodec: store source vb2 buffer
> >   media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
> >     v4l2_m2m_src_buf_remove
> >   media: mediatek: vcodec: remove media request checking
> > 
> >  .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 44 ++++++++-------
> > --
> >  .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  4 +-
> >  .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-
> > ----
> >  .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
> >  .../decoder/vdec/vdec_h264_req_multi_if.c     |  4 +-
> >  .../decoder/vdec/vdec_hevc_req_multi_if.c     |  4 +-
> >  .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
> >  .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  4 +-
> >  8 files changed, 85 insertions(+), 60 deletions(-)
> > 
> 
> 

Re: [PATCH v4 2/7] media: mediatek: vcodec: change flush decode order when stream off

[Sebastian Fricke]

Hey Yunfei,

On 07.08.2024 16:24, Yunfei Dong wrote:
>The buffer remove and buffer done of output queue is separated into
>two works, the value of owned_by_drv_count isn't zero when output
>queue stream off before flush decode.

You have to try this again, I cannot make sense out of your message.

What do you mean with:
"The buffer remove and buffer done of output queue is separated into two works"

I suppose this:
the value of owned_by_drv_count isn't zero when output queue stream off
before flush decode.

should be:
therefore the value of `owned_by_drv_count` isn't zero while flushing
the decoder in the STREAMOFF(OUTPUT queue) IOCTL.

right?

>Changing the flush decode from capture to output when stream off to
>make sure all the output queue buffers are set to done list.

I'd change this section to:

Flushing the decoder during STREAMOFF(OUTPUT) instead of during
STREAMOFF(CAPTURE) makes sure that all buffers on the OUTPUT queue are
set done.

The rest looks fine.

Regards,
Sebastian Fricke

>
>Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
>---
> .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 45 +++++++++----------
> 1 file changed, 22 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>index 2b787e60a1f9..7080ca3e18b0 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>@@ -893,32 +893,31 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
> 				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
> 			}
> 		}
>-		return;
>-	}
>-
>-	if (ctx->state >= MTK_STATE_HEADER) {
>-
>-		/* Until STREAMOFF is called on the CAPTURE queue
>-		 * (acknowledging the event), the driver operates
>-		 * as if the resolution hasn't changed yet, i.e.
>-		 * VIDIOC_G_FMT< etc. return previous resolution.
>-		 * So we update picinfo here
>-		 */
>-		ctx->picinfo = ctx->last_decoded_picinfo;
>
>-		mtk_v4l2_vdec_dbg(2, ctx,
>-				  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
>-				  ctx->id, ctx->last_decoded_picinfo.pic_w,
>-				  ctx->last_decoded_picinfo.pic_h,
>-				  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
>-				  ctx->last_decoded_picinfo.buf_w,
>-				  ctx->last_decoded_picinfo.buf_h);
>+		if (ctx->state >= MTK_STATE_HEADER) {
>+			/*
>+			 * The resolution hasn't been changed when STREAMOFF is called.
>+			 * Update the picinfo here with previous resolution if VIDIOC_G_FMT
>+			 * is called.
>+			 */
>+			ctx->picinfo = ctx->last_decoded_picinfo;
>+
>+			mtk_v4l2_vdec_dbg(2, ctx,
>+					  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
>+					  ctx->id, ctx->last_decoded_picinfo.pic_w,
>+					  ctx->last_decoded_picinfo.pic_h,
>+					  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
>+					  ctx->last_decoded_picinfo.buf_w,
>+					  ctx->last_decoded_picinfo.buf_h);
>+
>+			ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
>+			if (ret)
>+				mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
>+		}
>
>-		ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
>-		if (ret)
>-			mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
>+		ctx->state = MTK_STATE_FLUSH;
>+		return;
> 	}
>-	ctx->state = MTK_STATE_FLUSH;
>
> 	while ((dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx))) {
> 		vb2_set_plane_payload(&dst_buf->vb2_buf, 0, 0);
>-- 
>2.46.0
>
>

Re: [PATCH v4 3/7] media: mediatek: vcodec: flush decoder before stream off

[Sebastian Fricke]

Hey Yunfei,

On 07.08.2024 16:24, Yunfei Dong wrote:
>Flush decoder will reset the driver to flush status. If lat or core
>work queue in active before flush when stream off, will lead to get
>dst buffer NULL or buff done with one non-existent source buffer.
>
>Flush decoder when stream off no matter output or capture queue
>calling stream off firstly.
>
>Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
>---
> .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 45 ++++++++++---------
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>index 7080ca3e18b0..fc4ee1fb7cd1 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
>@@ -882,6 +882,29 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
> 	mtk_v4l2_vdec_dbg(3, ctx, "[%d] (%d) state=(%x) ctx->decoded_frame_cnt=%d",
> 			  ctx->id, q->type, ctx->state, ctx->decoded_frame_cnt);
>
>+	if (ctx->state >= MTK_STATE_HEADER && ctx->state != MTK_STATE_FLUSH) {
>+		/*
>+		 * The resolution hasn't been changed when STREAMOFF is called.
>+		 * Update the picinfo here with previous resolution if VIDIOC_G_FMT
>+		 * is called.
>+		 */
>+		ctx->picinfo = ctx->last_decoded_picinfo;
>+
>+		mtk_v4l2_vdec_dbg(2, ctx,
>+				  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
>+				  ctx->id, ctx->last_decoded_picinfo.pic_w,
>+				  ctx->last_decoded_picinfo.pic_h,
>+				  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
>+				  ctx->last_decoded_picinfo.buf_w,
>+				  ctx->last_decoded_picinfo.buf_h);
>+
>+		ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
>+		if (ret)
>+			mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
>+
>+		ctx->state = MTK_STATE_FLUSH;
>+	}
>+
> 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> 		while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx))) {
> 			if (src_buf != &ctx->empty_flush_buf.vb) {
>@@ -894,28 +917,6 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
> 			}
> 		}
>
>-		if (ctx->state >= MTK_STATE_HEADER) {
>-			/*
>-			 * The resolution hasn't been changed when STREAMOFF is called.
>-			 * Update the picinfo here with previous resolution if VIDIOC_G_FMT
>-			 * is called.
>-			 */
>-			ctx->picinfo = ctx->last_decoded_picinfo;
>-
>-			mtk_v4l2_vdec_dbg(2, ctx,
>-					  "[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
>-					  ctx->id, ctx->last_decoded_picinfo.pic_w,
>-					  ctx->last_decoded_picinfo.pic_h,
>-					  ctx->picinfo.pic_w, ctx->picinfo.pic_h,
>-					  ctx->last_decoded_picinfo.buf_w,
>-					  ctx->last_decoded_picinfo.buf_h);
>-
>-			ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
>-			if (ret)
>-				mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
>-		}
>-
>-		ctx->state = MTK_STATE_FLUSH;

you just changed this routine in patch 2/7, why was patch 2/7 needed if
you remove it right away in the next patch?

regards,
Sebastian Fricke

> 		return;
> 	}
>
>-- 
>2.46.0
>
>

Re: [PATCH v4 4/7] media: mediatek: vcodec: using input information to get vb2 buffer

[Sebastian Fricke]

Hey Yunfei,

I would rename the title to something like this:

media: mediatek: vcodec: Get SRC buffer from bitstream instead of M2M

On 07.08.2024 16:24, Yunfei Dong wrote:
>vb2 buffer may be removed from ready list when lat try to get next
>src buffer, leading to vb2 buffer not the current one. Need to get
>vb2 buffer according to current input memory information.

And I would rewrite the commit log like this:

Getting the SRC buffer from the M2M buffer-queue risks picking a
different SRC buffer than the one used for the current decode operation.
Get the SRC buffer therefore from the bitstream data, which was set up
earlier during the decode.

Did I get that right?

Also could you explain why this change is required in this series?

Regards,
Sebastian Fricke

>
>Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
>---
> .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c     | 13 +++++++------
> .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c     | 15 +++++++--------
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
>index 90217cc8e242..a744740ba5f1 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_lat_if.c
>@@ -1062,19 +1062,20 @@ static inline void vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
>
> static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_instance *instance,
> 						 struct vdec_av1_slice_vsi *vsi,
>+						 struct mtk_vcodec_mem *bs,
> 						 struct vdec_lat_buf *lat_buf)
> {
>-	struct vb2_v4l2_buffer *src;
>+	struct mtk_video_dec_buf *src_buf_info;
> 	struct vb2_v4l2_buffer *dst;
>
>-	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
>-	if (!src)
>+	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
>+	if (!src_buf_info)
> 		return -EINVAL;
>
>-	lat_buf->vb2_v4l2_src = src;
>+	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
>
> 	dst = &lat_buf->ts_info;
>-	v4l2_m2m_buf_copy_metadata(src, dst, true);
>+	v4l2_m2m_buf_copy_metadata(lat_buf->vb2_v4l2_src, dst, true);
> 	vsi->frame.cur_ts = dst->vb2_buf.timestamp;
>
> 	return 0;
>@@ -1724,7 +1725,7 @@ static int vdec_av1_slice_setup_lat(struct vdec_av1_slice_instance *instance,
> 	struct vdec_av1_slice_vsi *vsi = &pfc->vsi;
> 	int ret;
>
>-	ret = vdec_av1_slice_setup_lat_from_src_buf(instance, vsi, lat_buf);
>+	ret = vdec_av1_slice_setup_lat_from_src_buf(instance, vsi, bs, lat_buf);
> 	if (ret)
> 		return ret;
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
>index 3dceb668ba1c..c50a454ab4f7 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_lat_if.c
>@@ -712,19 +712,18 @@ int vdec_vp9_slice_setup_single_from_src_to_dst(struct vdec_vp9_slice_instance *
> }
>
> static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_vp9_slice_instance *instance,
>+						 struct mtk_vcodec_mem *bs,
> 						 struct vdec_lat_buf *lat_buf)
> {
>-	struct vb2_v4l2_buffer *src;
>-	struct vb2_v4l2_buffer *dst;
>+	struct mtk_video_dec_buf *src_buf_info;
>
>-	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
>-	if (!src)
>+	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
>+	if (!src_buf_info)
> 		return -EINVAL;
>
>-	lat_buf->vb2_v4l2_src = src;
>+	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
>
>-	dst = &lat_buf->ts_info;
>-	v4l2_m2m_buf_copy_metadata(src, dst, true);
>+	v4l2_m2m_buf_copy_metadata(lat_buf->vb2_v4l2_src, &lat_buf->ts_info, true);
> 	return 0;
> }
>
>@@ -1154,7 +1153,7 @@ static int vdec_vp9_slice_setup_lat(struct vdec_vp9_slice_instance *instance,
> 	struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
> 	int ret;
>
>-	ret = vdec_vp9_slice_setup_lat_from_src_buf(instance, lat_buf);
>+	ret = vdec_vp9_slice_setup_lat_from_src_buf(instance, bs, lat_buf);
> 	if (ret)
> 		goto err;
>
>-- 
>2.46.0
>
>

Re: [PATCH v4 5/7] media: mediatek: vcodec: store source vb2 buffer

[Sebastian Fricke]

Hey Yunfei,

On 07.08.2024 16:24, Yunfei Dong wrote:
>Store the current vb2 buffer when lat need to decode again.
>Then lat work can get the same vb2 buffer directly next time.

I would reword this with:

Store the current source buffer in the specific data for the IC. When
the LAT needs to retry a decode it can pick that buffer directly.

---

Additionally, this is not a good commit description as you just say what
you do, but you don't say WHY this needs to happen, why is it necessary?
What does it improve, is this a preparation patch for another, a fix for
something or an improvement of performance?


more below...

>
>Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
>---
> .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h      |  2 ++
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c         | 11 ++++++++---
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>index 1fabe8c5b7a4..0817be73f8e0 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
>@@ -155,6 +155,7 @@ struct mtk_vcodec_dec_pdata {
>  * @last_decoded_picinfo: pic information get from latest decode
>  * @empty_flush_buf: a fake size-0 capture buffer that indicates flush. Used
>  *		     for stateful decoder.
>+ * @last_vb2_v4l2_src: the backup of current source buffer.

I think last is confusing in this context especially as there is for
example in the m2m_ctx:
  * @last_src_buf: indicate the last source buffer for draining
  * @next_buf_last: next capture queud buffer will be tagged as last
or:
  * v4l2_m2m_last_buf() - return last buffer from the list of ready buffers

I think a better name would be:

  * @cur_src_buf: current source buffer with the bitstream data for the latest decode

>  * @is_flushing: set to true if flushing is in progress.
>  *
>  * @current_codec: current set input codec, in V4L2 pixel format
>@@ -201,6 +202,7 @@ struct mtk_vcodec_dec_ctx {
> 	struct work_struct decode_work;
> 	struct vdec_pic_info last_decoded_picinfo;
> 	struct v4l2_m2m_buffer empty_flush_buf;
>+	struct vb2_v4l2_buffer *last_vb2_v4l2_src;

Likewise here:

struct vb2_v4l2_buffer *cur_src_buf;

> 	bool is_flushing;
>
> 	u32 current_codec;
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>index 2a7e4fe24ed3..8aa379872ddc 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>@@ -320,7 +320,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> 	struct mtk_vcodec_dec_ctx *ctx =
> 		container_of(work, struct mtk_vcodec_dec_ctx, decode_work);
> 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
>-	struct vb2_v4l2_buffer *vb2_v4l2_src;
>+	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;

And here:

struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->cur_src_buf;

> 	struct vb2_buffer *vb2_src;
> 	struct mtk_vcodec_mem *bs_src;
> 	struct mtk_video_dec_buf *dec_buf_src;
>@@ -329,7 +329,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> 	bool res_chg = false;
> 	int ret;
>
>-	vb2_v4l2_src = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
>+	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx);

Please add a comment above this line that explains why this search can
be made, explaining why this buffer is still valid in this call and when
we pick the next source buffer.

Regards,
Sebastian Fricke

> 	if (!vb2_v4l2_src) {
> 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source buffer", ctx->id);
>@@ -383,8 +383,13 @@ static void mtk_vdec_worker(struct work_struct *work)
> 			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
> 		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
> 	} else {
>-		if (ret != -EAGAIN)
>+		if (ret != -EAGAIN) {
> 			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
>+			ctx->last_vb2_v4l2_src = NULL;
>+		} else {
>+			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
>+		}
>+
> 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> 	}
> }
>-- 
>2.46.0
>
>

Re: [PATCH v4 6/7] media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

[Sebastian Fricke]

Hey Yunfei,

On 07.08.2024 16:24, Yunfei Dong wrote:
>There isn't lock to protect source buffer when get next src buffer,
>if the source buffer is removed for some unknown reason before lat
>work queue execute done, will lead to remove source buffer or buffer
>done error.

This is really hard to understand, can try wording this a bit clearer?
Stuff like: if the source buffer is removed ... will lead to remove
source buffer, just leaves me scratching my head.
And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf` so I
suppose you mean something else when you say that there is no lock to
protect the source buffer?

You might not know all reasons but for this commit description you
should at least know one reason. Please highlight a case how this can
happen, so that you can justify the change.

>
>Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
>---
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>index 8aa379872ddc..3dba3549000a 100644
>--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>@@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> 		container_of(work, struct mtk_vcodec_dec_ctx, decode_work);
> 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
> 	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;
>+	struct vb2_v4l2_buffer *vb2_v4l2_dst;
> 	struct vb2_buffer *vb2_src;
> 	struct mtk_vcodec_mem *bs_src;
> 	struct mtk_video_dec_buf *dec_buf_src;
>@@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> 	bool res_chg = false;
> 	int ret;
>
>-	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx);
>+	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> 	if (!vb2_v4l2_src) {
> 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source buffer", ctx->id);
>@@ -381,17 +382,28 @@ static void mtk_vdec_worker(struct work_struct *work)
> 	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
> 		if (src_buf_req)
> 			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
>-		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
>-	} else {
>-		if (ret != -EAGAIN) {
>-			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
>-			ctx->last_vb2_v4l2_src = NULL;
>-		} else {
>-			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
>-		}
>+		vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
>+		v4l2_m2m_buf_done(vb2_v4l2_dst, state);
>+		v4l2_m2m_buf_done(vb2_v4l2_src, state);

This is another case where you just remove again completely what you
have added in the previous patch.

>
> 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
>+		return;
> 	}
>+
>+	/* If each codec return -EAGAIN to decode again, need to backup current source
>+	 * buffer, then the driver will get this buffer next time.

I would reword this like:

	/* Store the current source buffer for the next attempt to decode,
    * if this decode returned -EAGAIN */

>+	 *
>+	 * If each codec decode error, must to set buffer done with error status for
>+	 * this buffer have been removed from ready list.
>+	 */
>+	ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src;

Okay and here you add the same thing again as in the previous patch but
differently, this collection of commits feels more and more to me like a
work in progress. Please make sure in the future that each commit does
one job and does it completely.
It is not only confussing but also makes it hard to read the changes as
the bigger picture is missing in these tiny commits.

Please try to combine the patches where possible.

Regards,
Sebastian Fricke

>+	if (ret && ret != -EAGAIN) {
>+		if (src_buf_req)
>+			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
>+		v4l2_m2m_buf_done(vb2_v4l2_src, state);
>+	}
>+
>+	v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> }
>
> static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
>-- 
>2.46.0
>
>

Re: [PATCH v4 0/7] media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail

[Sebastian Fricke]

Hey Yunfei,

given this new series by Hans:
https://patchwork.linuxtv.org/project/linux-media/list/?series=13427

we might actually be able to find a more performant solution of the
problem, I'll work on that a bit and give you feedback.

Regards,
Sebastian

On 07.08.2024 16:24, Yunfei Dong wrote:
>v4l2_m2m_buf_done is called in lat work queue, v4l2_ctrl_request_complete
>is called in core queue. The request status of output queue will be set to
>MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called, leading to
>output queue request complete fail. Must move v4l2_ctrl_request_complete
>in front of v4l2_m2m_buf_done.
>
>Patch 1 setting request complete before buffer done
>Patch 2 change flush decode order when stream off
>Patch 3 flush decoder before stream off
>Patch 4 using input information to get vb2 buffer
>Patch 5 store source vb2 buffer
>Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
>Patch 7 remove media request checking
>
>---
>compared with v3:
>- fix flush decoder issue when userspace stream off capture queue firstly
>- fluster test result same with v3
>
>compared with v2:
>- add patch 5/6/7 to fix decode again issue
>- add fluster test result with mt8195 platform(same with no changed):
>  1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
>     VP8-TEST-VECTORS 59/61
>  2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
>     VP9-TEST-VECTORS 276/305
>  3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
>     AV1-TEST-VECTORS 237/239
>  4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
>     JVT-AVC_V1       95/135
>  5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
>     JCT-VC-HEVC_V1   142/147
>
>compared with v1:
>- add patch 2/3/4 to fix timing issue.
>---
>Yunfei Dong (7):
>  media: mediatek: vcodec: setting request complete before buffer done
>  media: mediatek: vcodec: change flush decode order when stream off
>  media: mediatek: vcodec: flush decoder before stream off
>  media: mediatek: vcodec: using input information to get vb2 buffer
>  media: mediatek: vcodec: store source vb2 buffer
>  media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
>    v4l2_m2m_src_buf_remove
>  media: mediatek: vcodec: remove media request checking
>
> .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 44 ++++++++---------
> .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  4 +-
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++-----
> .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
> .../decoder/vdec/vdec_h264_req_multi_if.c     |  4 +-
> .../decoder/vdec/vdec_hevc_req_multi_if.c     |  4 +-
> .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
> .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  4 +-
> 8 files changed, 85 insertions(+), 60 deletions(-)
>
>-- 
>2.46.0
>
>

Re: [PATCH v4 3/7] media: mediatek: vcodec: flush decoder before stream off

[Yunfei Dong (董云飞)]

Hi Sebastian,

Thanks for your suggestion.

On Thu, 2024-08-22 at 18:11 +0200, Sebastian Fricke wrote:
> Hey Yunfei,
> 
> On 07.08.2024 16:24, Yunfei Dong wrote:
> > Flush decoder will reset the driver to flush status. If lat or core
> > work queue in active before flush when stream off, will lead to get
> > dst buffer NULL or buff done with one non-existent source buffer.
> > 
> > Flush decoder when stream off no matter output or capture queue
> > calling stream off firstly.
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> > .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 45 ++++++++++----
> > -----
> > 1 file changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > index 7080ca3e18b0..fc4ee1fb7cd1 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> > @@ -882,6 +882,29 @@ void vb2ops_vdec_stop_streaming(struct
> > vb2_queue *q)
> > 	mtk_v4l2_vdec_dbg(3, ctx, "[%d] (%d) state=(%x) ctx-
> > >decoded_frame_cnt=%d",
> > 			  ctx->id, q->type, ctx->state, ctx-
> > >decoded_frame_cnt);
> > 
> > +	if (ctx->state >= MTK_STATE_HEADER && ctx->state !=
> > MTK_STATE_FLUSH) {
> > +		/*
> > +		 * The resolution hasn't been changed when STREAMOFF is
> > called.
> > +		 * Update the picinfo here with previous resolution if
> > VIDIOC_G_FMT
> > +		 * is called.
> > +		 */
> > +		ctx->picinfo = ctx->last_decoded_picinfo;
> > +
> > +		mtk_v4l2_vdec_dbg(2, ctx,
> > +				  "[%d]-> new(%d,%d), old(%d,%d),
> > real(%d,%d)",
> > +				  ctx->id, ctx-
> > >last_decoded_picinfo.pic_w,
> > +				  ctx->last_decoded_picinfo.pic_h,
> > +				  ctx->picinfo.pic_w, ctx-
> > >picinfo.pic_h,
> > +				  ctx->last_decoded_picinfo.buf_w,
> > +				  ctx->last_decoded_picinfo.buf_h);
> > +
> > +		ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
> > +		if (ret)
> > +			mtk_v4l2_vdec_err(ctx, "DecodeFinal failed,
> > ret=%d", ret);
> > +
> > +		ctx->state = MTK_STATE_FLUSH;
> > +	}
> > +
> > 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > 		while ((src_buf = v4l2_m2m_src_buf_remove(ctx-
> > >m2m_ctx))) {
> > 			if (src_buf != &ctx->empty_flush_buf.vb) {
> > @@ -894,28 +917,6 @@ void vb2ops_vdec_stop_streaming(struct
> > vb2_queue *q)
> > 			}
> > 		}
> > 
> > -		if (ctx->state >= MTK_STATE_HEADER) {
> > -			/*
> > -			 * The resolution hasn't been changed when
> > STREAMOFF is called.
> > -			 * Update the picinfo here with previous
> > resolution if VIDIOC_G_FMT
> > -			 * is called.
> > -			 */
> > -			ctx->picinfo = ctx->last_decoded_picinfo;
> > -
> > -			mtk_v4l2_vdec_dbg(2, ctx,
> > -					  "[%d]-> new(%d,%d),
> > old(%d,%d), real(%d,%d)",
> > -					  ctx->id, ctx-
> > >last_decoded_picinfo.pic_w,
> > -					  ctx-
> > >last_decoded_picinfo.pic_h,
> > -					  ctx->picinfo.pic_w, ctx-
> > >picinfo.pic_h,
> > -					  ctx-
> > >last_decoded_picinfo.buf_w,
> > -					  ctx-
> > >last_decoded_picinfo.buf_h);
> > -
> > -			ret = ctx->dev->vdec_pdata->flush_decoder(ctx);
> > -			if (ret)
> > -				mtk_v4l2_vdec_err(ctx, "DecodeFinal
> > failed, ret=%d", ret);
> > -		}
> > -
> > -		ctx->state = MTK_STATE_FLUSH;
> 
> you just changed this routine in patch 2/7, why was patch 2/7 needed
> if
> you remove it right away in the next patch?
> 
Patch 2/7 is used to fix that ctrl request complete and buffer done are
separated into two works for output queue, flush decoder on capture
queue may lead to ctrl request complete fail when user space stop
output queue firstly.

Patch 3/7 is used to fix that user space may call stop capture or
output queue first randomly.

Need to combine these two patches together?

Best Regards,
Yunfei Dong
> regards,
> Sebastian Fricke
> 
> > 		return;
> > 	}
> > 
> > -- 
> > 2.46.0
> > 
> > 

Re: [PATCH v4 4/7] media: mediatek: vcodec: using input information to get vb2 buffer

[Yunfei Dong (董云飞)]

Hi Sebastian,

Thanks for your suggestion.
On Fri, 2024-08-23 at 14:40 +0200, Sebastian Fricke wrote:
> Hey Yunfei,
> 
> I would rename the title to something like this:
> 
> media: mediatek: vcodec: Get SRC buffer from bitstream instead of M2M
> 
> On 07.08.2024 16:24, Yunfei Dong wrote:
> > vb2 buffer may be removed from ready list when lat try to get next
> > src buffer, leading to vb2 buffer not the current one. Need to get
> > vb2 buffer according to current input memory information.
> 
> And I would rewrite the commit log like this:
> 
> Getting the SRC buffer from the M2M buffer-queue risks picking a
> different SRC buffer than the one used for the current decode
> operation.
> Get the SRC buffer therefore from the bitstream data, which was set
> up
> earlier during the decode.
> 
> Did I get that right?
> 
> Also could you explain why this change is required in this series?
> 
Must make sure the src buffer is removed from ready list, in case of
the buffer is removed when core work try to set the buffer done.

Best Regards,
Yunfei Dong
> Regards,
> Sebastian Fricke
> 
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> > .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c     | 13 +++++++-----
> > -
> > .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c     | 15 +++++++-----
> > ---
> > 2 files changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_
> > lat_if.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_
> > lat_if.c
> > index 90217cc8e242..a744740ba5f1 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_
> > lat_if.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_av1_req_
> > lat_if.c
> > @@ -1062,19 +1062,20 @@ static inline void
> > vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
> > 
> > static int vdec_av1_slice_setup_lat_from_src_buf(struct
> > vdec_av1_slice_instance *instance,
> > 						 struct
> > vdec_av1_slice_vsi *vsi,
> > +						 struct mtk_vcodec_mem
> > *bs,
> > 						 struct vdec_lat_buf
> > *lat_buf)
> > {
> > -	struct vb2_v4l2_buffer *src;
> > +	struct mtk_video_dec_buf *src_buf_info;
> > 	struct vb2_v4l2_buffer *dst;
> > 
> > -	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
> > -	if (!src)
> > +	src_buf_info = container_of(bs, struct mtk_video_dec_buf,
> > bs_buffer);
> > +	if (!src_buf_info)
> > 		return -EINVAL;
> > 
> > -	lat_buf->vb2_v4l2_src = src;
> > +	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
> > 
> > 	dst = &lat_buf->ts_info;
> > -	v4l2_m2m_buf_copy_metadata(src, dst, true);
> > +	v4l2_m2m_buf_copy_metadata(lat_buf->vb2_v4l2_src, dst, true);
> > 	vsi->frame.cur_ts = dst->vb2_buf.timestamp;
> > 
> > 	return 0;
> > @@ -1724,7 +1725,7 @@ static int vdec_av1_slice_setup_lat(struct
> > vdec_av1_slice_instance *instance,
> > 	struct vdec_av1_slice_vsi *vsi = &pfc->vsi;
> > 	int ret;
> > 
> > -	ret = vdec_av1_slice_setup_lat_from_src_buf(instance, vsi,
> > lat_buf);
> > +	ret = vdec_av1_slice_setup_lat_from_src_buf(instance, vsi, bs,
> > lat_buf);
> > 	if (ret)
> > 		return ret;
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_
> > lat_if.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_
> > lat_if.c
> > index 3dceb668ba1c..c50a454ab4f7 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_
> > lat_if.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_vp9_req_
> > lat_if.c
> > @@ -712,19 +712,18 @@ int
> > vdec_vp9_slice_setup_single_from_src_to_dst(struct
> > vdec_vp9_slice_instance *
> > }
> > 
> > static int vdec_vp9_slice_setup_lat_from_src_buf(struct
> > vdec_vp9_slice_instance *instance,
> > +						 struct mtk_vcodec_mem
> > *bs,
> > 						 struct vdec_lat_buf
> > *lat_buf)
> > {
> > -	struct vb2_v4l2_buffer *src;
> > -	struct vb2_v4l2_buffer *dst;
> > +	struct mtk_video_dec_buf *src_buf_info;
> > 
> > -	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
> > -	if (!src)
> > +	src_buf_info = container_of(bs, struct mtk_video_dec_buf,
> > bs_buffer);
> > +	if (!src_buf_info)
> > 		return -EINVAL;
> > 
> > -	lat_buf->vb2_v4l2_src = src;
> > +	lat_buf->vb2_v4l2_src = &src_buf_info->m2m_buf.vb;
> > 
> > -	dst = &lat_buf->ts_info;
> > -	v4l2_m2m_buf_copy_metadata(src, dst, true);
> > +	v4l2_m2m_buf_copy_metadata(lat_buf->vb2_v4l2_src, &lat_buf-
> > >ts_info, true);
> > 	return 0;
> > }
> > 
> > @@ -1154,7 +1153,7 @@ static int vdec_vp9_slice_setup_lat(struct
> > vdec_vp9_slice_instance *instance,
> > 	struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
> > 	int ret;
> > 
> > -	ret = vdec_vp9_slice_setup_lat_from_src_buf(instance, lat_buf);
> > +	ret = vdec_vp9_slice_setup_lat_from_src_buf(instance, bs,
> > lat_buf);
> > 	if (ret)
> > 		goto err;
> > 
> > -- 
> > 2.46.0
> > 
> > 
> 
> 

Re: [PATCH v4 5/7] media: mediatek: vcodec: store source vb2 buffer

[Yunfei Dong (董云飞)]

Hi Sebastian,

Thanks for your suggestion.
On Fri, 2024-08-23 at 16:14 +0200, Sebastian Fricke wrote:
> Hey Yunfei,
> 
> On 07.08.2024 16:24, Yunfei Dong wrote:
> > Store the current vb2 buffer when lat need to decode again.
> > Then lat work can get the same vb2 buffer directly next time.
> 
> I would reword this with:
> 
> Store the current source buffer in the specific data for the IC. When
> the LAT needs to retry a decode it can pick that buffer directly.
> 
> ---
> 
> Additionally, this is not a good commit description as you just say
> what
> you do, but you don't say WHY this needs to happen, why is it
> necessary?
> What does it improve, is this a preparation patch for another, a fix
> for
> something or an improvement of performance?
> 
> 
> more below...
> 
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> > .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h      |  2 ++
> > .../vcodec/decoder/mtk_vcodec_dec_stateless.c         | 11
> > ++++++++---
> > 2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > index 1fabe8c5b7a4..0817be73f8e0 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv
> > .h
> > @@ -155,6 +155,7 @@ struct mtk_vcodec_dec_pdata {
> >  * @last_decoded_picinfo: pic information get from latest decode
> >  * @empty_flush_buf: a fake size-0 capture buffer that indicates
> > flush. Used
> >  *		     for stateful decoder.
> > + * @last_vb2_v4l2_src: the backup of current source buffer.
> 
> I think last is confusing in this context especially as there is for
> example in the m2m_ctx:
>   * @last_src_buf: indicate the last source buffer for draining
>   * @next_buf_last: next capture queud buffer will be tagged as last
> or:
>   * v4l2_m2m_last_buf() - return last buffer from the list of ready
> buffers
> 
> I think a better name would be:
> 
>   * @cur_src_buf: current source buffer with the bitstream data for
> the latest decode
> 
> >  * @is_flushing: set to true if flushing is in progress.
> >  *
> >  * @current_codec: current set input codec, in V4L2 pixel format
> > @@ -201,6 +202,7 @@ struct mtk_vcodec_dec_ctx {
> > 	struct work_struct decode_work;
> > 	struct vdec_pic_info last_decoded_picinfo;
> > 	struct v4l2_m2m_buffer empty_flush_buf;
> > +	struct vb2_v4l2_buffer *last_vb2_v4l2_src;
> 
> Likewise here:
> 
> struct vb2_v4l2_buffer *cur_src_buf;
> 
> > 	bool is_flushing;
> > 
> > 	u32 current_codec;
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > index 2a7e4fe24ed3..8aa379872ddc 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > @@ -320,7 +320,7 @@ static void mtk_vdec_worker(struct work_struct
> > *work)
> > 	struct mtk_vcodec_dec_ctx *ctx =
> > 		container_of(work, struct mtk_vcodec_dec_ctx,
> > decode_work);
> > 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
> > -	struct vb2_v4l2_buffer *vb2_v4l2_src;
> > +	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;
> 
> And here:
> 
> struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->cur_src_buf;
> 
> > 	struct vb2_buffer *vb2_src;
> > 	struct mtk_vcodec_mem *bs_src;
> > 	struct mtk_video_dec_buf *dec_buf_src;
> > @@ -329,7 +329,7 @@ static void mtk_vdec_worker(struct work_struct
> > *work)
> > 	bool res_chg = false;
> > 	int ret;
> > 
> > -	vb2_v4l2_src = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> > +	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src :
> > v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> 
> Please add a comment above this line that explains why this search
> can
> be made, explaining why this buffer is still valid in this call and
> when
> we pick the next source buffer.
> 

As the commit wrote, if the driver need to decoder again, need to use
the last remove source buffer to decode again, not to get one new.

Regards,
Yunfei Dong
> Regards,
> Sebastian Fricke
> 
> > 	if (!vb2_v4l2_src) {
> > 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source
> > buffer", ctx->id);
> > @@ -383,8 +383,13 @@ static void mtk_vdec_worker(struct work_struct
> > *work)
> > 			v4l2_ctrl_request_complete(src_buf_req, &ctx-
> > >ctrl_hdl);
> > 		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx-
> > >m2m_ctx, state);
> > 	} else {
> > -		if (ret != -EAGAIN)
> > +		if (ret != -EAGAIN) {
> > 			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > +			ctx->last_vb2_v4l2_src = NULL;
> > +		} else {
> > +			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
> > +		}
> > +
> > 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > 	}
> > }
> > -- 
> > 2.46.0
> > 
> > 
> 
> 

Re: [PATCH v4 6/7] media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

[Yunfei Dong (董云飞)]

Hi Sebastian,

Thanks for your suggestion.
On Fri, 2024-08-23 at 17:23 +0200, Sebastian Fricke wrote:
> Hey Yunfei,
> 
> On 07.08.2024 16:24, Yunfei Dong wrote:
> > There isn't lock to protect source buffer when get next src buffer,
> > if the source buffer is removed for some unknown reason before lat
> > work queue execute done, will lead to remove source buffer or
> > buffer
> > done error.
> 
> This is really hard to understand, can try wording this a bit
> clearer?
> Stuff like: if the source buffer is removed ... will lead to remove
> source buffer, just leaves me scratching my head.
> And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf`
> so I
> suppose you mean something else when you say that there is no lock to
> protect the source buffer?
> 
> You might not know all reasons but for this commit description you
> should at least know one reason. Please highlight a case how this can
> happen, so that you can justify the change.
> 
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> > .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++---
> > ---
> > 1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > index 8aa379872ddc..3dba3549000a 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > @@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct
> > *work)
> > 		container_of(work, struct mtk_vcodec_dec_ctx,
> > decode_work);
> > 	struct mtk_vcodec_dec_dev *dev = ctx->dev;
> > 	struct vb2_v4l2_buffer *vb2_v4l2_src = ctx->last_vb2_v4l2_src;
> > +	struct vb2_v4l2_buffer *vb2_v4l2_dst;
> > 	struct vb2_buffer *vb2_src;
> > 	struct mtk_vcodec_mem *bs_src;
> > 	struct mtk_video_dec_buf *dec_buf_src;
> > @@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct
> > *work)
> > 	bool res_chg = false;
> > 	int ret;
> > 
> > -	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src :
> > v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> > +	vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src :
> > v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > 	if (!vb2_v4l2_src) {
> > 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > 		mtk_v4l2_vdec_dbg(1, ctx, "[%d] no available source
> > buffer", ctx->id);
> > @@ -381,17 +382,28 @@ static void mtk_vdec_worker(struct
> > work_struct *work)
> > 	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
> > 		if (src_buf_req)
> > 			v4l2_ctrl_request_complete(src_buf_req, &ctx-
> > >ctrl_hdl);
> > -		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx-
> > >m2m_ctx, state);
> > -	} else {
> > -		if (ret != -EAGAIN) {
> > -			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> > -			ctx->last_vb2_v4l2_src = NULL;
> > -		} else {
> > -			ctx->last_vb2_v4l2_src = vb2_v4l2_src;
> > -		}
> > +		vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> > +		v4l2_m2m_buf_done(vb2_v4l2_dst, state);
> > +		v4l2_m2m_buf_done(vb2_v4l2_src, state);
> 
> This is another case where you just remove again completely what you
> have added in the previous patch.
> 
> > 
> > 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > +		return;
> > 	}
> > +
> > +	/* If each codec return -EAGAIN to decode again, need to backup
> > current source
> > +	 * buffer, then the driver will get this buffer next time.
> 
> I would reword this like:
> 
> 	/* Store the current source buffer for the next attempt to
> decode,
>     * if this decode returned -EAGAIN */
> 
> > +	 *
> > +	 * If each codec decode error, must to set buffer done with
> > error status for
> > +	 * this buffer have been removed from ready list.
> > +	 */
> > +	ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL :
> > vb2_v4l2_src;
> 
> Okay and here you add the same thing again as in the previous patch
> but
> differently, this collection of commits feels more and more to me
> like a
> work in progress. Please make sure in the future that each commit
> does
> one job and does it completely.
> It is not only confussing but also makes it hard to read the changes
> as
> the bigger picture is missing in these tiny commits.
> 
> Please try to combine the patches where possible.
> 
Path 5/7 is used to store the source buffer.
path 6/7 is used to handle decoder again when decoder error.

I will check whether it's possible to combine patch 5 and 6 together.

Best Regards,
Yunfei Dong
> Regards,
> Sebastian Fricke
> 
> > +	if (ret && ret != -EAGAIN) {
> > +		if (src_buf_req)
> > +			v4l2_ctrl_request_complete(src_buf_req, &ctx-
> > >ctrl_hdl);
> > +		v4l2_m2m_buf_done(vb2_v4l2_src, state);
> > +	}
> > +
> > +	v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > }
> > 
> > static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
> > -- 
> > 2.46.0
> > 
> > 

Re: [PATCH v4 0/7] media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail

[Yunfei Dong (董云飞)]

Hi Sebastian,

Thanks for your suggestion.
On Fri, 2024-08-23 at 17:45 +0200, Sebastian Fricke wrote:
> Hey Yunfei,
> 
> given this new series by Hans:
> 
https://urldefense.com/v3/__https://patchwork.linuxtv.org/project/linux-media/list/?series=13427__;!!CTRNKA9wMg0ARbw!h4__bSC_RX1erZCq5wvZbjRQrmS9ecXytwjcdLc9sgjBgj3buUC8JrUG78rhtEsXAN6vmZEhM3tWumFQ50R5NcR_1hbqLyYlSg$
>  
> 
> we might actually be able to find a more performant solution of the
> problem, I'll work on that a bit and give you feedback.
> 
I already do so many test for different project with these patches,
whether it's possible to merge these patch firstly, then to call hans's
changes to do stress test.

Best Regards,
Yunfei Dong
> Regards,
> Sebastian
> 
> On 07.08.2024 16:24, Yunfei Dong wrote:
> > v4l2_m2m_buf_done is called in lat work queue,
> > v4l2_ctrl_request_complete
> > is called in core queue. The request status of output queue will be
> > set to
> > MEDIA_REQUEST_STATE_COMPLETE when v4l2_m2m_buf_done is called,
> > leading to
> > output queue request complete fail. Must move
> > v4l2_ctrl_request_complete
> > in front of v4l2_m2m_buf_done.
> > 
> > Patch 1 setting request complete before buffer done
> > Patch 2 change flush decode order when stream off
> > Patch 3 flush decoder before stream off
> > Patch 4 using input information to get vb2 buffer
> > Patch 5 store source vb2 buffer
> > Patch 6 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
> > Patch 7 remove media request checking
> > 
> > ---
> > compared with v3:
> > - fix flush decoder issue when userspace stream off capture queue
> > firstly
> > - fluster test result same with v3
> > 
> > compared with v2:
> > - add patch 5/6/7 to fix decode again issue
> > - add fluster test result with mt8195 platform(same with no
> > changed):
> >  1> ./fluster.py run -d GStreamer-VP8-V4L2SL-Gst1.0 -j1 -t 90
> >     VP8-TEST-VECTORS 59/61
> >  2> ./fluster.py run -d GStreamer-VP9-V4L2SL-Gst1.0 -j1 -t 90
> >     VP9-TEST-VECTORS 276/305
> >  3> ./fluster.py run -d GStreamer-AV1-V4L2SL-Gst1.0 -j1 -t 90
> >     AV1-TEST-VECTORS 237/239
> >  4> ./fluster.py run -d GStreamer-H.264-V4L2SL-Gst1.0 -j1 -t 90
> >     JVT-AVC_V1       95/135
> >  5> ./fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -j1 -t 90
> >     JCT-VC-HEVC_V1   142/147
> > 
> > compared with v1:
> > - add patch 2/3/4 to fix timing issue.
> > ---
> > Yunfei Dong (7):
> >  media: mediatek: vcodec: setting request complete before buffer
> > done
> >  media: mediatek: vcodec: change flush decode order when stream off
> >  media: mediatek: vcodec: flush decoder before stream off
> >  media: mediatek: vcodec: using input information to get vb2 buffer
> >  media: mediatek: vcodec: store source vb2 buffer
> >  media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
> >    v4l2_m2m_src_buf_remove
> >  media: mediatek: vcodec: remove media request checking
> > 
> > .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 44 ++++++++------
> > ---
> > .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  4 +-
> > .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 48 ++++++++++++++
> > -----
> > .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 18 +++----
> > .../decoder/vdec/vdec_h264_req_multi_if.c     |  4 +-
> > .../decoder/vdec/vdec_hevc_req_multi_if.c     |  4 +-
> > .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 19 ++++----
> > .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  4 +-
> > 8 files changed, 85 insertions(+), 60 deletions(-)
> > 
> > -- 
> > 2.46.0
> > 
> >