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

On 7/26/2026 11:35 AM, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

Ack.

>> +	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?

iris_opp_set_rate(core->dev, 0) sets the rate to the minimum OPP defined
in the DT — it does not disable the voltage. The actual voltage rail stays
up until all vcodec and ctrl power domains are put_sync'd, so register
accesses in subsequent loop iterations remain safe.

>> +}