[PATCH V1, 4/6] media: mtk-jpegdec: add jpeg decode worker interface

[kyrie.wu]

On Mon, 2021-12-06 at 17:26 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/12/21 06:34, kyrie.wu ha scritto:
> > Add jpeg decoding worker to ensure that three HWs
> > run in parallel in MT8195.
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu at mediatek.com>
> > ---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 190
> > +++++++++++++++++++---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |   5 +
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c |  17 ++
> >   3 files changed, 189 insertions(+), 23 deletions(-)
> > 
> 
> Hello Kyrie,
> thanks for the patch! However, there are some things to improve...
> 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index f2a5e84..2518660 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -1065,57 +1065,187 @@ static void mtk_jpeg_enc_device_run(void
> > *priv)
> >   	queue_work(jpeg->workqueue, &ctx->jpeg_work);
> >   }
> >   
> > -static void mtk_jpeg_dec_device_run(void *priv)
> > +static int mtk_jpegdec_select_hw(struct mtk_jpeg_ctx *ctx)
> >   {
> > -	struct mtk_jpeg_ctx *ctx = priv;
> > +	struct mtk_jpegdec_comp_dev *comp_jpeg;
> >   	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > -	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > -	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> >   	unsigned long flags;
> > -	struct mtk_jpeg_src_buf *jpeg_src_buf;
> > +	int hw_id = -1;
> > +	int i;
> > +
> > +	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++) {
> > +		comp_jpeg = jpeg->dec_hw_dev[i];
> > +		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {
> > +			hw_id = i;
> > +			comp_jpeg->hw_state = MTK_JPEG_HW_BUSY;
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> > +
> > +	return hw_id;
> > +}
> > +
> > +static int mtk_jpegdec_deselect_hw(struct mtk_jpeg_dev *jpeg, int
> > hw_id)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +	jpeg->dec_hw_dev[hw_id]->hw_state =
> > +		MTK_JPEG_HW_IDLE;
> > +	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_jpegdec_set_hw_param(struct mtk_jpeg_ctx *ctx,
> > +	int hw_id,
> > +	struct vb2_v4l2_buffer *src_buf,
> > +	struct vb2_v4l2_buffer *dst_buf)
> > +{
> > +	struct mtk_jpegdec_comp_dev *jpeg =
> > +		ctx->jpeg->dec_hw_dev[hw_id];
> > +
> > +	jpeg->hw_param.curr_ctx = ctx;
> > +	jpeg->hw_param.src_buffer = src_buf;
> > +	jpeg->hw_param.dst_buffer = dst_buf;
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_jpegdec_worker(struct work_struct *work)
> > +{
> > +	struct mtk_jpeg_ctx *ctx = container_of(work, struct
> > mtk_jpeg_ctx,
> > +		jpeg_work);
> > +	struct mtk_jpegdec_comp_dev *comp_jpeg[MTK_JPEGDEC_HW_MAX];
> > +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > +	struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
> > +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > +	atomic_t *hw_rdy[MTK_JPEGDEC_HW_MAX];
> > +	struct clk *jpegdec_clk;
> > +	int ret, i, hw_id = 0;
> >   	struct mtk_jpeg_bs bs;
> >   	struct mtk_jpeg_fb fb;
> > -	int ret;
> > +	unsigned long flags;
> > +
> > +	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++) {
> > +		comp_jpeg[i] = jpeg->dec_hw_dev[i];
> > +		hw_rdy[i] = &comp_jpeg[i]->hw_rdy;
> > +	}
> > +
> 
> This entire retry_select block should go to a helper function instead
> of being inside of here.
> 
> Also, there's a big issue with this snippet that has to be solved:
> you're
> unconditionally calling "goto retry_select" at the end of the if
> branch,
> but please consider the following scenario:
> 
> - mtk_jpegdec_select_hw() returns a hw_id < 0
> - wait_event_interruptible returns 0
> ... then we redo the same, and we still get the same result.
> 
> This may generate an infinite loop!!
> 
> After putting this into a separate function, please use a controlled
> loop
> with a well thought maximum number of retries, as to avoid this
> situation.
Dear Angelo,
I will use wait_event_interruptible_timeout to replace
wait_event_interruptible to avoid the situation you memtioned above and
use a helper function instead those code.
> 
> > +retry_select:
> > +	hw_id = mtk_jpegdec_select_hw(ctx);
> > +	if (hw_id < 0) {
> > +		ret = wait_event_interruptible(jpeg->dec_hw_wq,
> > +			(atomic_read(hw_rdy[0]) ||
> > +			atomic_read(hw_rdy[1]) ||
> > +			atomic_read(hw_rdy[2])) > 0);
> > +		if (ret != 0) {
> > +			dev_err(jpeg->dev, "%s : %d, all HW are
> > busy\n",
> > +				__func__, __LINE__);
> > +			v4l2_m2m_job_finish(jpeg->m2m_dev, ctx-
> > >fh.m2m_ctx);
> > +			return;
> > +		}
> > +
> > +		dev_err(jpeg->dev, "%s : %d, NEW HW IDLE, please retry
> > selcet!!!\n",
> > +			__func__, __LINE__);
> > +		goto retry_select;
> > +	}
> >   
> > +	atomic_dec(&comp_jpeg[hw_id]->hw_rdy);
> >   	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > +	if (!src_buf) {
> > +		dev_err(jpeg->dev, "%s : %d, get src_buf fail !!!\n",
> > +			__func__, __LINE__);
> > +		goto getbuf_fail;
> > +	}
> >   	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +	if (!dst_buf) {
> > +		dev_err(jpeg->dev, "%s : %d, get dst_buf fail !!!\n",
> > +			__func__, __LINE__);
> > +		goto getbuf_fail;
> > +	}
> > +
> >   	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> > +	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
> >   
> > -	if (mtk_jpeg_check_resolution_change(ctx, &jpeg_src_buf-
> > >dec_param)) {
> > +	if (mtk_jpeg_check_resolution_change(ctx,
> > +		&jpeg_src_buf->dec_param)) {
> 
> Why are you breaking this line? There's no need to.
If the resolution changed, all input and output buffer would be free
and the new size buffer would be malloc to match the lastest resolution
for jpeg decode.
> 
> >   		mtk_jpeg_queue_src_chg_event(ctx);
> >   		ctx->state = MTK_JPEG_SOURCE_CHANGE;
> > -		v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > -		return;
> > +		goto getbuf_fail;
> >   	}
> >   
> 
> 
> Regards,
> - Angelo