[PATCH v2,0/4] media: mediatek: vcodec: fix ctrl request complete fail

[PATCH v2,0/4] media: mediatek: vcodec: fix ctrl request complete fail

[Yunfei Dong]

Moving v4l2_ctrl_request_complete to before of function
v4l2_m2m_buf_done to make sure the status of request correctly.

Replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove to
make sure the src buffer won't be removed for some unknown
reason leading to buffer done error.

Patch 1 setting request complete before buffer done
Patch 3 flush decoder before remove all source buffer
Patch 2 change flush decode from capture to output when stream off
Patch 4 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
---
compared with v1:
- add patch 2/3/4 to fix timing issue.
---
Yunfei Dong (4):
  media: mediatek: vcodec: setting request complete before buffer done
  media: mediatek: vcodec: change flush decode from capture to output
    when stream off
  media: mediatek: vcodec: flush decoder before remove all source buffer
  media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
    v4l2_m2m_src_buf_remove

 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 52 +++++++++----------
 .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  3 +-
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 28 +++++++---
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 28 +++++-----
 .../decoder/vdec/vdec_h264_req_multi_if.c     |  3 +-
 .../decoder/vdec/vdec_hevc_req_multi_if.c     |  3 +-
 .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 37 ++++++-------
 .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  2 +
 8 files changed, 84 insertions(+), 72 deletions(-)

-- 
2.18.0

[PATCH v2,1/4] media: mediatek: vcodec: setting request complete before buffer done

[Yunfei Dong]

The request status of output queue will be set to MEDIA_REQUEST_STATE_COMPLETE
when user space dequeue output buffer. Then calling v4l2_ctrl_request_complete
will get below warning, need to call 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     |  3 ++-
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c        | 12 +++++++-----
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c        |  7 +++++--
 .../vcodec/decoder/vdec/vdec_h264_req_multi_if.c     |  3 ++-
 .../vcodec/decoder/vdec/vdec_hevc_req_multi_if.c     |  3 ++-
 .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c        |  6 ++++--
 .../mediatek/vcodec/decoder/vdec_msg_queue.h         |  2 ++
 8 files changed, 26 insertions(+), 14 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 ba742f0e391d..409a105c5c12 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -839,10 +839,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 849b89dd205c..3f5b625330bc 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,8 @@ 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 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..3060768e0ea9 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,7 +245,8 @@ 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 media_request *src_buf_req,
+					   struct vb2_v4l2_buffer *vb2_v4l2_src)
 {
 	struct vb2_v4l2_buffer *vb2_dst;
 	enum vb2_buffer_state state;
@@ -266,6 +267,9 @@ static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int e
 
 	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 +378,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 2b6a5adbc419..f277b907c345 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
@@ -1064,6 +1064,8 @@ static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_instance
 		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;
@@ -2187,7 +2189,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->src_buf_req, lat_buf->vb2_v4l2_src);
 
 	return 0;
 
@@ -2196,7 +2198,8 @@ 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->src_buf_req,
+						  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 0e741e0dc8ba..7033999018ca 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->src_buf_req, lat_buf->vb2_v4l2_src);
 	mtk_vdec_debug(ctx, "core decode done err=%d", err);
 	ctx->decoded_frame_cnt++;
 	return 0;
@@ -606,6 +606,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 06ed47df693b..67fe3c4bfac3 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
@@ -742,6 +742,7 @@ static int vdec_hevc_slice_setup_lat_buffer(struct vdec_hevc_slice_inst *inst,
 
 	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 +962,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->src_buf_req, 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 cf48d09b78d7..0dedbc3680e8 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
@@ -722,6 +722,7 @@ static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_vp9_slice_instance
 		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 +2188,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->src_buf_req, lat_buf->vb2_v4l2_src);
 
 	return 0;
 
@@ -2197,7 +2198,8 @@ 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->src_buf_req,
+							  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 1d9beb9e4a14..b0f2443d186f 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.h
+++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.h
@@ -55,6 +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
+ * @vb2_v4l2_src: the vb2 buffer of output queue
  * @src_buf_req: output buffer media request object
  *
  * @private_data: shared information used to lat and core hardware
