Message-ID: <0f6897a6e080b63873b997ec54e65707d15c7c0c.camel@mediatek.com>
Date: Thu, 13 Jun 2024 07:49:07 +0000
From: Yunfei Dong (董云飞) <Yunfei.Dong@...iatek.com>
To: "daniel.almeida@...labora.com" <daniel.almeida@...labora.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"frkoenig@...omium.org" <frkoenig@...omium.org>, "stevecho@...omium.org"
	<stevecho@...omium.org>, "nhebert@...omium.org" <nhebert@...omium.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"nicolas.dufresne@...labora.com" <nicolas.dufresne@...labora.com>,
	"daniel@...ll.ch" <daniel@...ll.ch>, Project_Global_Chrome_Upstream_Group
	<Project_Global_Chrome_Upstream_Group@...iatek.com>,
	"benjamin.gaignard@...labora.com" <benjamin.gaignard@...labora.com>,
	"sebastian.fricke@...labora.com" <sebastian.fricke@...labora.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "hverkuil-cisco@...all.nl"
	<hverkuil-cisco@...all.nl>, "hsinyi@...omium.org" <hsinyi@...omium.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "nfraprado@...labora.com"
	<nfraprado@...labora.com>
Subject: Re: [PATCH v2,4/4] media: mediatek: vcodec: replace
 v4l2_m2m_next_src_buf with v4l2_m2m_src_buf_remove

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

Powered by blists - more mailing lists