Message-Id: <69C5D24C-D33A-4F1E-B9A9-6C32AB1951BA@collabora.com>
Date: Fri, 15 Mar 2024 15:52:01 -0300
From: Daniel Almeida <daniel.almeida@...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>,
 Sebastian Fricke <sebastian.fricke@...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 v2,4/4] media: mediatek: vcodec: replace
 v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove



> On 14 Mar 2024, at 08:44, Yunfei Dong <yunfei.dong@...iatek.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@...iatek.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
> 
> 

Powered by blists - more mailing lists