Re: [PATCH v5 09/16] media: iris: Add framework support for AR50_LITE video core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] pm_runtime_get_sync() leaks usage count on failure in iris_enable_power_domains().
- [High] iris_disable_power_domains() bails early on OPP failure, bypassing pm_runtime_put_sync() and leaking the PM reference.
- [High] 32-bit multiplication in VPU frequency calculations silently overflows, leading to drastic underclocking at high resolutions/framerates.
- [High] The function `iris_vpu_boot_firmware()` returns success (0) even when it detects a firmware boot error.
--

commit 7269410ec2faaa10691a53dc0ab6c6efcc23fc3e
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Add framework support for AR50_LITE video core

Add power sequence for ar5lt core. Add register handling for ar50lt by
hooking up vpu op with ar50lt specific implemtation or resue from earlier
generation wherever feasible.

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c
> new file mode 100644
> index 0000000000000..e084a5b49f2e3
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c

[ ... ]

> +static int iris_vpu_ar50lt_power_on_controller(struct iris_core *core)
> +{
> +	int ret;
> +
> +	ret = iris_enable_power_domains(core, core->pmdomain_tbl->pd_devs[IRIS_CTRL_POWER_DOMAIN]);

[Severity: High]
This isn't a bug introduced by this patch, but do iris_enable_power_domains()
and iris_disable_power_domains() leak runtime PM references on failure?

In drivers/media/platform/qcom/iris/iris_resources.c,
iris_enable_power_domains() calls:

	ret = pm_runtime_get_sync(pd_dev);
	if (ret < 0)
		return ret;

If the resume fails, it returns a negative error code but leaves the usage
count incremented without calling pm_runtime_put_noidle().

Similarly, iris_disable_power_domains() has an early return that bypasses
pm_runtime_put_sync():

	ret = iris_opp_set_rate(core->dev, 0);
	if (ret)
		return ret;
	
	pm_runtime_put_sync(pd_dev);

If iris_opp_set_rate() fails, the pm_runtime_put_sync() call is skipped,
permanently leaking the runtime PM reference and leaving the power domain
active.

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 41498f94480e8..d64e7745a63dc 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -97,7 +97,8 @@ int iris_vpu_boot_firmware(struct iris_core *core)
>  	}
>  
>  	writel(HOST2XTENSA_INTR_ENABLE, core->reg_base + CPU_CS_H2XSOFTINTEN);
> -	writel(0x0, core->reg_base + CPU_CS_X2RPMH);
> +	if (!core->iris_platform_data->no_rpmh)
> +		writel(0x0, core->reg_base + CPU_CS_X2RPMH);
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does iris_vpu_boot_firmware()
return success even when it detects a firmware boot error?

Earlier in this function:

	if ((ctrl_status & CTRL_ERROR_STATUS__M) == 0x4) {
		dev_err(core->dev, "invalid setting for uc_region\n");
		break;
	}

Because the loop is exited early via break, the count does not reach
max_tries. This skips the subsequent timeout check (if (count >= max_tries)),
and the function incorrectly falls through to return 0.

Could this cause the caller to proceed assuming the firmware booted correctly,
leading to undefined behavior and hardware timeouts later?

[ ... ]

> @@ -422,6 +423,32 @@ void iris_vpu35_vpu4x_program_bootup_registers(struct iris_core *core)
>  	writel(0x1, core->reg_base + WRAPPER_IRIS_VCODEC_VPU_WRAPPER_SPARE_0);
>  }
>  
> +u64 iris_vpu2_calculate_frequency(struct iris_inst *inst, size_t data_size)
> +{
> +	struct platform_inst_caps *caps = inst->core->iris_platform_data->inst_caps;
> +	struct v4l2_format *inp_f = inst->fmt_src;
> +	u32 mbs_per_second, mbpf, height, width;
> +	unsigned long vpp_freq, vsp_freq;
> +	u32 fps = inst->frame_rate;
> +
> +	width = max(inp_f->fmt.pix_mp.width, inst->crop.width);
> +	height = max(inp_f->fmt.pix_mp.height, inst->crop.height);
> +
> +	mbpf = NUM_MBS_PER_FRAME(height, width);
> +	mbs_per_second = mbpf * fps;
> +
> +	vpp_freq = mbs_per_second * caps->mb_cycles_vpp;

[Severity: High]
This is a pre-existing issue, but does this 32-bit multiplication silently
overflow before assignment?

Both mbs_per_second and caps->mb_cycles_vpp are u32. For high resolutions
and framerates (like 8K at 240+ FPS), mbs_per_second can reach ~33M and
mb_cycles_vpp is ~200. This results in a product greater than 6.6 billion,
exceeding the u32 maximum.

The value wraps around before being cast to unsigned long, which might cause
the requested clock frequency to be set drastically lower than required.

The same multiplication overflow also exists in
iris_vpu3x_vpu4x_calculate_frequency() where mult_frac() internally executes a
u32 * u32 operation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=9