Re: [PATCH v6 09/18] media: iris: Add platform flag for instantaneous bandwidth voting

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] State desynchronization on interconnect bandwidth update failure, leading to permanent bandwidth voting bypass.
- [High] The interconnect bandwidth vote for the 'cpu-cfg' path is permanently dropped to 0 after the first suspend cycle.
--

--- Patch [9]: [PATCH v6 09/18] media: iris: Add platform flag for instantaneous bandwidth voting ---
commit 626a7011b6fd7b2bb9e6fe29e1b8acf22f4ae173
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Add platform flag for instantaneous bandwidth voting
    
AR50LT require explicit instantaneous bandwidth (IB) voting in addition
to average bandwidth (AB) when configuring interconnect QoS. This
requirement is due to QSB (Qualcomm System Bus) 128b to
QNS ( Qualcomm Network Switch) 256b conversion at video noc in AR50LT
which is not needed for other IRIS cores.

In preparation of adding support for AR50LT core, introduce
platform-configurable IB multiplier and enable IB voting for all SoCs.
Existing platforms default to IB == AB, while AR50LT requires 2x peak
bandwidth.

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
> index 773f6548370a2..caeaf199cef74 100644
> --- a/drivers/media/platform/qcom/iris/iris_resources.c
> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
> @@ -18,6 +18,7 @@
>  
>  int iris_set_icc_bw(struct iris_core *core, unsigned long icc_bw)
>  {
> +	u32 icc_ib_multiplier = core->iris_platform_data->icc_ib_multiplier;

[Severity: High]
This isn't a bug introduced by this patch, but does this function
permanently drop the interconnect bandwidth vote for the 'cpu-cfg' path
after the first suspend cycle?

During initialization, 'cpu-cfg' is set to bw_min_kbps. On suspend,
iris_unset_icc_bw() sets all interconnect paths (including 'cpu-cfg') to 0.

However, upon resume, iris_set_icc_bw() iterates through the paths but
only updates 'video-mem':

	for (i = 0; i < core->icc_count; i++) {
		if (!strcmp(core->icc_tbl[i].name, "video-mem")) {
			bw_kbps = icc_bw;
			...

When icc_bulk_set_bw() is called at the end of this function, 'cpu-cfg'
remains at 0. This could lead to a fatal system crash on Qualcomm SoCs
when register access to the VPU is attempted over the AHB bus without
this bandwidth.

>  	unsigned long bw_kbps = 0, bw_prev = 0;
>  	const struct icc_info *icc_tbl;
>  	int ret = 0, i;
> @@ -36,6 +37,7 @@ int iris_set_icc_bw(struct iris_core *core, unsigned long icc_bw)
>  				return ret;
>  
>  			core->icc_tbl[i].avg_bw = bw_kbps;
> +			core->icc_tbl[i].peak_bw = bw_kbps * icc_ib_multiplier;
>  
>  			core->power.icc_bw = bw_kbps;
>  			break;

[Severity: High]
This is a pre-existing issue, but can updating core->power.icc_bw here
lead to state desynchronization if the actual hardware vote fails?

This updates the internal state before ensuring the vote succeeds:

			core->power.icc_bw = bw_kbps;
			break;
		}
	}

	return icc_bulk_set_bw(core->icc_count, core->icc_tbl);

If icc_bulk_set_bw() returns an error, the operation is aborted but the
software state remains updated. On the next call to iris_set_icc_bw()
with a similar bandwidth, the check abs(bw_kbps - bw_prev) < BW_THRESHOLD
will succeed. This causes it to return early and bypass the interconnect
update, leaving the hardware operating with insufficient or no interconnect
bandwidth.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-iris-ar50lt-v6-0-374f0a46c23b@oss.qualcomm.com?part=9