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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240823141454.vvtbkkj5qm4pmpfr@basti-XPS-13-9310>
Date: Fri, 23 Aug 2024 16:14:54 +0200
From: Sebastian Fricke <sebastian.fricke@...labora.com>
To: Yunfei Dong <yunfei.dong@...iatek.com>
Cc: Nícolas F . R . A . Prado <nfraprado@...labora.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Benjamin Gaignard <benjamin.gaignard@...labora.com>,
	Nathan Hebert <nhebert@...omium.org>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Hsin-Yi Wang <hsinyi@...omium.org>,
	Fritz Koenig <frkoenig@...omium.org>,
	Daniel Vetter <daniel@...ll.ch>, Steve Cho <stevecho@...omium.org>,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v4 5/7] media: mediatek: vcodec: store source vb2 buffer

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@...iatek.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
>
>