lib: MCPS budget manager by kfrydryx · Pull Request #6649 · thesofproject/sof

kfrydryx · 2022-11-22T16:21:51Z

Add initial implementation of mcps budget management.

Ideally it should be used to control cpu clock, so that it runs on the lowest frequency suitable for performed tasks.
Different parts of FW can request/free some budget using core_cps_adjust and clock should be changed accordingly. (i.e. when setting pipeline to run we should request budget required by its modules).

Currently there is no proper performance data to be used, so it's advised to request some artificial, high budget on platform init to keep the clock running fast and avoid performance issues.

Requires zephyrproject-rtos/zephyr#52205 to work on mtl

abonislawski

Why there are no users of this manager? Even not supporting IPC to set CoreCpsAdjust from IPC data?

abonislawski · 2022-11-23T10:04:48Z

CONFIG_ZEPHYR_NATIVE_DRIVERS not needed in this file

lgirdwood · 2022-11-23T11:33:25Z

This is not recommended on multicore as the array[CORE_COUNT] can easily be clobbered. This structure should either use coherent API or be uncache only (with the correct SPARSE types).

lgirdwood · 2022-11-23T11:35:48Z

Needs to be a for loop with an array size end condition.

lgirdwood · 2022-11-23T11:38:52Z

Given my previous statement, its worth saying in a comment that this should be in uncache (which it should be on Zephyr) so no one is tempted to move it to cache.

is a comment there sufficient or do we have sth else (sparse?) for uncache specifically?

@lyakh can sparse be used to say static variables are uncache atm ?

@lgirdwood don't think so. An address space has to be specified explicitly for sparse to check it, all objects, that don't specify one explicitly are assumed to belong to the default address space - regardless whether static or automatic.

lgirdwood · 2022-11-23T11:39:45Z

lgirdwood · 2022-11-23T11:40:13Z

would be nice to have something in the logs here.

abonislawski

Please add basic usage for this driver/manager in base_fw (set&get kcps data)

btian1 · 2022-11-25T02:17:05Z

Add initial implementation of mcps budget management.

Ideally it should be used to control cpu clock, so that it runs on the lowest frequency suitable for performed tasks. Different parts of FW can request/free some budget using core_cps_adjust and clock should be changed accordingly. (i.e. when setting pipeline to run we should request budget required by its modules).

Currently there is no proper performance data to be used, so it's advised to request some artificial, high budget on platform init to keep the clock running fast and avoid performance issues.

Requires zephyrproject-rtos/zephyr#52205 to work on mtl

For clock adjustment, discuss with single core first, if I remember correctly, ADSP have 38.4/100/400 Mhz to choose, 38.4 is not possible for now, since a single playback is 40-50 mcps, and peak may go to 60, 38.4 may be possible for capture, I did not test it, later CI will have data on capture.

So our strategy may be simple: whether need set clock to 100Mhz, a simple way is depend on pipeline and component num, we do have those data, please check implementation from app/perf_overlay.conf, later CI will have a report for different case performance data.

btian1 · 2022-11-25T02:20:04Z

What's the full name for cps and kcps? anything special inside?

cycles per second and kilo cycles per second. cps = 1000 * kcps. Nothing special inside, though I did unify the naming to use kcps where possible.

btian1

whether need a extra function for estimate current workload performance based on pipeline num and component name/num? then consolidate these data and set freq accordingy?

kfrydryx · 2022-11-28T15:58:34Z

@btian1 The performance numbers can come in multiple forms. We definitely need some base usage for basefw. For audio modules their usage should come in ipc4 message (the values there are unmeasured yet afaik). Maybe some other parts of fw will have to request budget for themselves. That'll come with future PRs.

lgirdwood · 2022-12-02T13:23:27Z

@kfrydryx btw, is there any CI test that uses code in this PR or is this currently locally tested ? i.e will this code be invoked today if CI runs a low MCPS pipeline ?

pblaszko · 2022-12-02T15:12:21Z

 		return ipc4_logging_enable_logs(first_block, last_block, data_offset, data);
+	case IPC4_REGISTER_KCPS:
+		return basefw_register_kcps(first_block, last_block, data_offset, data);
+	case IPC4_RESOURCE_ALLOCATION_REQUEST:


