Re: [PATCH v10 08/14] media: iris: Add power sequence for glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing required NOC LPI DONE polling before hardware reset in iris_vpu36_power_off_vcodec causes a race condition.
- [High] Iteratively dropping the shared device OPP rate to 0 before all cores are powered off will cause a synchronous external abort.
--

--- Patch [8]: [PATCH v10 08/14] media: iris: Add power sequence for glymur ---
commit d2cd6a91322f190f5a10ce98b74613cfdf850a61
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>

media: iris: Add power sequence for glymur

This commit adds power sequencing for the glymur platform's video codec. It
introduces support for handling multiple codec cores (vcodec0 and vcodec1) that
share a common clock source and power rails.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 7cda946015558..1b08461ee4d70 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
[ ... ]
> @@ -221,6 +221,113 @@ static void iris_vpu35_power_off_hw(struct iris_core *core)
[ ... ]
> +static void iris_vpu36_power_off_vcodec(struct iris_core *core, u32 core_id)
> +{
[ ... ]
> +	do {
> +		writel(power_down_prep[core_id], core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(15, 20);
> +		value = readl(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS);
> +
> +		handshake_done = value & lpi_status_done[core_id];
> +		handshake_busy = value & (lpi_status_deny[core_id] | lpi_status_active[core_id]);
> +
> +		if (handshake_done || !handshake_busy)
> +			break;

[Severity: High]
Can this loop exit prematurely if the NOC hasn't yet transitioned to busy?

If `handshake_busy` is initially 0 after writing to `power_down_prep`, the loop
will break before the NOC LPI DONE status is asserted.

> +
> +		writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +		usleep_range(15, 20);
> +	} while (++count < 1000);
> +
> +	if (!handshake_done && handshake_busy)
> +		goto disable_power;
> +
> +	writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +
> +	writel(bridge_sw_reset[core_id] | bridge_hw_reset[core_id],
> +	       core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);

[Severity: High]
Without an additional poll for the DONE status (as seen in
`iris_vpu33_power_off_hardware`), couldn't asserting a bridge reset here while
the NOC is still active or transitioning cause a hardware bus hang?

> +	writel(bridge_hw_reset[core_id], core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(0x0, core->reg_base + CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +
> +disable_power:
> +	iris_genpd_set_hwmode(&core->vcodec[core_id], false);
> +	iris_disable_power_domain_and_clocks(core, &core->vcodec[core_id]);
> +}
> +
> +static void iris_vpu36_power_off_hw(struct iris_core *core)
> +{
> +	u32 num_cores = core->iris_platform_data->num_cores;
> +	int i;
> +
> +	for (i = 0; i < num_cores; i++)
> +		iris_vpu36_power_off_vcodec(core, i);

[Severity: High]
Could this loop cause a synchronous external abort when shutting down multiple
cores?

When `iris_vpu36_power_off_vcodec()` powers off the first core, it reaches
the `disable_power` label and calls `iris_disable_power_domain_and_clocks()`,
which executes:

    iris_opp_set_rate(core->dev, 0);

When the loop then iterates to the next core, `iris_vpu36_power_off_vcodec()`
will read hardware registers such as `WRAPPER_CORE_POWER_STATUS` and
`WRAPPER_CORE_CLOCK_CONFIG`. Is it safe to read these registers when the
shared device voltage vote (OPP rate) has already been dropped to 0?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260726-glymur-v10-0-de451559e88b@oss.qualcomm.com?part=8