| Message ID | 20240807082444.21280-6-yunfei.dong@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: mediatek: vcodec: fix v4l2_ctrl_request_complete fail | expand |
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_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 > >
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(-)