[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 at 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
>
>