| Message ID | 1638509655-14296-5-git-send-email-kyrie.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support multi-hardware jpeg decoding using of_platform_populate | expand |
kyrie.wu wrote: > Add jpeg decoding worker to ensure that three HWs > run in parallel in MT8195. > > Signed-off-by: kyrie.wu <kyrie.wu@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(-) > > 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; > + } > + > +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)) { > 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; > } > > - ret = pm_runtime_resume_and_get(jpeg->dev); > - if (ret < 0) > + jpeg_src_buf->curr_ctx = ctx; > + jpeg_src_buf->frame_num = ctx->total_frame_num; > + jpeg_dst_buf->curr_ctx = ctx; > + jpeg_dst_buf->frame_num = ctx->total_frame_num; > + ctx->total_frame_num++; > + > + v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > + v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); > + > + mtk_jpegdec_set_hw_param(ctx, hw_id, src_buf, dst_buf); > + ret = pm_runtime_get_sync(comp_jpeg[hw_id]->pm.dev); > + if (ret < 0) { > + dev_err(jpeg->dev, "%s : %d, pm_runtime_get_sync fail !!!\n", > + __func__, __LINE__); > goto dec_end; > + } > > - schedule_delayed_work(&jpeg->job_timeout_work, > - msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC)); > + jpegdec_clk = comp_jpeg[hw_id]->pm.dec_clk.clk_info->jpegdec_clk; > + ret = clk_prepare_enable(jpegdec_clk); > + if (ret) { > + dev_err(jpeg->dev, "%s : %d, jpegdec clk_prepare_enable fail\n", > + __func__, __LINE__); > + goto clk_end; > + } > + > + schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work, > + msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC)); > > mtk_jpeg_set_dec_src(ctx, &src_buf->vb2_buf, &bs); > - if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, &dst_buf->vb2_buf, &fb)) > - goto dec_end; > + if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, > + &dst_buf->vb2_buf, &fb)) { > + dev_err(jpeg->dev, "%s : %d, mtk_jpeg_set_dec_dst fail\n", > + __func__, __LINE__); > + goto setdst_end; > + } > > - spin_lock_irqsave(&jpeg->hw_lock, flags); > - mtk_jpeg_dec_reset(jpeg->reg_base); > + spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags); > + mtk_jpeg_dec_reset(comp_jpeg[hw_id]->reg_base); > mtk_jpeg_dec_set_config(jpeg->reg_base, > - &jpeg_src_buf->dec_param, &bs, &fb); > + &jpeg_src_buf->dec_param, > + &bs, &fb); > + mtk_jpeg_dec_start(comp_jpeg[hw_id]->reg_base); > + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx); > + spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags); > > - mtk_jpeg_dec_start(jpeg->reg_base); > - spin_unlock_irqrestore(&jpeg->hw_lock, flags); > return; > > +setdst_end: > + clk_disable_unprepare(jpegdec_clk); > +clk_end: > + pm_runtime_put(comp_jpeg[hw_id]->pm.dev); > dec_end: > - v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > - v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); > v4l2_m2m_buf_done(src_buf, buf_state); > v4l2_m2m_buf_done(dst_buf, buf_state); > +getbuf_fail: > + atomic_inc(&comp_jpeg[hw_id]->hw_rdy); > + mtk_jpegdec_deselect_hw(jpeg, hw_id); > v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx); > } > > +static void mtk_jpeg_dec_device_run(void *priv) > +{ > + struct mtk_jpeg_ctx *ctx = priv; > + struct mtk_jpeg_dev *jpeg = ctx->jpeg; > + > + queue_work(jpeg->dec_workqueue, &ctx->jpeg_work); > +} > + > static int mtk_jpeg_dec_job_ready(void *priv) > { > struct mtk_jpeg_ctx *ctx = priv; > @@ -1334,6 +1464,8 @@ static int mtk_jpeg_open(struct file *file) > > if (jpeg->variant->is_encoder) > INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker); > + else > + INIT_WORK(&ctx->jpeg_work, mtk_jpegdec_worker); > > INIT_LIST_HEAD(&ctx->dst_done_queue); > v4l2_fh_init(&ctx->fh, vfd); > @@ -1481,7 +1613,17 @@ static int mtk_jpeg_probe(struct platform_device *pdev) > jpeg->workqueue = alloc_ordered_workqueue(MTK_JPEG_NAME, > WQ_MEM_RECLAIM | WQ_FREEZABLE); > if (!jpeg->workqueue) { > - dev_err(&pdev->dev, "Failed to create jpeg workqueue!\n"); > + dev_err(&pdev->dev, "Failed to create jpegenc workqueue!\n"); > + ret = -EINVAL; > + goto err_alloc_workqueue; > + } > + } else { > + init_waitqueue_head(&jpeg->dec_hw_wq); > + > + jpeg->dec_workqueue = alloc_ordered_workqueue(MTK_JPEG_NAME, > + WQ_MEM_RECLAIM | WQ_FREEZABLE); > + if (!jpeg->dec_workqueue) { > + dev_err(&pdev->dev, "Failed to create jpegdec workqueue!\n"); > ret = -EINVAL; > goto err_alloc_workqueue; > } > @@ -1571,6 +1713,8 @@ static int mtk_jpeg_remove(struct platform_device *pdev) > mtk_jpeg_clk_release(jpeg); > flush_workqueue(jpeg->workqueue); > destroy_workqueue(jpeg->workqueue); > + flush_workqueue(jpeg->dec_workqueue); > + destroy_workqueue(jpeg->dec_workqueue); > > return 0; > } > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > index b7102db..d67c49f 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > @@ -202,6 +202,9 @@ struct mtk_jpegdec_comp_dev { > int jpegdec_irq; > struct delayed_work job_timeout_work; > struct mtk_jpeg_hw_param hw_param; > + atomic_t hw_rdy; > + enum mtk_jpeg_hw_state hw_state; > + spinlock_t hw_lock; > }; > > /** > @@ -239,6 +242,8 @@ struct mtk_jpeg_dev { > > void __iomem *reg_decbase[MTK_JPEGDEC_HW_MAX]; > struct mtk_jpegdec_comp_dev *dec_hw_dev[MTK_JPEGDEC_HW_MAX]; > + wait_queue_head_t dec_hw_wq; > + struct workqueue_struct *dec_workqueue; > }; > > /** > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c > index e295576..9138ecb 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c > @@ -449,6 +449,7 @@ static void mtk_jpegdec_timeout_work(struct work_struct *work) > struct mtk_jpegdec_comp_dev *cjpeg = > container_of(work, struct mtk_jpegdec_comp_dev, > job_timeout_work.work); > + struct mtk_jpeg_dev *master_jpeg = cjpeg->master_dev; > struct vb2_v4l2_buffer *src_buf, *dst_buf; > > src_buf = cjpeg->hw_param.src_buffer; > @@ -457,6 +458,9 @@ static void mtk_jpegdec_timeout_work(struct work_struct *work) > mtk_jpeg_dec_reset(cjpeg->reg_base); > clk_disable_unprepare(cjpeg->pm.dec_clk.clk_info->jpegdec_clk); > pm_runtime_put(cjpeg->pm.dev); > + cjpeg->hw_state = MTK_JPEG_HW_IDLE; > + atomic_inc(&cjpeg->hw_rdy); > + wake_up(&master_jpeg->dec_hw_wq); > v4l2_m2m_buf_done(src_buf, buf_state); > v4l2_m2m_buf_done(dst_buf, buf_state); > } > @@ -557,8 +561,18 @@ static irqreturn_t mtk_jpegdec_hw_irq_handler(int irq, void *priv) > v4l2_m2m_buf_done(src_buf, buf_state); > v4l2_m2m_buf_done(dst_buf, buf_state); > v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx); > + clk_disable_unprepare(jpeg->pm.dec_clk.clk_info->jpegdec_clk); > pm_runtime_put(ctx->jpeg->dev); > > + if (ctx->fh.m2m_ctx && > + (!list_empty(&ctx->fh.m2m_ctx->out_q_ctx.rdy_queue) || > + !list_empty(&ctx->fh.m2m_ctx->cap_q_ctx.rdy_queue))) > + queue_work(master_jpeg->workqueue, &ctx->jpeg_work); > + > + jpeg->hw_state = MTK_JPEG_HW_IDLE; > + wake_up(&master_jpeg->dec_hw_wq); > + atomic_inc(&jpeg->hw_rdy); > + > return IRQ_HANDLED; > } > > @@ -612,6 +626,9 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev) > return -ENOMEM; > > dev->plat_dev = pdev; > + atomic_set(&dev->hw_rdy, 1U); > + spin_lock_init(&dev->hw_lock); > + dev->hw_state = MTK_JPEG_HW_IDLE; > > INIT_DELAYED_WORK(&dev->job_timeout_work, mtk_jpegdec_timeout_work); > > -- > 2.6.4 > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek >
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@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. > +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. > 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
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@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
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; + } + +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)) { 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; } - ret = pm_runtime_resume_and_get(jpeg->dev); - if (ret < 0) + jpeg_src_buf->curr_ctx = ctx; + jpeg_src_buf->frame_num = ctx->total_frame_num; + jpeg_dst_buf->curr_ctx = ctx; + jpeg_dst_buf->frame_num = ctx->total_frame_num; + ctx->total_frame_num++; + + v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); + v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); + + mtk_jpegdec_set_hw_param(ctx, hw_id, src_buf, dst_buf); + ret = pm_runtime_get_sync(comp_jpeg[hw_id]->pm.dev); + if (ret < 0) { + dev_err(jpeg->dev, "%s : %d, pm_runtime_get_sync fail !!!\n", + __func__, __LINE__); goto dec_end; + } - schedule_delayed_work(&jpeg->job_timeout_work, - msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC)); + jpegdec_clk = comp_jpeg[hw_id]->pm.dec_clk.clk_info->jpegdec_clk; + ret = clk_prepare_enable(jpegdec_clk); + if (ret) { + dev_err(jpeg->dev, "%s : %d, jpegdec clk_prepare_enable fail\n", + __func__, __LINE__); + goto clk_end; + } + + schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work, + msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC)); mtk_jpeg_set_dec_src(ctx, &src_buf->vb2_buf, &bs); - if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, &dst_buf->vb2_buf, &fb)) - goto dec_end; + if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, + &dst_buf->vb2_buf, &fb)) { + dev_err(jpeg->dev, "%s : %d, mtk_jpeg_set_dec_dst fail\n", + __func__, __LINE__); + goto setdst_end; + } - spin_lock_irqsave(&jpeg->hw_lock, flags); - mtk_jpeg_dec_reset(jpeg->reg_base); + spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags); + mtk_jpeg_dec_reset(comp_jpeg[hw_id]->reg_base); mtk_jpeg_dec_set_config(jpeg->reg_base, - &jpeg_src_buf->dec_param, &bs, &fb); + &jpeg_src_buf->dec_param, + &bs, &fb); + mtk_jpeg_dec_start(comp_jpeg[hw_id]->reg_base); + v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx); + spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags); - mtk_jpeg_dec_start(jpeg->reg_base); - spin_unlock_irqrestore(&jpeg->hw_lock, flags); return; +setdst_end: + clk_disable_unprepare(jpegdec_clk); +clk_end: + pm_runtime_put(comp_jpeg[hw_id]->pm.dev); dec_end: - v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); - v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx); v4l2_m2m_buf_done(src_buf, buf_state); v4l2_m2m_buf_done(dst_buf, buf_state); +getbuf_fail: + atomic_inc(&comp_jpeg[hw_id]->hw_rdy); + mtk_jpegdec_deselect_hw(jpeg, hw_id); v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx); } +static void mtk_jpeg_dec_device_run(void *priv) +{ + struct mtk_jpeg_ctx *ctx = priv; + struct mtk_jpeg_dev *jpeg = ctx->jpeg; + + queue_work(jpeg->dec_workqueue, &ctx->jpeg_work); +} + static int mtk_jpeg_dec_job_ready(void *priv) { struct mtk_jpeg_ctx *ctx = priv; @@ -1334,6 +1464,8 @@ static int mtk_jpeg_open(struct file *file) if (jpeg->variant->is_encoder) INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker); + else + INIT_WORK(&ctx->jpeg_work, mtk_jpegdec_worker); INIT_LIST_HEAD(&ctx->dst_done_queue); v4l2_fh_init(&ctx->fh, vfd); @@ -1481,7 +1613,17 @@ static int mtk_jpeg_probe(struct platform_device *pdev) jpeg->workqueue = alloc_ordered_workqueue(MTK_JPEG_NAME, WQ_MEM_RECLAIM | WQ_FREEZABLE); if (!jpeg->workqueue) { - dev_err(&pdev->dev, "Failed to create jpeg workqueue!\n"); + dev_err(&pdev->dev, "Failed to create jpegenc workqueue!\n"); + ret = -EINVAL; + goto err_alloc_workqueue; + } + } else { + init_waitqueue_head(&jpeg->dec_hw_wq); + + jpeg->dec_workqueue = alloc_ordered_workqueue(MTK_JPEG_NAME, + WQ_MEM_RECLAIM | WQ_FREEZABLE); + if (!jpeg->dec_workqueue) { + dev_err(&pdev->dev, "Failed to create jpegdec workqueue!\n"); ret = -EINVAL; goto err_alloc_workqueue; } @@ -1571,6 +1713,8 @@ static int mtk_jpeg_remove(struct platform_device *pdev) mtk_jpeg_clk_release(jpeg); flush_workqueue(jpeg->workqueue); destroy_workqueue(jpeg->workqueue); + flush_workqueue(jpeg->dec_workqueue); + destroy_workqueue(jpeg->dec_workqueue); return 0; } diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h index b7102db..d67c49f 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h @@ -202,6 +202,9 @@ struct mtk_jpegdec_comp_dev { int jpegdec_irq; struct delayed_work job_timeout_work; struct mtk_jpeg_hw_param hw_param; + atomic_t hw_rdy; + enum mtk_jpeg_hw_state hw_state; + spinlock_t hw_lock; }; /** @@ -239,6 +242,8 @@ struct mtk_jpeg_dev { void __iomem *reg_decbase[MTK_JPEGDEC_HW_MAX]; struct mtk_jpegdec_comp_dev *dec_hw_dev[MTK_JPEGDEC_HW_MAX]; + wait_queue_head_t dec_hw_wq; + struct workqueue_struct *dec_workqueue; }; /** diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c index e295576..9138ecb 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c @@ -449,6 +449,7 @@ static void mtk_jpegdec_timeout_work(struct work_struct *work) struct mtk_jpegdec_comp_dev *cjpeg = container_of(work, struct mtk_jpegdec_comp_dev, job_timeout_work.work); + struct mtk_jpeg_dev *master_jpeg = cjpeg->master_dev; struct vb2_v4l2_buffer *src_buf, *dst_buf; src_buf = cjpeg->hw_param.src_buffer; @@ -457,6 +458,9 @@ static void mtk_jpegdec_timeout_work(struct work_struct *work) mtk_jpeg_dec_reset(cjpeg->reg_base); clk_disable_unprepare(cjpeg->pm.dec_clk.clk_info->jpegdec_clk); pm_runtime_put(cjpeg->pm.dev); + cjpeg->hw_state = MTK_JPEG_HW_IDLE; + atomic_inc(&cjpeg->hw_rdy); + wake_up(&master_jpeg->dec_hw_wq); v4l2_m2m_buf_done(src_buf, buf_state); v4l2_m2m_buf_done(dst_buf, buf_state); } @@ -557,8 +561,18 @@ static irqreturn_t mtk_jpegdec_hw_irq_handler(int irq, void *priv) v4l2_m2m_buf_done(src_buf, buf_state); v4l2_m2m_buf_done(dst_buf, buf_state); v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx); + clk_disable_unprepare(jpeg->pm.dec_clk.clk_info->jpegdec_clk); pm_runtime_put(ctx->jpeg->dev); + if (ctx->fh.m2m_ctx && + (!list_empty(&ctx->fh.m2m_ctx->out_q_ctx.rdy_queue) || + !list_empty(&ctx->fh.m2m_ctx->cap_q_ctx.rdy_queue))) + queue_work(master_jpeg->workqueue, &ctx->jpeg_work); + + jpeg->hw_state = MTK_JPEG_HW_IDLE; + wake_up(&master_jpeg->dec_hw_wq); + atomic_inc(&jpeg->hw_rdy); + return IRQ_HANDLED; } @@ -612,6 +626,9 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev) return -ENOMEM; dev->plat_dev = pdev; + atomic_set(&dev->hw_rdy, 1U); + spin_lock_init(&dev->hw_lock); + dev->hw_state = MTK_JPEG_HW_IDLE; INIT_DELAYED_WORK(&dev->job_timeout_work, mtk_jpegdec_timeout_work);
Add jpeg decoding worker to ensure that three HWs run in parallel in MT8195. Signed-off-by: kyrie.wu <kyrie.wu@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(-)