| Message ID | 20240807082444.21280-7-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: >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. This is really hard to understand, can try wording this a bit clearer? Stuff like: if the source buffer is removed ... will lead to remove source buffer, just leaves me scratching my head. And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf` so I suppose you mean something else when you say that there is no lock to protect the source buffer? You might not know all reasons but for this commit description you should at least know one reason. Please highlight a case how this can happen, so that you can justify the change. > >Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> >--- > .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++------ > 1 file changed, 21 insertions(+), 9 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 8aa379872ddc..3dba3549000a 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 >@@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct *work) > 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 = ctx->last_vb2_v4l2_src; >+ struct vb2_v4l2_buffer *vb2_v4l2_dst; > struct vb2_buffer *vb2_src; > struct mtk_vcodec_mem *bs_src; > struct mtk_video_dec_buf *dec_buf_src; >@@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct *work) > bool res_chg = false; > int ret; > >- vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx); >+ vb2_v4l2_src = vb2_v4l2_src ? 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); >@@ -381,17 +382,28 @@ 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); >- ctx->last_vb2_v4l2_src = NULL; >- } else { >- ctx->last_vb2_v4l2_src = vb2_v4l2_src; >- } >+ vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx); >+ v4l2_m2m_buf_done(vb2_v4l2_dst, state); >+ v4l2_m2m_buf_done(vb2_v4l2_src, state); This is another case where you just remove again completely what you have added in the previous patch. > > v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx); >+ return; > } >+ >+ /* If each codec return -EAGAIN to decode again, need to backup current source >+ * buffer, then the driver will get this buffer next time. I would reword this like: /* Store the current source buffer for the next attempt to decode, * if this decode returned -EAGAIN */ >+ * >+ * If each codec decode error, must to set buffer done with error status for >+ * this buffer have been removed from ready list. >+ */ >+ ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src; Okay and here you add the same thing again as in the previous patch but differently, this collection of commits feels more and more to me like a work in progress. Please make sure in the future that each commit does one job and does it completely. It is not only confussing but also makes it hard to read the changes as the bigger picture is missing in these tiny commits. Please try to combine the patches where possible. Regards, Sebastian Fricke >+ if (ret && ret != -EAGAIN) { >+ if (src_buf_req) >+ v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl); >+ v4l2_m2m_buf_done(vb2_v4l2_src, state); >+ } >+ >+ v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx); > } > > static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb) >-- >2.46.0 > >
Hi Sebastian, Thanks for your suggestion. On Fri, 2024-08-23 at 17:23 +0200, Sebastian Fricke wrote: > Hey Yunfei, > > On 07.08.2024 16:24, Yunfei Dong 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. > > This is really hard to understand, can try wording this a bit > clearer? > Stuff like: if the source buffer is removed ... will lead to remove > source buffer, just leaves me scratching my head. > And there is a spinlock in the m2m framework in `v4l2_m2m_next_buf` > so I > suppose you mean something else when you say that there is no lock to > protect the source buffer? > > You might not know all reasons but for this commit description you > should at least know one reason. Please highlight a case how this can > happen, so that you can justify the change. > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++--- > > --- > > 1 file changed, 21 insertions(+), 9 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 8aa379872ddc..3dba3549000a 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 > > @@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct > > *work) > > 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 = ctx->last_vb2_v4l2_src; > > + struct vb2_v4l2_buffer *vb2_v4l2_dst; > > struct vb2_buffer *vb2_src; > > struct mtk_vcodec_mem *bs_src; > > struct mtk_video_dec_buf *dec_buf_src; > > @@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct > > *work) > > bool res_chg = false; > > int ret; > > > > - vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : > > v4l2_m2m_next_src_buf(ctx->m2m_ctx); > > + vb2_v4l2_src = vb2_v4l2_src ? 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); > > @@ -381,17 +382,28 @@ 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); > > - ctx->last_vb2_v4l2_src = NULL; > > - } else { > > - ctx->last_vb2_v4l2_src = vb2_v4l2_src; > > - } > > + vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx); > > + v4l2_m2m_buf_done(vb2_v4l2_dst, state); > > + v4l2_m2m_buf_done(vb2_v4l2_src, state); > > This is another case where you just remove again completely what you > have added in the previous patch. > > > > > v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx); > > + return; > > } > > + > > + /* If each codec return -EAGAIN to decode again, need to backup > > current source > > + * buffer, then the driver will get this buffer next time. > > I would reword this like: > > /* Store the current source buffer for the next attempt to > decode, > * if this decode returned -EAGAIN */ > > > + * > > + * If each codec decode error, must to set buffer done with > > error status for > > + * this buffer have been removed from ready list. > > + */ > > + ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : > > vb2_v4l2_src; > > Okay and here you add the same thing again as in the previous patch > but > differently, this collection of commits feels more and more to me > like a > work in progress. Please make sure in the future that each commit > does > one job and does it completely. > It is not only confussing but also makes it hard to read the changes > as > the bigger picture is missing in these tiny commits. > > Please try to combine the patches where possible. > Path 5/7 is used to store the source buffer. path 6/7 is used to handle decoder again when decoder error. I will check whether it's possible to combine patch 5 and 6 together. Best Regards, Yunfei Dong > Regards, > Sebastian Fricke > > > + if (ret && ret != -EAGAIN) { > > + if (src_buf_req) > > + v4l2_ctrl_request_complete(src_buf_req, &ctx- > > >ctrl_hdl); > > + v4l2_m2m_buf_done(vb2_v4l2_src, state); > > + } > > + > > + v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx); > > } > > > > static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb) > > -- > > 2.46.0 > > > >
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 8aa379872ddc..3dba3549000a 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 @@ -321,6 +321,7 @@ static void mtk_vdec_worker(struct work_struct *work) 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 = ctx->last_vb2_v4l2_src; + struct vb2_v4l2_buffer *vb2_v4l2_dst; struct vb2_buffer *vb2_src; struct mtk_vcodec_mem *bs_src; struct mtk_video_dec_buf *dec_buf_src; @@ -329,7 +330,7 @@ static void mtk_vdec_worker(struct work_struct *work) bool res_chg = false; int ret; - vb2_v4l2_src = vb2_v4l2_src ? vb2_v4l2_src : v4l2_m2m_next_src_buf(ctx->m2m_ctx); + vb2_v4l2_src = vb2_v4l2_src ? 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); @@ -381,17 +382,28 @@ 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); - ctx->last_vb2_v4l2_src = NULL; - } else { - ctx->last_vb2_v4l2_src = vb2_v4l2_src; - } + vb2_v4l2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx); + v4l2_m2m_buf_done(vb2_v4l2_dst, state); + v4l2_m2m_buf_done(vb2_v4l2_src, state); v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx); + return; } + + /* If each codec return -EAGAIN to decode again, need to backup current source + * buffer, then the driver will get this buffer next time. + * + * If each codec decode error, must to set buffer done with error status for + * this buffer have been removed from ready list. + */ + ctx->last_vb2_v4l2_src = (ret != -EAGAIN) ? NULL : vb2_v4l2_src; + if (ret && ret != -EAGAIN) { + if (src_buf_req) + v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl); + v4l2_m2m_buf_done(vb2_v4l2_src, state); + } + + v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx); } static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
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@mediatek.com> --- .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-)