@kfrydryx did you try to clarify with ACE FW/Windows SW why do we have two IPC's doing actually the same?

I did not. register_kcps is not used in internal testing, though the call is implemented. Both are handled by closed source fw.

is 1 mcps == 1000 kcps？

yes, exactly. Just like in metric system. 1 MCPS = 1000 KCPS = 1000 000 CPS.

kfrydryx · 2022-12-02T15:59:10Z

@lgirdwood This code is not used yet anywhere else, other than in those IPC requests. There are tests in internal CI calling them (though they are not made exclusively to test this functionality).

lgirdwood

@kfrydryx I assume this should be our single way/API to control CPU frequency/source (when it's enabled to avoid collisions with the manual API) ? If so, I would recommend

We check current CPU frequency/source is correct before we set new frequency/source.
I would call it cpu-clk-manager.c/.h, the word "budget" suggest scheduling and this PR is about clock frequency/source.

lgirdwood · 2022-12-05T12:14:08Z

can we prefix to avoid any naming collisions/confusion ?

Sure
Also:
File name changed.
I was thinking about having a pause/resume mechanism for overriding the managers clock choice, but according to @mmaka1 it is currently sufficient. This can be easily worked around by requesting artificial kcps budget. We were also considering an approach of holding a reference of who requested the clock and decide on the change on clock driver level, but left that topic for later.

lgirdwood · 2022-12-05T12:17:14Z

We need to add doxygen for these new APIs.

mmaka1 · 2022-12-07T12:22:00Z

Pls update the pre-proc symbols to reflect the new (nice) name.

Sorry for the late answer, sure. Missed that.

mmaka1 · 2022-12-07T12:26:01Z

Using core and core_id as symbols is a bit confusing, especially that the first variable contains a core id too. core could be renamed as adjusted_core_id or sth similar.

agree, changed

mmaka1 · 2022-12-07T12:34:53Z

Not sure why core is of signed int type (also in many other places in the code) while this piece of the code (as many others) works only for sequential values in 0..CONFIG_CORE_COUNT-1 range. Note that cpu_get_id() is now mapped to Zephyr's arch_proc_id() that returns uint32_t. Note that the latter does not guarantee the condition mentioned above (see https://docs.zephyrproject.org/latest/hardware/porting/arch.html#c.arch_proc_id), so indexing kcps_consumption with core might not be safe, unless there is some integration note but I did not find anything in place the cpu_get_id() is declared/defined.

That's the way it's done in other parts of code. I also couldn't find any spec guaranteeing core ids to be continues and starting at 0. I can imagine something like platform specific iterator for enumerating cores, but that is probably not the place for such discussion. And it would require refactoring other code too.
I did change the iterators to be unsigned, just for the sake of it :)

mmaka1 · 2022-12-07T12:37:20Z

Defining multiple exit points and requiring serious cleanup like unlock in all that places might be less readable that setting error code to the local variable and jump to the label near the end of hte function.

lgirdwood · 2022-12-14T14:03:28Z

This needs to be POSIX and do the IPC4 translation in the IPC layer.

I see, in base_fw.c they are mixed up. I suppose that's just lack of cleanup?

btian1 · 2023-01-03T06:16:47Z

Is there a way to adjust DSP clock at runtime instead of at beginning? this is because beginning performance is poor at the first 1-3 ms running.

Initial implementation of MCPS budget manager. FW can request/free MCPS usage on enabling certain FW elements. Caution should be taken when processing on different cores, as kcps_budget_data is uncache only. The manager will change clock source to match requested MCPS consumption. Budget manager may interfere with manually setting clock values! Signed-off-by: Krzysztof Frydryk <krzysztofx.frydryk@intel.com>

Handle register_kcps, resource_allocation_request for kcps, power_state_info_get. Signed-off-by: Krzysztof Frydryk <krzysztofx.frydryk@intel.com>

kfrydryx · 2023-01-04T08:35:35Z

@btian1 Well... that's what this is for. The desired model is to register everything that is running on the fw and change clock accordingly at runtime. At the moment there's not enough performance data for this to make sense, so that PR consists only of base mechanism and some IPC handling as usage.
Also, at least for MTL, I believe the clock is hardcode selected to fastest at boot.

btian1 · 2023-01-04T11:46:59Z

