Re: [PATCH v7 09/12] media: iris: Add support to select core for dual core platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dynamic core assignment logic mutates `inst->core_id` during streaming without firmware notification and without holding `inst->lock`.
- [Medium] Initial core assignment bypasses MBPS constraints.
--

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

media: iris: Add support to select core for dual core platforms

On platforms with two video codec cores, select the hardware core for
a new session based on the current Macroblocks Per Frame (MBPF) and
Macroblocks Per Second (MBPS) load on each core. The selected core is
communicated to the firmware via the HFI_PROP_CORE_ID property at
stream-on time.

> diff --git a/drivers/media/platform/qcom/iris/iris_vb2.c b/drivers/media/platform/qcom/iris/iris_vb2.c
> index a2ea2d67f60d0..dbb89396e6514 100644
> --- a/drivers/media/platform/qcom/iris/iris_vb2.c
> +++ b/drivers/media/platform/qcom/iris/iris_vb2.c
> @@ -176,6 +176,10 @@ int iris_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
>  	if (ret)
>  		goto error;
>  
> +	ret = iris_set_core_id(inst);

[Severity: Medium]
Since this initial core assignment is driven by iris_check_session_supported()
called just before this, does that function need to check both MBPF and MBPS
limits?

Currently, iris_check_session_supported() only calls iris_check_core_mbpf() to
verify Macroblocks Per Frame limits. The complementary MBPS constraint check
via iris_check_core_mbps() appears to be entirely omitted.

Could this allow a session to be successfully assigned to a core that meets
the MBPF limit but vastly exceeds its maximum processing capability (MBPS)?

> +	if (ret)
> +		goto error;
> +
>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
>  		if (inst->domain == DECODER)
>  			ret = iris_vdec_streamon_input(inst);

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 65896d0c1f16b..9f8dacfa768d8 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> @@ -318,6 +318,67 @@ static void iris_vpu36_program_bootup_registers(struct iris_core *core)
>  	writel(0x0, core->reg_base + CPU_CS_SCIACMDARG3);
>  }
>  
> +static int iris_vpu36_check_core_load(struct iris_inst *inst, bool mbpf)
> +{
> +	const struct iris_platform_data *platform_data = inst->core->iris_platform_data;
> +	u32 max_load = mbpf ? platform_data->max_core_mbpf : platform_data->max_core_mbps;
> +	u32 max_session_cnt = platform_data->max_session_count;
> +	u32 core0_session_cnt = 0, core1_session_cnt = 0;
> +	u32 core0_load = 0, core1_load = 0;
> +	bool select_core0, select_core1;
> +	struct iris_inst *instance;
> +	u32 load, new_load;
> +
> +	inst->core_id = 0;

[Severity: High]
Is it safe to mutate inst->core_id here during active streaming without
holding inst->lock?

This function can be reached mid-stream if a concurrent s_parm call triggers
iris_venc_s_param() -> iris_check_core_mbpf(). Since iris_venc_s_param()
executes without holding inst->lock, a concurrent call can clear
inst->core_id to 0 right before iris_set_core_id() reads it during
iris_vb2_start_streaming().

Additionally, altering inst->core_id mid-stream without notifying the
firmware (which only receives HFI_PROP_CORE_ID at stream-on) permanently
desynchronizes the driver from the firmware.

This could cause iris_vpu36_get_required_freq() to incorrectly shift the
session's power votes to the new core while the firmware continues executing
on the old core, potentially leading to hardware underclocking and system hangs.

> +
> +	list_for_each_entry(instance, &inst->core->instances, list) {
> +		load = mbpf ? iris_get_mbpf(instance) : iris_get_mbps(instance);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-glymur-v7-0-afaa55d11fe0@oss.qualcomm.com?part=9