Re: [PATCH v6, 6/8] media: mediatek: vcodec: Refactor encoder clock on/off function

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Irui Wang <irui.wang@mediatek.com>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Tzung-Bi Shih <tzungbi@chromium.org>,
	angelogioacchino.delregno@collabora.com,
	nicolas.dufresne@collabora.com,
	Tiffany Lin <tiffany.lin@mediatek.com>,
	Yunfei Dong <yunfei.dong@mediatek.com>,
	Maoguang Meng <maoguang.meng@mediatek.com>,
	Longfei Wang <longfei.wang@mediatek.com>,
	Project_Global_Chrome_Upstream_Group@mediatek.com,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v6, 6/8] media: mediatek: vcodec: Refactor encoder clock on/off function
Date: Tue, 4 Oct 2022 13:21:52 +0300 (EEST)	[thread overview]
Message-ID: <3645b24f-436e-d6a5-27cd-287712eab22@linux.intel.com> (raw)
In-Reply-To: <20221001031737.18266-7-irui.wang@mediatek.com>

On Sat, 1 Oct 2022, Irui Wang wrote:

> when enable multi-core encoding, encoder cores use their own clock,
> refactor clock management functions with used encoder hardware id.
> 
> Signed-off-by: Irui Wang <irui.wang@mediatek.com>
> ---
>  .../mediatek/vcodec/mtk_vcodec_enc_pm.c       | 89 ++++++++++++++++---
>  .../mediatek/vcodec/mtk_vcodec_enc_pm.h       |  6 +-
>  .../platform/mediatek/vcodec/venc_drv_if.c    |  4 +-
>  3 files changed, 84 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_pm.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_pm.c
> index 213c3f50e9eb..2f83aade779a 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_pm.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_enc_pm.c
> @@ -60,7 +60,9 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_init_enc_clk);
>  static int mtk_enc_core_power_on(struct mtk_vcodec_ctx *ctx)
>  {
>  	struct mtk_venc_hw_dev *sub_core;
> +	struct mtk_vcodec_clk *clk;
>  	int ret, i;
> +	int j = 0;
>  
>  	/* multi-core encoding need power on all available cores */
>  	for (i = 0; i < MTK_VENC_HW_MAX; i++) {
> @@ -73,12 +75,27 @@ static int mtk_enc_core_power_on(struct mtk_vcodec_ctx *ctx)
>  			mtk_v4l2_err("power on sub_core[%d] fail %d", i, ret);
>  			goto pm_on_fail;
>  		}
> +
> +		clk = &sub_core->pm.venc_clk;
> +		for (j = 0; j < clk->clk_num; j++) {
> +			ret = clk_prepare(clk->clk_info[j].vcodec_clk);
> +			if (ret) {
> +				mtk_v4l2_err("prepare clk [%s] fail %d",
> +					     clk->clk_info[j].clk_name, ret);
> +				goto pm_on_fail;
> +			}
> +		}
>  	}
>  	return ret;
>  
>  pm_on_fail:
>  	for (i -= 1; i >= 0; i--) {
>  		sub_core = (struct mtk_venc_hw_dev *)ctx->dev->enc_hw_dev[i];
> +
> +		clk = &sub_core->pm.venc_clk;
> +		for (j -= 1; j >= 0; j--)
> +			clk_unprepare(clk->clk_info[j].vcodec_clk);
> +
>  		pm_runtime_put_sync(&sub_core->plat_dev->dev);

There's more than one thing wrong here.

pm_runtime_put_sync() won't be called for the ith entry when the later 
goto pm_on_fail is taken because the loop decrements i right at the 
start.

Similarly, i and j will mismatch for the ith entry because i was 
decremented.

Third, j does not start from clk->clk_num - 1 for the other entries 
(for those lower "i"s).

-- 
 i.