Re: [PATCH v5 10/14] media: iris: Add power sequence for Glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Improper hardware teardown sequence violates LIFO ordering.
- [High] Hardware state leak on NOC LPI polling timeout error path.
- [High] Missing back-off and retry loop for VCODEC1 NOC LPI handshake.
- [High] Missing `.program_bootup_registers` callback in `iris_vpu36_ops`.
--

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

media: iris: Add power sequence for Glymur

Glymur has a secondary video codec core (vcodec1), equivalent to the
primary core (vcodec0), but with independent power domains, clocks,
and reset lines. Reuse the existing code wherever possible and add
power sequence for vcodec1.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 13fbb21c21824..ff90c375e805c 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c

[ ... ]

> @@ -254,6 +264,124 @@ static void iris_vpu35_power_off_hw(struct iris_core *core)

[ ... ]

> +static void iris_vpu36_power_off_hw1(struct iris_core *core)
> +{
> +	u32 value, i;
> +	int ret;
> +
> +	if (iris_vpu36_hw1_power_collapsed(core))
> +		goto disable_power;
> +
> +	value = readl(core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +	if (value)
> +		writel(CORE_CLK_RUN, core->reg_base + WRAPPER_CORE_CLOCK_CONFIG);
> +
> +	for (i = 0; i < core->iris_platform_data->num_vpp_pipe; i++) {
> +		ret = readl_poll_timeout(core->reg_base + VCODEC1_SS_IDLE_STATUSN + 4 * i,
> +					 value, value & DMA_NOC_IDLE, 2000, 20000);
> +		if (ret)
> +			goto disable_power;
> +	}
> +
> +	writel(REQ_VCODEC1_POWER_DOWN_PREP, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +	ret = readl_poll_timeout(core->reg_base + AON_WRAPPER_MVP_NOC_LPI_STATUS,
> +				 value, value & NOC_LPI_VCODEC1_STATUS_DONE, 2000, 20000);
> +	if (ret)
> +		goto disable_power;

Should this implement a back-off and retry loop? A single poll without clearing
the request bit and retrying might timeout when the NOC is busy, causing an
unclean hardware shutdown.

Also, if the readl_poll_timeout() fails and takes the goto, the code skips
clearing the request bit in AON_WRAPPER_MVP_NOC_LPI_CONTROL. Because this
register resides in the Always-On wrapper, will the power-down request bit
remain asserted across power cycles and prevent the hardware from properly
resuming on the next boot?

> +
> +	writel(0, core->reg_base + AON_WRAPPER_MVP_NOC_LPI_CONTROL);
> +
> +	writel(VCODEC1_BRIDGE_SW_RESET | VCODEC1_BRIDGE_HW_RESET_DISABLE, core->reg_base +
> +	       CPU_CS_AHB_BRIDGE_SYNC_RESET);
> +	writel(VCODEC1_BRIDGE_HW_RESET_DISABLE, 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, IRIS_VCODEC1_POWER_DOMAIN, false);
> +	iris_disable_unprepare_clock(core, IRIS_VCODEC1_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_VCODEC1_FREERUN_CLK);
> +	iris_disable_unprepare_clock(core, IRIS_AXI_VCODEC1_CLK);
> +	iris_disable_power_domains(core, IRIS_VCODEC1_POWER_DOMAIN);
> +}
> +
> +static void iris_vpu36_power_off_hw(struct iris_core *core)
> +{
> +	iris_vpu35_power_off_hw(core);
> +	iris_vpu36_power_off_hw1(core);
> +}

Does this hardware teardown sequence violate LIFO ordering? The power-on
sequence initializes the primary codec before the secondary codec, so tearing
down the primary codec first in iris_vpu35_power_off_hw() triggers a full
video NOC reset and disables the shared primary clock domains.

Consequently, when vcodec1's teardown sequence executes, will its attempts to
read registers trigger bus hangs or timeouts?

[ ... ]

> @@ -281,3 +409,12 @@ const struct vpu_ops iris_vpu35_ops = {
>  	.calc_freq = iris_vpu3x_vpu4x_calculate_frequency,
>  	.set_hwmode = iris_vpu_set_hwmode,
>  };
> +
> +const struct vpu_ops iris_vpu36_ops = {
> +	.power_off_hw = iris_vpu36_power_off_hw,
> +	.power_on_hw = iris_vpu36_power_on_hw,
> +	.power_off_controller = iris_vpu35_vpu4x_power_off_controller,
> +	.power_on_controller = iris_vpu35_vpu4x_power_on_controller,
> +	.calc_freq = iris_vpu3x_vpu4x_calculate_frequency,
> +	.set_hwmode = iris_vpu36_set_hwmode,
> +};

Is the .program_bootup_registers callback intentionally omitted here?
It is present in the closely related iris_vpu35_ops struct. Without it,
the core framework function might skip writing to
WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0, which could leave the hardware
improperly configured.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509-glymur-v5-0-7fbb340c5dbd@oss.qualcomm.com?part=10