Re: [PATCH v8 06/12] media: iris: Replace enum-indexed clock and power domain tables with per-block structures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bundling global OPP voltage votes into per-block power helpers causes fatal undervolting on multi-block platforms.
--

commit 86dd0baf96fce305fc0cfdbc52efa9bd09e3d865
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Replace enum-indexed clock and power domain tables with per-block structures

This commit replaces flat tables and enum-based lookups with per-block
iris_power_domain structures. Platform data now describes each hardware
block's clocks and power domains independently, and the enable/disable helpers
operate on these instances instead of looping through tables.

> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
> --- a/drivers/media/platform/qcom/iris/iris_resources.c
> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
> @@ -70,74 +69,61 @@ int iris_opp_set_rate(struct device *dev, unsigned long freq)
[ ... ]
> +int iris_enable_power_domain_and_clocks(struct iris_core *core, struct iris_power_domain *pd)
>  {
> -	int ret;
> +	int ret, i;
>  
>  	ret = iris_opp_set_rate(core->dev, ULONG_MAX);
>  	if (ret)
>  		return ret;
[ ... ]
> +	ret = clk_bulk_prepare_enable(pd->clk_cnt, pd->clocks);
>  	if (ret)
> -		return ret;
> -
> -	pm_runtime_put_sync(pd_dev);
> +		goto error;
>  
>  	return 0;
> -}
>  
> -static struct clk *iris_get_clk_by_type(struct iris_core *core, enum platform_clk_type clk_type)
> -{
> -	const struct platform_clk_data *clk_tbl;
> -	u32 clk_cnt, i, j;
> -
> -	clk_tbl = core->iris_platform_data->clk_tbl;
> -	clk_cnt = core->iris_platform_data->clk_tbl_size;
> -
> -	for (i = 0; i < clk_cnt; i++) {
> -		if (clk_tbl[i].clk_type == clk_type) {
> -			for (j = 0; core->clock_tbl && j < core->clk_count; j++) {
> -				if (!strcmp(core->clock_tbl[j].id, clk_tbl[i].clk_name))
> -					return core->clock_tbl[j].clk;
> -			}
> -		}
> -	}
> +error:
> +	iris_opp_set_rate(core->dev, 0);

[Severity: High]
Does dropping the global OPP vote to 0 here cause issues if a secondary block
fails to enable? Since the OPP vote is tied to core->dev and shared across all
hardware blocks, it appears this could drop the voltage while a previously
enabled primary block is still running, potentially starving it before it can
be cleanly disabled.

>  
> -	return NULL;
> +	while (--i >= 0)
> +		pm_runtime_put_sync(pd->dev[i]);
> +
> +	return ret;
>  }
>  
> -int iris_prepare_enable_clock(struct iris_core *core, enum platform_clk_type clk_type)
> +void iris_disable_power_domain_and_clocks(struct iris_core *core, struct iris_power_domain *pd)
>  {
> -	struct clk *clock;
> +	int i;
>  
> -	clock = iris_get_clk_by_type(core, clk_type);
> -	if (!clock)
> -		return -ENOENT;
> +	clk_bulk_disable_unprepare(pd->clk_cnt, pd->clocks);
> +	iris_opp_set_rate(core->dev, 0);

[Severity: High]
Could dropping the global voltage vote to 0 here upon disabling a single block
trigger immediate hardware lockups or SErrors? Other blocks might still be
active and running at high clock frequencies.

>  
> -	return clk_prepare_enable(clock);
> +	for (i = 0; i < pd->pd_cnt; i++)
> +		pm_runtime_put_sync(pd->dev[i]);
>  }
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu4x.c b/drivers/media/platform/qcom/iris/iris_vpu4x.c
> --- a/drivers/media/platform/qcom/iris/iris_vpu4x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu4x.c
> @@ -335,17 +240,13 @@ static void iris_vpu4x_power_off_hardware(struct iris_core *core)
>  	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
>  
>  disable_clocks_and_power:
> -	iris_vpu4x_disable_hardware_clocks(core, efuse_value);
> -
>  	if (!(efuse_value & DISABLE_VIDEO_VPP1_BIT))
> -		iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs
> -					   [IRIS_VPP1_HW_POWER_DOMAIN]);
> +		iris_disable_power_domain_and_clocks(core, core->vcodec_vpp1);

[Severity: High]
When disabling the first block (like vcodec_vpp1 here), the global OPP vote is
dropped to 0 due to the new per-block teardown sequence. Does this result in
undervolting for subsequent blocks (like VPP0 and VCODEC) that haven't been
disabled yet? The old architecture appeared to disable all clocks globally
across all blocks before dropping the OPP vote.

>  
>  	if (!(efuse_value & DISABLE_VIDEO_VPP0_BIT))
> -		iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs
> -					   [IRIS_VPP0_HW_POWER_DOMAIN]);
> +		iris_disable_power_domain_and_clocks(core, core->vcodec_vpp0);
>  
> -	iris_disable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_HW_POWER_DOMAIN]);
> +	iris_disable_power_domain_and_clocks(core, core->vcodec);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-glymur-v8-0-1c79b9d51fc0@oss.qualcomm.com?part=6