@@ -71,6 +72,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 vb2_v4l2_buffer *vb2_v4l2_src;
 	struct media_request *src_buf_req;
 
 	void *private_data;
-- 
2.18.0

[PATCH v2,2/4] media: mediatek: vcodec: change flush decode from capture to output 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, need
to change flush decode from capture to output when stream off.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 46 +++++++++----------
 1 file changed, 23 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 409a105c5c12..3766e2176899 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -845,32 +845,32 @@ 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) {
+			/* 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);
+
+			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.18.0

[PATCH v2,3/4] media: mediatek: vcodec: flush decoder before remove all source buffer

[Yunfei Dong]

Flush decoder will reset all driver to init status, lat and core work
queue will stop to work. If lat or core work queue in working when
remove all source buffer, will lead to remove source buffer again
or buff done with one non-existent source buffer.

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 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 3766e2176899..cdd36a5320cd 100644
--- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
@@ -835,17 +835,6 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 			  ctx->id, q->type, ctx->state, ctx->decoded_frame_cnt);
 
 	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) {
-				struct media_request *req =
-					src_buf->vb2_buf.req_obj.req;
-
-				if (req)
-					v4l2_ctrl_request_complete(req, &ctx->ctrl_hdl);
-				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
-			}
-		}
-
 		if (ctx->state >= MTK_STATE_HEADER) {
 			/* Until STREAMOFF is called on the CAPTURE queue
 			 * (acknowledging the event), the driver operates
@@ -868,6 +857,17 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
 				mtk_v4l2_vdec_err(ctx, "DecodeFinal failed, ret=%d", ret);
 		}
 
+		while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx))) {
+			if (src_buf != &ctx->empty_flush_buf.vb) {
+				struct media_request *req =
+					src_buf->vb2_buf.req_obj.req;
+
+				if (req)
+					v4l2_ctrl_request_complete(req, &ctx->ctrl_hdl);
+				v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
+			}
+		}
+
 		ctx->state = MTK_STATE_FLUSH;
 		return;
 	}
-- 
2.18.0

[PATCH v2,4/4] 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 | 22 +++++++++----
 .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 25 ++++++--------
 .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 33 ++++++++-----------
 3 files changed, 40 insertions(+), 40 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 3060768e0ea9..bb2680f3ec5b 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
@@ -328,7 +328,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 = 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);
@@ -363,7 +363,7 @@ static void mtk_vdec_worker(struct work_struct *work)
 		mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
 
 	ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);
-	if (ret && ret != -EAGAIN) {
+	if (ret) {
 		mtk_v4l2_vdec_err(ctx,
 				  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu ret=%d res_chg=%d",
 				  ctx->id, vb2_src->index, bs_src->size,
@@ -380,11 +380,21 @@ 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);
+		if (vb2_v4l2_src)
+			v4l2_m2m_buf_done(vb2_v4l2_src, state);
+
 		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+	} else {
+		if (!ret) {
+			v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
+		} else {
+			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);
+
+			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 f277b907c345..339c5c88da1a 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
@@ -1052,23 +1052,18 @@ static inline void vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
 	memcpy(remote_vsi, vsi, sizeof(*vsi));
 }
 
-static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_instance *instance,
-						 struct vdec_av1_slice_vsi *vsi,
-						 struct vdec_lat_buf *lat_buf)
+static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_vsi *vsi,
+						 struct vdec_lat_buf *lat_buf,
+						 struct mtk_vcodec_mem *bs)
 {
-	struct vb2_v4l2_buffer *src;
-	struct vb2_v4l2_buffer *dst;
-
-	src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
-	if (!src)
-		return -EINVAL;
+	struct mtk_video_dec_buf *src_buf_info;
 
-	lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
-	lat_buf->vb2_v4l2_src = src;
+	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;
 
-	dst = &lat_buf->ts_info;
-	v4l2_m2m_buf_copy_metadata(src, dst, true);
-	vsi->frame.cur_ts = dst->vb2_buf.timestamp;
+	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
+	vsi->frame.cur_ts = lat_buf->ts_info.vb2_buf.timestamp;
 
 	return 0;
 }
@@ -1717,7 +1712,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(vsi, lat_buf, bs);
 	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 0dedbc3680e8..2697e04f4313 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
@@ -693,39 +693,34 @@ static int vdec_vp9_slice_tile_offset(int idx, int mi_num, int tile_log2)
 }
 
 static
-int vdec_vp9_slice_setup_single_from_src_to_dst(struct vdec_vp9_slice_instance *instance)
+int vdec_vp9_slice_setup_single_from_src_to_dst(struct vdec_vp9_slice_instance *instance,
+						struct mtk_vcodec_mem *bs)
 {
-	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)
-		return -EINVAL;
+	src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
 
 	dst = v4l2_m2m_next_dst_buf(instance->ctx->m2m_ctx);
 	if (!dst)
 		return -EINVAL;
 
-	v4l2_m2m_buf_copy_metadata(src, dst, true);
+	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, dst, true);
 
 	return 0;
 }
 
-static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_vp9_slice_instance *instance,
-						 struct vdec_lat_buf *lat_buf)
+static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_lat_buf *lat_buf,
+						 struct mtk_vcodec_mem *bs)
 {
-	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)
-		return -EINVAL;
+	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;
 
-	lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
-	lat_buf->vb2_v4l2_src = src;
+	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
 
-	dst = &lat_buf->ts_info;
-	v4l2_m2m_buf_copy_metadata(src, dst, true);
 	return 0;
 }
 
@@ -1155,7 +1150,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(lat_buf, bs);
 	if (ret)
 		goto err;
 
@@ -1796,7 +1791,7 @@ static int vdec_vp9_slice_setup_single(struct vdec_vp9_slice_instance *instance,
 	struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
 	int ret;
 
-	ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance);
+	ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance, bs);
 	if (ret)
 		goto err;
 
-- 
2.18.0

Re: [PATCH v2,4/4] media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

[Daniel Almeida]

> On 14 Mar 2024, at 08:44, Yunfei Dong <yunfei.dong@mediatek.com> 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.
> 
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> ---
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 22 +++++++++----
> .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 25 ++++++--------
> .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 33 ++++++++-----------
> 3 files changed, 40 insertions(+), 40 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 3060768e0ea9..bb2680f3ec5b 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
> @@ -328,7 +328,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 = 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);
> @@ -363,7 +363,7 @@ static void mtk_vdec_worker(struct work_struct *work)
> mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
> 
> ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);


Can you leave a comment explaining why the != -EAGAIN check was
removed? Doesn’t seem obvious to me.


> - if (ret && ret != -EAGAIN) {
> + if (ret) {
> mtk_v4l2_vdec_err(ctx,
>  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu ret=%d res_chg=%d",
>  ctx->id, vb2_src->index, bs_src->size,
> @@ -380,11 +380,21 @@ 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);
> + if (vb2_v4l2_src)
> + v4l2_m2m_buf_done(vb2_v4l2_src, state);
> +
> v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> + } else {
> + if (!ret) {
> + v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> + } else {
> + if (src_buf_req)
> + v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);

vb2_v4l2_src can’t really be NULL here due to this:

```
	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);
		return;
	}
```

I must say I find the control flow here a bit confusing, so I wonder if this block can’t go
into an inline helper to clean up stuff a bit:

```
			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);

			v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
```

Also, I dislike that this apparently fails silently if the pointers are NULL. It is
not clear at a first glance if you’re just being careful or if you legitimately expect
`src_buf_req` to possibly be NULL at this point for whatever reason. The lifecycle
of request objects is not trivial, so it’s good to be explicit here even if this means
only leaving a comment or similar

— Daniel

> + if (vb2_v4l2_src)
> + 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 f277b907c345..339c5c88da1a 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
> @@ -1052,23 +1052,18 @@ static inline void vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
> memcpy(remote_vsi, vsi, sizeof(*vsi));
> }
> 
> -static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_instance *instance,
> - struct vdec_av1_slice_vsi *vsi,
> - struct vdec_lat_buf *lat_buf)
> +static int vdec_av1_slice_setup_lat_from_src_buf(struct vdec_av1_slice_vsi *vsi,
> + struct vdec_lat_buf *lat_buf,
> + struct mtk_vcodec_mem *bs)
> {
> - struct vb2_v4l2_buffer *src;
> - struct vb2_v4l2_buffer *dst;
> -
> - src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
> - if (!src)
> - return -EINVAL;
> + struct mtk_video_dec_buf *src_buf_info;
> 
> - lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
> - lat_buf->vb2_v4l2_src = src;
> + 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;
> 
> - dst = &lat_buf->ts_info;
> - v4l2_m2m_buf_copy_metadata(src, dst, true);
> - vsi->frame.cur_ts = dst->vb2_buf.timestamp;
> + v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
> + vsi->frame.cur_ts = lat_buf->ts_info.vb2_buf.timestamp;
> 
> return 0;
> }
> @@ -1717,7 +1712,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(vsi, lat_buf, bs);
> 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 0dedbc3680e8..2697e04f4313 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
> @@ -693,39 +693,34 @@ static int vdec_vp9_slice_tile_offset(int idx, int mi_num, int tile_log2)
> }
> 
> static
> -int vdec_vp9_slice_setup_single_from_src_to_dst(struct vdec_vp9_slice_instance *instance)
> +int vdec_vp9_slice_setup_single_from_src_to_dst(struct vdec_vp9_slice_instance *instance,
> + struct mtk_vcodec_mem *bs)
> {
> - 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)
> - return -EINVAL;
> + src_buf_info = container_of(bs, struct mtk_video_dec_buf, bs_buffer);
> 
> dst = v4l2_m2m_next_dst_buf(instance->ctx->m2m_ctx);
> if (!dst)
> return -EINVAL;
> 
> - v4l2_m2m_buf_copy_metadata(src, dst, true);
> + v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, dst, true);
> 
> return 0;
> }
> 
> -static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_vp9_slice_instance *instance,
> - struct vdec_lat_buf *lat_buf)
> +static int vdec_vp9_slice_setup_lat_from_src_buf(struct vdec_lat_buf *lat_buf,
> + struct mtk_vcodec_mem *bs)
> {
> - 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)
> - return -EINVAL;
> + 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;
> 
> - lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
> - lat_buf->vb2_v4l2_src = src;
> + v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
> 
> - dst = &lat_buf->ts_info;
> - v4l2_m2m_buf_copy_metadata(src, dst, true);
> return 0;
> }
> 
> @@ -1155,7 +1150,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(lat_buf, bs);
> if (ret)
> goto err;
> 
> @@ -1796,7 +1791,7 @@ static int vdec_vp9_slice_setup_single(struct vdec_vp9_slice_instance *instance,
> struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
> int ret;
> 
> - ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance);
> + ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance, bs);
> if (ret)
> goto err;
> 
> -- 
> 2.18.0
> 
> 

Re: [PATCH v2,4/4] media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

[Yunfei Dong (董云飞)]

Hi Daniel,

Sorry to reply your suggestion too later.

Thanks for your help to review the patch.

On Fri, 2024-03-15 at 15:52 -0300, Daniel Almeida wrote:
> > On 14 Mar 2024, at 08:44, Yunfei Dong <yunfei.dong@mediatek.com>
> > 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.
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> > .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 22 +++++++++----
> > .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 25 ++++++--------
> > .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 33 ++++++++------
> > -----
> > 3 files changed, 40 insertions(+), 40 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 3060768e0ea9..bb2680f3ec5b 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
> > @@ -328,7 +328,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 = 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);
> > @@ -363,7 +363,7 @@ static void mtk_vdec_worker(struct work_struct
> > *work)
> > mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
> > 
> > ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);
> 
> 
> Can you leave a comment explaining why the != -EAGAIN check was
> removed? Doesn’t seem obvious to me.
> 
Need to support -EAGAIN condition, change the logic in patch v3.
> 
> > - if (ret && ret != -EAGAIN) {
> > + if (ret) {
> > mtk_v4l2_vdec_err(ctx,
> >  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu ret=%d res_chg=%d",
> >  ctx->id, vb2_src->index, bs_src->size,
> > @@ -380,11 +380,21 @@ 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);
> > + if (vb2_v4l2_src)
> > + v4l2_m2m_buf_done(vb2_v4l2_src, state);
> > +
> > v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > + } else {
> > + if (!ret) {
> > + v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > + } else {
> > + if (src_buf_req)
> > + v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
> 
> vb2_v4l2_src can’t really be NULL here due to this:
> 
> ```
> 	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);
> 		return;
> 	}
> ```
> 
> I must say I find the control flow here a bit confusing, so I wonder
> if this block can’t go
> into an inline helper to clean up stuff a bit:
> 
> ```
> 			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);
> 
> 			v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx-
> >m2m_ctx);
> ```
> 

> Also, I dislike that this apparently fails silently if the pointers
> are NULL. It is
> not clear at a first glance if you’re just being careful or if you
> legitimately expect
> `src_buf_req` to possibly be NULL at this point for whatever reason.
> The lifecycle
> of request objects is not trivial, so it’s good to be explicit here
> even if this means
> only leaving a comment or similar
> 
> — Daniel
> 
In order to make the code logic more readable, add some comments to
describe the reason.

Change the code flow to remove NULL checking and to more test.

Best Regards,
Yunfei Dong
> > + if (vb2_v4l2_src)
> > + 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 f277b907c345..339c5c88da1a 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
> > @@ -1052,23 +1052,18 @@ static inline void
> > vdec_av1_slice_vsi_to_remote(struct vdec_av1_slice_vsi *vsi,
> > memcpy(remote_vsi, vsi, sizeof(*vsi));
> > }
> > 
> > -static int vdec_av1_slice_setup_lat_from_src_buf(struct
> > vdec_av1_slice_instance *instance,
> > - struct vdec_av1_slice_vsi *vsi,
> > - struct vdec_lat_buf *lat_buf)
> > +static int vdec_av1_slice_setup_lat_from_src_buf(struct
> > vdec_av1_slice_vsi *vsi,
> > + struct vdec_lat_buf *lat_buf,
> > + struct mtk_vcodec_mem *bs)
> > {
> > - struct vb2_v4l2_buffer *src;
> > - struct vb2_v4l2_buffer *dst;
> > -
> > - src = v4l2_m2m_next_src_buf(instance->ctx->m2m_ctx);
> > - if (!src)
> > - return -EINVAL;
> > + struct mtk_video_dec_buf *src_buf_info;
> > 
> > - lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
> > - lat_buf->vb2_v4l2_src = src;
> > + 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;
> > 
> > - dst = &lat_buf->ts_info;
> > - v4l2_m2m_buf_copy_metadata(src, dst, true);
> > - vsi->frame.cur_ts = dst->vb2_buf.timestamp;
> > + v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf-
> > >ts_info, true);
> > + vsi->frame.cur_ts = lat_buf->ts_info.vb2_buf.timestamp;
> > 
> > return 0;
> > }
> > @@ -1717,7 +1712,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(vsi, lat_buf, bs);
> > 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 0dedbc3680e8..2697e04f4313 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
> > @@ -693,39 +693,34 @@ static int vdec_vp9_slice_tile_offset(int
> > idx, int mi_num, int tile_log2)
> > }
> > 
> > static
> > -int vdec_vp9_slice_setup_single_from_src_to_dst(struct
> > vdec_vp9_slice_instance *instance)
> > +int vdec_vp9_slice_setup_single_from_src_to_dst(struct
> > vdec_vp9_slice_instance *instance,
> > + struct mtk_vcodec_mem *bs)
> > {
> > - 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)
> > - return -EINVAL;
> > + src_buf_info = container_of(bs, struct mtk_video_dec_buf,
> > bs_buffer);
> > 
> > dst = v4l2_m2m_next_dst_buf(instance->ctx->m2m_ctx);
> > if (!dst)
> > return -EINVAL;
> > 
> > - v4l2_m2m_buf_copy_metadata(src, dst, true);
> > + v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, dst, true);
> > 
> > return 0;
> > }
> > 
> > -static int vdec_vp9_slice_setup_lat_from_src_buf(struct
> > vdec_vp9_slice_instance *instance,
> > - struct vdec_lat_buf *lat_buf)
> > +static int vdec_vp9_slice_setup_lat_from_src_buf(struct
> > vdec_lat_buf *lat_buf,
> > + struct mtk_vcodec_mem *bs)
> > {
> > - 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)
> > - return -EINVAL;
> > + 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;
> > 
> > - lat_buf->src_buf_req = src->vb2_buf.req_obj.req;
> > - lat_buf->vb2_v4l2_src = src;
> > + v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf-
> > >ts_info, true);
> > 
> > - dst = &lat_buf->ts_info;
> > - v4l2_m2m_buf_copy_metadata(src, dst, true);
> > return 0;
> > }
> > 
> > @@ -1155,7 +1150,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(lat_buf, bs);
> > if (ret)
> > goto err;
> > 
> > @@ -1796,7 +1791,7 @@ static int vdec_vp9_slice_setup_single(struct
> > vdec_vp9_slice_instance *instance,
> > struct vdec_vp9_slice_vsi *vsi = &pfc->vsi;
> > int ret;
> > 
> > - ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance);
> > + ret = vdec_vp9_slice_setup_single_from_src_to_dst(instance, bs);
> > if (ret)
> > goto err;
> > 
> > -- 
> > 2.18.0
> > 
> > 
> 
> 

Re: [PATCH v2,0/4] media: mediatek: vcodec: fix ctrl request complete fail

[Daniel Almeida]

Hi Yunfei!

I say this very respectfully, but I believe that you must improve the wording of some of your commit messages. It is hard to understand your changes otherwise. More importantly, it is hard to understand why they are needed or what exactly they fix.

This series has some checkpatch errors: 

total: 53 errors, 0 warnings, 0 checks, 1047 lines checked

Did you test this with Fluster? We should really make sure that this does not regress any of the tests there.

See a few comments from me in the thread

— Daniel

> On 14 Mar 2024, at 08:44, Yunfei Dong <yunfei.dong@mediatek.com> wrote:
> 
> Moving v4l2_ctrl_request_complete to before of function
> v4l2_m2m_buf_done to make sure the status of request correctly.
> 
> Replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove to
> make sure the src buffer won't be removed for some unknown
> reason leading to buffer done error.
> 
> Patch 1 setting request complete before buffer done
> Patch 3 flush decoder before remove all source buffer
> Patch 2 change flush decode from capture to output when stream off
> Patch 4 replace v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove
> ---
> compared with v1:
> - add patch 2/3/4 to fix timing issue.
> ---
> Yunfei Dong (4):
>  media: mediatek: vcodec: setting request complete before buffer done
>  media: mediatek: vcodec: change flush decode from capture to output
>    when stream off
>  media: mediatek: vcodec: flush decoder before remove all source buffer
>  media: mediatek: vcodec: replace v4l2_m2m_next_src_buf with
>    v4l2_m2m_src_buf_remove
> 
> .../mediatek/vcodec/decoder/mtk_vcodec_dec.c  | 52 +++++++++----------
> .../vcodec/decoder/mtk_vcodec_dec_drv.h       |  3 +-
> .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 28 +++++++---
> .../vcodec/decoder/vdec/vdec_av1_req_lat_if.c | 28 +++++-----
> .../decoder/vdec/vdec_h264_req_multi_if.c     |  3 +-
> .../decoder/vdec/vdec_hevc_req_multi_if.c     |  3 +-
> .../vcodec/decoder/vdec/vdec_vp9_req_lat_if.c | 37 ++++++-------
> .../mediatek/vcodec/decoder/vdec_msg_queue.h  |  2 +
> 8 files changed, 84 insertions(+), 72 deletions(-)
> 
> -- 
> 2.18.0
> 
>