| Message ID | 20240807082444.21280-6-yunfei.dong@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail | expand |
Hi Sebastian, Thanks for your suggestion. On Fri, 2024-08-23 at 16:14 +0200, Sebastian Fricke wrote: > 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@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_sta > > teless.c > > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta > > teless.c > > index 2a7e4fe24ed3..8aa379872ddc 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 > > @@ -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. > As the commit wrote, if the driver need to decoder again, need to use the last remove source buffer to decode again, not to get one new. Regards, Yunfei Dong > 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 > > > > > >
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. * @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; 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; 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); 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); } }
Store the current vb2 buffer when lat need to decode again. Then lat work can get the same vb2 buffer directly next time. Signed-off-by: Yunfei Dong <yunfei.dong@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(-)