Dear Angelo,
Thanks for your reviewing
On Mon, 2023-09-25 at 11:13 +0200, AngeloGioacchino Del Regno wrote:
> Il 25/09/23 06:02, Irui Wang ha scritto:
> > Remove PM functions at start/stop streaming, add PM helper
> > functions
> > to get PM before encoding frame start and put PM after encoding
> > frame
> > done. Meanwhile, remove unnecessary clock operations.
> >
> > Signed-off-by: Irui Wang <irui.wang@xxxxxxxxxxxx>
> > ---
> > .../mediatek/vcodec/encoder/mtk_vcodec_enc.c | 21 +++-----------
> > -----
> > .../vcodec/encoder/mtk_vcodec_enc_pm.c | 18
> > ++++++++++++++++
> > .../vcodec/encoder/mtk_vcodec_enc_pm.h | 3 ++-
> > .../mediatek/vcodec/encoder/venc_drv_if.c | 8 ++-----
> > 4 files changed, 25 insertions(+), 25 deletions(-)
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > index 04948d3eb011..eb381fa6e7d1 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc.c
> > @@ -866,7 +866,7 @@ static int vb2ops_venc_start_streaming(struct
> > vb2_queue *q, unsigned int count)
> > {
> > struct mtk_vcodec_enc_ctx *ctx = vb2_get_drv_priv(q);
> > struct venc_enc_param param;
> > - int ret, pm_ret;
> > + int ret;
> > int i;
> >
> > /* Once state turn into MTK_STATE_ABORT, we need stop_streaming
> > @@ -886,18 +886,12 @@ static int vb2ops_venc_start_streaming(struct
> > vb2_queue *q, unsigned int count)
> > return 0;
> > }
> >
> > - ret = pm_runtime_resume_and_get(&ctx->dev->plat_dev->dev);
> > - if (ret < 0) {
> > - mtk_v4l2_venc_err(ctx, "pm_runtime_resume_and_get fail
> > %d", ret);
> > - goto err_start_stream;
> > - }
> > -
> > mtk_venc_set_param(ctx, ¶m);
> > ret = venc_if_set_param(ctx, VENC_SET_PARAM_ENC, ¶m);
> > if (ret) {
> > mtk_v4l2_venc_err(ctx, "venc_if_set_param failed=%d",
> > ret);
> > ctx->state = MTK_STATE_ABORT;
> > - goto err_set_param;
> > + goto err_start_stream;
> > }
> > ctx->param_change = MTK_ENCODE_PARAM_NONE;
> >
> > @@ -910,18 +904,13 @@ static int vb2ops_venc_start_streaming(struct
> > vb2_queue *q, unsigned int count)
> > if (ret) {
> > mtk_v4l2_venc_err(ctx, "venc_if_set_param
> > failed=%d", ret);
> > ctx->state = MTK_STATE_ABORT;
> > - goto err_set_param;
> > + goto err_start_stream;
> > }
> > ctx->state = MTK_STATE_HEADER;
> > }
> >
> > return 0;
> >
> > -err_set_param:
> > - pm_ret = pm_runtime_put(&ctx->dev->plat_dev->dev);
> > - if (pm_ret < 0)
> > - mtk_v4l2_venc_err(ctx, "pm_runtime_put fail %d",
> > pm_ret);
> > -
> > err_start_stream:
> > for (i = 0; i < q->num_buffers; ++i) {
> > struct vb2_buffer *buf = vb2_get_buffer(q, i);
> > @@ -1004,10 +993,6 @@ static void vb2ops_venc_stop_streaming(struct
> > vb2_queue *q)
> > if (ret)
> > mtk_v4l2_venc_err(ctx, "venc_if_deinit failed=%d",
> > ret);
> >
> > - ret = pm_runtime_put(&ctx->dev->plat_dev->dev);
> > - if (ret < 0)
> > - mtk_v4l2_venc_err(ctx, "pm_runtime_put fail %d", ret);
> > -
> > ctx->state = MTK_STATE_FREE;
> > }
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > c
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > c
> > index 3fce936e61b9..a22b7dfc656e 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > c
> > @@ -58,6 +58,24 @@ int mtk_vcodec_init_enc_clk(struct
> > mtk_vcodec_enc_dev *mtkdev)
> > return 0;
> > }
> >
> > +void mtk_vcodec_enc_pw_on(struct mtk_vcodec_pm *pm)
> > +{
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(pm->dev);
> > + if (ret)
> > + dev_err(pm->dev, "pm_runtime_resume_and_get fail: %d",
> > ret);
> > +}
>
> Those are exactly the same functions as the DECODER counterpart...
> instead
> of duplicating them, can you please simply commonize the functions in
> mtk_vcodec_dec_pm.c ?
>
> Regards,
> Angelo
Yes, they are the same functions as the DECODER, we think ENCODER and
DECODER have their owned pm_runtime helper functions for a better
readability and future flexibility. And yunfei has helped to separate
decoder and encoder, so we prefer to separate the power management, it
might be more easier to manage later.
Thank you very much
Best Regards
>
> > +
> > +void mtk_vcodec_enc_pw_off(struct mtk_vcodec_pm *pm)
> > +{
> > + int ret;
> > +
> > + ret = pm_runtime_put(pm->dev);
> > + if (ret && ret != -EAGAIN)
> > + dev_err(pm->dev, "pm_runtime_put fail %d", ret);
> > +}
> > +
> > void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm)
> > {
> > struct mtk_vcodec_clk *enc_clk = &pm->venc_clk;
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > h
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > h
> > index e50be0575190..157ea08ba9e3 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > h
> > +++
> > b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_pm.
> > h
> > @@ -10,7 +10,8 @@
> > #include "mtk_vcodec_enc_drv.h"
> >
> > int mtk_vcodec_init_enc_clk(struct mtk_vcodec_enc_dev *dev);
> > -
> > +void mtk_vcodec_enc_pw_on(struct mtk_vcodec_pm *pm);
> > +void mtk_vcodec_enc_pw_off(struct mtk_vcodec_pm *pm);
> > void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm);
> > void mtk_vcodec_enc_clock_off(struct mtk_vcodec_pm *pm);
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/encoder/venc_drv_if.c
> > b/drivers/media/platform/mediatek/vcodec/encoder/venc_drv_if.c
> > index 1bdaecdd64a7..c402a686f3cb 100644
> > --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_drv_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_drv_if.c
> > @@ -32,9 +32,7 @@ int venc_if_init(struct mtk_vcodec_enc_ctx *ctx,
> > unsigned int fourcc)
> > }
> >
> > mtk_venc_lock(ctx);
> > - mtk_vcodec_enc_clock_on(&ctx->dev->pm);
> > ret = ctx->enc_if->init(ctx);
> > - mtk_vcodec_enc_clock_off(&ctx->dev->pm);
> > mtk_venc_unlock(ctx);
> >
> > return ret;
> > @@ -46,9 +44,7 @@ int venc_if_set_param(struct mtk_vcodec_enc_ctx
> > *ctx,
> > int ret = 0;
> >
> > mtk_venc_lock(ctx);
> > - mtk_vcodec_enc_clock_on(&ctx->dev->pm);
> > ret = ctx->enc_if->set_param(ctx->drv_handle, type, in);
> > - mtk_vcodec_enc_clock_off(&ctx->dev->pm);
> > mtk_venc_unlock(ctx);
> >
> > return ret;
> > @@ -68,10 +64,12 @@ int venc_if_encode(struct mtk_vcodec_enc_ctx
> > *ctx,
> > ctx->dev->curr_ctx = ctx;
> > spin_unlock_irqrestore(&ctx->dev->irqlock, flags);
> >
> > + mtk_vcodec_enc_pw_on(&ctx->dev->pm);
> > mtk_vcodec_enc_clock_on(&ctx->dev->pm);
> > ret = ctx->enc_if->encode(ctx->drv_handle, opt, frm_buf,
> > bs_buf, result);
> > mtk_vcodec_enc_clock_off(&ctx->dev->pm);
> > + mtk_vcodec_enc_pw_off(&ctx->dev->pm);
> >
> > spin_lock_irqsave(&ctx->dev->irqlock, flags);
> > ctx->dev->curr_ctx = NULL;
> > @@ -89,9 +87,7 @@ int venc_if_deinit(struct mtk_vcodec_enc_ctx
> > *ctx)
> > return 0;
> >
> > mtk_venc_lock(ctx);
> > - mtk_vcodec_enc_clock_on(&ctx->dev->pm);
> > ret = ctx->enc_if->deinit(ctx->drv_handle);
> > - mtk_vcodec_enc_clock_off(&ctx->dev->pm);
> > mtk_venc_unlock(ctx);
> >
> > ctx->drv_handle = NULL;
>
>
