Re: [PATCH 4/9] media: mtk-vcodec: Add venc power on/off interface

[Tzung-Bi Shih <tzungbi@google.com>]

On Mon, Aug 16, 2021 at 06:59:29PM +0800, Irui Wang wrote:
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> @@ -16,6 +16,7 @@
>  #include "mtk_vcodec_intr.h"
>  #include "mtk_vcodec_util.h"
>  #include "venc_drv_if.h"
> +#include "mtk_vcodec_enc_pm.h"
Please try to maintain the order.

> @@ -285,11 +291,12 @@ static int fops_vcodec_release(struct file *file)
>  	mtk_v4l2_debug(1, "[%d] encoder", ctx->id);
>  	mutex_lock(&dev->dev_mutex);
>  
> +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
>  	mtk_vcodec_enc_release(ctx);
>  	v4l2_fh_del(&ctx->fh);
>  	v4l2_fh_exit(&ctx->fh);
>  	v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
> -	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> +	mtk_vcodec_enc_power_off(ctx);
Any reason to move the v4l2_m2m_ctx_release()?

> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> @@ -12,6 +12,7 @@
>  
>  #include "mtk_vcodec_enc_pm.h"
>  #include "mtk_vcodec_util.h"
> +#include "mtk_vcodec_enc_hw.h"
Please try to maintain the order.

> +int mtk_venc_enable_comp_hw(struct mtk_vcodec_dev *dev)
> +{
> +	int i, ret;
> +	struct mtk_venc_comp_dev *venc_comp;
> +
> +	/*
> +	 * frame_racing mode needs power on all available component devices.
> +	 */
> +	for (i = 0; i < MTK_VENC_HW_MAX; i++) {
> +		venc_comp = (struct mtk_venc_comp_dev *)dev->enc_comp_dev[i];
> +		if (!venc_comp)
> +			return 0;
> +
> +		ret = mtk_smi_larb_get(venc_comp->pm.larbvenc);
> +		if (ret < 0) {
> +			mtk_v4l2_err("power on core[%d] fail %d", i, ret);
> +				goto pw_err;
The goto statement has extra indent layer.

> +int mtk_venc_disable_comp_hw(struct mtk_vcodec_dev *dev)
> +{
> +	int i;
> +	struct mtk_venc_comp_dev *venc_comp;
> +
> +	/*power off all available component device*/
Need extra space between /* */.

> +int mtk_vcodec_enc_power_on(struct mtk_vcodec_ctx *ctx)
> +{
> +	int ret;
> +	struct mtk_vcodec_dev *dev = ctx->dev;
> +
> +	if (dev->venc_pdata->hw_mode == VENC_FRAME_RACING_MODE) {
> +		ret = mtk_venc_enable_comp_hw(dev);
> +		if (ret < 0) {
> +			mtk_v4l2_err("enable venc comp hw fail :%d", ret);
> +			return ret;
> +		}
> +	} else {
> +		ret = mtk_smi_larb_get(dev->pm.larbvenc);
> +		if (ret < 0) {
> +			mtk_v4l2_err("pm_runtime_resume_and_get fail %d", ret);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
It seems the function doesn't need struct mtk_vcodec_ctx.

Does mtk_vcodec_enc_power_on(struct mtk_vcodec_dev* ) make more sense?

> +int mtk_vcodec_enc_power_off(struct mtk_vcodec_ctx *ctx)
> +{
> +	struct mtk_vcodec_dev *dev = ctx->dev;
> +
> +	if (dev->venc_pdata->hw_mode == VENC_FRAME_RACING_MODE)
> +		mtk_venc_disable_comp_hw(dev);
> +	else
> +		mtk_smi_larb_put(dev->pm.larbvenc);
> +
> +	return 0;
> +}
It seems the function doesn't need struct mtk_vcodec_ctx.