ok，we will have mcps data later, Q1 will happen, for one single playback, expectation is change clock one time except there are new pipe join, since change clock also takes time.

Also I would echo Liam's suggestion, title and file name better come up with: cpu-clk-manager.c/.h or adsp-clk-manager.c/h

lgirdwood · 2023-01-06T15:17:40Z

@pblaszko good for you now ?

Review request changes have been made.

kfrydryx force-pushed the mcps_pr branch 5 times, most recently from 4091080 to d99810d Compare November 23, 2022 08:24

abonislawski requested changes Nov 23, 2022

View reviewed changes

kfrydryx force-pushed the mcps_pr branch 2 times, most recently from 77dfed5 to e6436e4 Compare November 23, 2022 10:38

lgirdwood reviewed Nov 23, 2022

View reviewed changes

abonislawski reviewed Nov 23, 2022

View reviewed changes

btian1 requested review from btian1 and ranj063 November 25, 2022 02:11

btian1 reviewed Nov 25, 2022

View reviewed changes

kfrydryx force-pushed the mcps_pr branch 8 times, most recently from 15a918d to 7dbef99 Compare November 28, 2022 13:26

tmleman requested changes Nov 28, 2022

View reviewed changes

Comment thread src/audio/base_fw.c Resolved

kfrydryx force-pushed the mcps_pr branch 4 times, most recently from 657ce00 to acc9e9e Compare November 28, 2022 15:25

tmleman reviewed Nov 28, 2022

View reviewed changes

Comment thread src/audio/base_fw.c Resolved

kfrydryx force-pushed the mcps_pr branch from f94d8b5 to 5fd55c0 Compare November 30, 2022 13:50

marc-hb removed their request for review November 30, 2022 18:51

kfrydryx force-pushed the mcps_pr branch from 5fd55c0 to d45a305 Compare December 1, 2022 08:51

pblaszko previously requested changes Dec 2, 2022

View reviewed changes

Comment thread src/lib/kcps-budget-manager.c Resolved

Comment thread src/lib/kcps-budget-manager.c Resolved

pblaszko reviewed Dec 2, 2022

View reviewed changes

kfrydryx force-pushed the mcps_pr branch from d45a305 to 9e95e36 Compare December 2, 2022 15:47

lgirdwood reviewed Dec 5, 2022

View reviewed changes

kfrydryx force-pushed the mcps_pr branch 2 times, most recently from 76d2a24 to cf08404 Compare December 7, 2022 11:00

mmaka1 reviewed Dec 7, 2022

View reviewed changes

kfrydryx force-pushed the mcps_pr branch from cf08404 to 243cecc Compare December 13, 2022 15:18

lgirdwood reviewed Dec 14, 2022

View reviewed changes

kfrydryx force-pushed the mcps_pr branch from 243cecc to abe41f4 Compare January 4, 2023 08:12

kfrydryx added 2 commits January 4, 2023 09:13

ipc4: basefw: Handle kcps related IPCs

8ae6c4c

Handle register_kcps, resource_allocation_request for kcps, power_state_info_get. Signed-off-by: Krzysztof Frydryk <krzysztofx.frydryk@intel.com>

kfrydryx force-pushed the mcps_pr branch from abe41f4 to 8ae6c4c Compare January 4, 2023 08:15

lgirdwood approved these changes Jan 4, 2023

View reviewed changes

btian1 approved these changes Jan 5, 2023

View reviewed changes

tmleman approved these changes Jan 5, 2023

View reviewed changes

abonislawski approved these changes Jan 9, 2023

View reviewed changes

mwasko approved these changes Jan 9, 2023

View reviewed changes

lgirdwood merged commit e584701 into thesofproject:main Jan 9, 2023

kv2019i mentioned this pull request Aug 8, 2023

[FEATURE][IPC4] Adjust DSP frequency based on pipeline cost data (CPC/CPS) #7267

Closed

Conversation

kfrydryx commented Nov 22, 2022

Uh oh!

abonislawski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abonislawski left a comment

Choose a reason for hiding this comment

Uh oh!

btian1 commented Nov 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btian1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfrydryx commented Nov 28, 2022

Uh oh!

Uh oh!

lgirdwood commented Dec 2, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfrydryx commented Dec 2, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment