Re: [PATCH v9 10/14] 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 reassignment mid-session corrupts hardware state and bypasses capacity limits.
- [High] Misattribution of unassigned sessions incorrectly inflates Core 1 clock frequencies, leading to hardware underclocking.
--

--- Patch [10]: [PATCH v9 10/14] media: iris: Add support to select core for dual core platforms ---
commit 9f55933e77ce82b0c6d889a73f17c3f4e9da2a7a
Author: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
Subject: media: iris: Add support to select core for dual core platforms

This commit adds support for dual video codec cores, selecting a core based
on MBPF and MBPS load and scaling limits and clock frequencies accordingly.

> 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: High]
Does setting the core ID happen too late in the streaming initialization?

Looking at iris_vb2_start_streaming(), iris_scale_power() is called before
iris_check_session_supported() and iris_set_core_id():

    iris_scale_power(inst);
    ret = iris_check_session_supported(inst);
    ...
    ret = iris_set_core_id(inst);

This means frequency calculations will run while the new session's core_id 
is still unassigned (0), which appears to cause issues in the frequency 
calculation logic below.

> +	if (ret)
> +		goto error;
> +
>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_vpu3x.c b/drivers/media/platform/qcom/iris/iris_vpu3x.c
> index 7d5007abaded8..97563bf899392 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu3x.c

[ ... ]

> @@ -338,6 +338,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;
> +
> +	list_for_each_entry(instance, &inst->core->instances, list) {
> +		load = mbpf ? iris_get_mbpf(instance) : iris_get_mbps(instance);
> +
> +		if (instance->core_id == IRIS_VCODEC0) {
> +			core0_load += load;
> +			core0_session_cnt++;
> +		} else if (instance->core_id == IRIS_VCODEC1) {
> +			core1_load += load;
> +			core1_session_cnt++;
> +		}
> +	}
> +
> +	new_load = mbpf ? iris_get_mbpf(inst) : iris_get_mbps(inst);
> +
> +	select_core0 = core0_load + new_load <= max_load && core0_session_cnt < max_session_cnt;
> +	select_core1 = core1_load + new_load <= max_load && core1_session_cnt < max_session_cnt;
> +
> +	if (select_core0 && select_core1)
> +		inst->core_id = (core0_load <= core1_load) ? IRIS_VCODEC0 : IRIS_VCODEC1;
> +	else if (select_core0)
> +		inst->core_id = IRIS_VCODEC0;
> +	else if (select_core1)
> +		inst->core_id = IRIS_VCODEC1;
> +	else
> +		return -ENOMEM;
> +
> +	return 0;
> +}

[Severity: High]
Does unconditionally zeroing and reassigning core_id corrupt the session 
state if called mid-session?

If parameters are dynamically updated during streaming (for example, via 
iris_venc_s_parm()), iris_check_core_mbpf() and iris_check_core_mbps() are 
invoked sequentially. Would the MBPS check overwrite the assignment made by 
the MBPF check, ignoring whether the newly assigned core has sufficient 
MBPF capacity?

Additionally, since the hardware is only notified of the core ID during 
stream-on via HFI_PROP_CORE_ID, could dynamically changing inst->core_id 
here leave the driver calculating power for one core while the firmware 
continues executing the session on another?

> +
> +static u64 iris_vpu36_get_required_freq(struct iris_inst *inst)
> +{
> +	u64 vcodec0_freq = 0, vcodec1_freq = 0;
> +	struct iris_core *core = inst->core;
> +	struct iris_inst *instance;
> +
> +	list_for_each_entry(instance, &core->instances, list) {
> +		if (!instance->max_input_data_size)
> +			continue;
> +
> +		if (instance->core_id == IRIS_VCODEC0)
> +			vcodec0_freq += instance->power.min_freq;
> +		else
> +			vcodec1_freq += instance->power.min_freq;

[Severity: High]
Does this else block unintentionally include unassigned sessions?

As mentioned above, new sessions initially have an unassigned core_id (0)
when iris_scale_power() is called. Since 0 is not equal to IRIS_VCODEC0, 
won't this falsely add the unassigned session's required frequency to 
Core 1?

If a heavy session is misattributed to a lightly loaded Core 1 instead of 
a heavily loaded Core 0, could the resulting maximum frequency be 
artificially low and cause the hardware to underclock?

> +	}
> +
> +	return max(vcodec0_freq, vcodec1_freq);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-glymur-v9-0-8cf2cbe12a07@oss.qualcomm.com?part=10