[DNM] intel_adsp: ace: dynamic clock switching #8423

tmleman · 2023-10-31T11:59:34Z

This pull request introduces a series of enhancements to the power management capabilities of the Intel ADSP platforms. The changes include the implementation of dynamic clock switching, which allows the DSP to adjust its clock frequency based on the idle state of the cores, thereby reducing power consumption during idle periods.

Depends on:

lyakh · 2024-01-02T14:05:05Z

src/ipc/ipc4/handler.c

@@ -516,13 +516,18 @@ static int ipc_wait_for_compound_msg(void)
 	int try_count = 30; /* timeout out is 30 x 10ms so 300ms for IPC */

 	while (atomic_read(&msg_data.delayed_reply)) {
+		/* preventing clock switching in idle */
+		pm_runtime_disable(CORE_HP_CLK, PLATFORM_PRIMARY_CORE_ID);
 		k_sleep(Z_TIMEOUT_MS(10));


I don't know the hardware details, but naively this feels like the opposite to what one would want: when sleeping you'd want preferably to switch off any clocks, do some kind of a wait-for-interrupt operation, and instead we turn the clock on to a maximum frequency?

Maybe I implemented it in the wrong function. But the goal here is not to turn on the highest clock but to prevent switching to the lowest one if a higher one is already in use.

ok, so the risk is that the sleep() would put us in a lower clock at Zephyr level and may impact other audio or the timeout ? Maybe best to add more context in the comments to reflect the why.

lgirdwood

@tmleman I assume you are happy with the testing/validation ?

lgirdwood · 2024-01-11T13:59:17Z

src/ipc/ipc4/handler.c

@@ -516,13 +516,18 @@ static int ipc_wait_for_compound_msg(void)
 	int try_count = 30; /* timeout out is 30 x 10ms so 300ms for IPC */

 	while (atomic_read(&msg_data.delayed_reply)) {
+		/* preventing clock switching in idle */
+		pm_runtime_disable(CORE_HP_CLK, PLATFORM_PRIMARY_CORE_ID);
 		k_sleep(Z_TIMEOUT_MS(10));


ok, so the risk is that the sleep() would put us in a lower clock at Zephyr level and may impact other audio or the timeout ? Maybe best to add more context in the comments to reflect the why.

btian1

is this PR a replacement with:#8019

tmleman · 2024-01-19T13:43:57Z

is this PR a replacement with:#8019

No, this change is not intended to replace it. It's more of an extension. Thanks for asking; I've added this PR as a dependency.

lgirdwood · 2024-01-24T14:38:38Z

is this PR a replacement with:#8019

No, this change is not intended to replace it. It's more of an extension. Thanks for asking; I've added this PR as a dependency.

Thanks @tmleman - pls keep this PR up to date with testing/readyness indicators. Lets see if we can merge this and #8019 for v2.9.

kv2019i · 2024-03-04T13:42:44Z

Stable-v2.9 branched, this didn't make the cut, bumping to 2.10.

lgirdwood

LGTM, but we need to plan to move our platform PM logic into Zephyr.

lgirdwood · 2024-04-09T15:58:41Z

zephyr/lib/pm_runtime.c

@@ -74,6 +80,12 @@ void platform_pm_runtime_enable(uint32_t context, uint32_t index)
 		tr_dbg(&power_tr, "removing prevent on d0i3 (lock is active=%d)",
 		       pm_policy_state_lock_is_active(PM_STATE_RUNTIME_IDLE, PM_ALL_SUBSTATES));
 		break;
+#if CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING


Btw, what's the long term plan to move this into Zephyr as I would expect we should all the platform PM logic in Zephyr.

We won't be able to move everything related to PM to Zephyr. We need to prevent changes to the clock and dynamic power gating on the application side. What can definitely be moved to Zephyr is the custom power policy (several changes in this direction have already been made).

ack, but we should do that with Zephyr PM APIs. e.g. zephyr_pm_prevent_clock_gating(CLOCK_ID) so that eventually we have zero IP code in SOF and its all in Zephyr. I know we cant yet do this today, but just for the record that we may have to extend Zephyr APIs here.

zephyr_pm_prevent_clock_gating(CLOCK_ID)

btw, we have clock gating, this is about clock switching.

I was wondering if there is really a benefit from idle clock switching (with switching overhead) if we have hw clock gating anyway

PR containing the required changes. Signed-off-by: Tomasz Leman <[email protected]>

Enable the CONFIG_ARCH_CPU_IDLE_CUSTOM option for ACE platforms (meteorlake and lunarlake). This configuration allows the use of a custom arch_cpu_idle implementation, providing opportunities for platform-specific optimizations in power management. Signed-off-by: Tomasz Leman <[email protected]>

Integrate dynamic clock switching support into the power management runtime system for Intel ADSP platforms. This patch ensures that the clock switching mechanism is controlled in a thread-safe manner during power management operations. Changes include: - Initializing the intel_adsp_ace_pm_data structure to manage the clock switching lock state. - Modifying platform_pm_runtime_enable/disable functions to lock and unlock clock switching when the CORE_HP_CLK context is affected. Signed-off-by: Tomasz Leman <[email protected]>

Ensure a stable clock frequency in the IPC4 handler during the wait for a delayed reply. This is achieved by disabling dynamic clock switching when entering the wait loop and re-enabling it upon exit or when a timeout occurs. The patch addresses potential timing issues that could arise from clock frequency changes while the core is in an idle state awaiting a response from a different thread that was scheduled by this IPC. The added calls to pm_runtime_disable/enable ensure that the high performance clock remains active during the critical wait period, thus maintaining the necessary processing speed and responsiveness of the IPC mechanism. Signed-off-by: Tomasz Leman <[email protected]>

With this patch, the adsp_clock_idle_exit function is called within the IPC message handler to reset the clock frequency if it was lowered during idle periods. This is crucial for maintaining the performance and responsiveness of the system when processing incoming IPC messages. The patch is conditionally compiled only when the CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING configuration option is enabled, ensuring that the functionality is available only on systems that support dynamic clock management. Signed-off-by: Tomasz Leman <[email protected]>

The adsp_clock_idle_exit function is invoked at the beginning of the scheduler thread function to reset the clock frequency if it was lowered during idle periods. This change is crucial for maintaining the performance and responsiveness of the system when it resumes work after being idle. This patch is conditionally compiled with CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING to ensure that the functionality is available only on systems that support dynamic clock management. Signed-off-by: Tomasz Leman <[email protected]>

Enable the dynamic clock switching feature for Intel ADSP platforms. This feature allows the DSP to adjust its clock frequency dynamically when all cores enter the idle state, reducing energy consumption while waiting for interrupts. The following Kconfig options are added to MTL and LNL configuration: - CONFIG_PM_STATE_CUSTOM_DATA: Allows setting custom data pointers for specific power states. - CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING: Enables dynamic clock switching during idle states, dependent on the availability of custom power state data. This change is part of ongoing efforts to enhance power management capabilities and improve the energy efficiency of these platforms. Signed-off-by: Tomasz Leman <[email protected]>

abonislawski · 2024-04-18T07:42:13Z

app/boards/intel_adsp_ace15_mtpm.conf

@@ -15,13 +15,17 @@ CONFIG_PM_DEVICE_RUNTIME=y
 CONFIG_PM_DEVICE_RUNTIME_EXCLUSIVE=n
 CONFIG_PM_DEVICE_POWER_DOMAIN=y
 CONFIG_PM_POLICY_CUSTOM=y
+CONFIG_PM_STATE_CUSTOM_DATA=y
+CONFIG_ADSP_DYNAMIC_CLOCK_SWITCHING=y


I strongly suggest to rename it (in zephyr) to CONFIG_ADSP_IDLE_DYNAMIC_CLOCK_SWITCHING or something like that to avoid confusion with all kconfigs about clocks and kcps

abonislawski · 2024-04-18T07:42:17Z

src/ipc/ipc4/handler.c

@@ -536,16 +536,20 @@ static int ipc_wait_for_compound_msg(void)
 {
 	int try_count = 30;

+	pm_runtime_disable(CORE_HP_CLK, PLATFORM_PRIMARY_CORE_ID);


Is this still needed? Will this work correctly with KCPS which changes clock during IPC?

kv2019i

A few comments inline.

kv2019i · 2024-04-18T08:36:02Z

app/boards/intel_adsp_ace15_mtpm.conf

@@ -22,6 +22,8 @@ CONFIG_POWER_DOMAIN_INTEL_ADSP=y
 CONFIG_ADSP_IMR_CONTEXT_SAVE=y
 CONFIG_ADSP_IDLE_CLOCK_GATING=y

+CONFIG_ARCH_CPU_IDLE_CUSTOM=y


Btw, why do you need this? I don't see a custom idle handled added in this patch.

This setting is now enabled in upstream by default (the new icache-workaround), so no need to enable this on SOF side anymore.

kv2019i · 2024-04-18T08:51:44Z

src/ipc/ipc4/handler.c

@@ -536,16 +536,20 @@ static int ipc_wait_for_compound_msg(void)
 {
 	int try_count = 30;


Could we generalize this a bit? I guess this could be a Kconfig to "keep core clock high when responding to IPC".
The actual implementation will vary on different platforms, so ideally we'd have a Zephyr interface to communicate this constraints, but for the time being we might need glue code on SOF side (e.g. for Intel this is turned into pm_runtime call on CORE_HP_CLK).

I'm ok to also go with a TODO comment for now if there's no direct Zephyr interface yet.

tmleman · 2024-06-20T07:59:58Z

The changes this PR depends on will not be merged into Zephyr in the near future, so I'm closing it.

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from 21a83d2 to f89029e Compare November 15, 2023 13:08

tmleman mentioned this pull request Dec 6, 2023

intel_adsp: ace: clock gating in idle zephyrproject-rtos/zephyr#64468

Merged

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from f89029e to 687de9f Compare December 22, 2023 12:51

tmleman changed the title ~~[CIONLY] intel_adsp: clock optimizations~~ intel_adsp: ace: dynamic clock switching Dec 22, 2023

tmleman changed the title ~~intel_adsp: ace: dynamic clock switching~~ [DNM] intel_adsp: ace: dynamic clock switching Dec 22, 2023

tmleman marked this pull request as ready for review December 22, 2023 13:03

tmleman requested review from kv2019i, lyakh, iuliana-prodan, dbaluta, abonislawski, pblaszko, marcinszkudlinski, LaurentiuM1234, bardliao, lgirdwood, plbossart, mmaka1 and lbetlej as code owners December 22, 2023 13:03

tmleman mentioned this pull request Dec 22, 2023

intel_adsp: ace: dynamic clock switching zephyrproject-rtos/zephyr#66092

Closed

2 tasks

lyakh reviewed Jan 2, 2024

View reviewed changes

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch 2 times, most recently from c08a156 to 16145c1 Compare January 11, 2024 13:06

lgirdwood reviewed Jan 11, 2024

View reviewed changes

btian1 reviewed Jan 15, 2024

View reviewed changes

lgirdwood added this to the v2.9 milestone Jan 17, 2024

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from 16145c1 to 8d5e0aa Compare January 23, 2024 11:53

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from 8d5e0aa to b6e1620 Compare January 24, 2024 15:13

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch 2 times, most recently from 50bcb65 to 9574435 Compare January 29, 2024 14:56

kv2019i modified the milestones: v2.9, v2.10 Mar 4, 2024

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from 9574435 to 9b542d8 Compare April 9, 2024 14:32

lgirdwood reviewed Apr 9, 2024

View reviewed changes

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from 9b542d8 to 9c18503 Compare April 17, 2024 10:21

tmleman added 7 commits April 18, 2024 09:14

[DRAFT] west.yml: zephyr version update

5183a8b

PR containing the required changes. Signed-off-by: Tomasz Leman <[email protected]>

tmleman force-pushed the topic/upstream/dev/intel/adsp/clock_optimizations branch from 9c18503 to c11531b Compare April 18, 2024 07:14

abonislawski reviewed Apr 18, 2024

View reviewed changes

kv2019i reviewed Apr 18, 2024

View reviewed changes

lgirdwood modified the milestones: v2.10, v2.11 Jun 10, 2024

tmleman closed this Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNM] intel_adsp: ace: dynamic clock switching #8423

[DNM] intel_adsp: ace: dynamic clock switching #8423

tmleman commented Oct 31, 2023 •

edited

Loading

lyakh Jan 2, 2024

tmleman Jan 11, 2024

lgirdwood Jan 11, 2024

lgirdwood left a comment

lgirdwood Jan 11, 2024

btian1 left a comment

tmleman commented Jan 19, 2024

lgirdwood commented Jan 24, 2024

kv2019i commented Mar 4, 2024

lgirdwood left a comment

lgirdwood Apr 9, 2024

tmleman Apr 17, 2024

lgirdwood Apr 17, 2024

abonislawski Apr 18, 2024

abonislawski Apr 18, 2024

abonislawski Apr 18, 2024

kv2019i left a comment

kv2019i Apr 18, 2024

kv2019i Apr 18, 2024

tmleman commented Jun 20, 2024

		@@ -536,16 +536,20 @@ static int ipc_wait_for_compound_msg(void)
		{
		int try_count = 30;

[DNM] intel_adsp: ace: dynamic clock switching #8423

[DNM] intel_adsp: ace: dynamic clock switching #8423

Conversation

tmleman commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 left a comment

Choose a reason for hiding this comment

tmleman commented Jan 19, 2024

lgirdwood commented Jan 24, 2024

kv2019i commented Mar 4, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmleman commented Jun 20, 2024

tmleman commented Oct 31, 2023 •

edited

Loading