board: intel_adsp_ace15_mtpm: Disable IMR context save for MTL

[mengdonglin]

Disable IMR context save on MTL by default, as it isn't a mandatory feature for MTL.

The feature implementation is kept. So users can set CONFIG_ADSP_IMR_CONTEXT_SAVE=y to use this feature.

This is also to save some latency for audio to suspend #8071

[btian1]

if not for merge purpose, please change to DRAFT:

[gkbldcig]

Can one of the admins verify this patch?

[mengdonglin]

SOFCI TEST

[lgirdwood]

@mengdonglin thanks, lets enable when needed by developers.

[mengdonglin]

@kv2019i @lgirdwood @gkbldcig @abonislawski

• All CI functionality tests are passing now. The previous Jenkins CI failure on system suspend/resume case is an RTC issue, not audio issue.
• Sparse Zephyr / warnings-subset (tgl) and (mtl) are common warnings as other PRs.
• I will check failure with 'Internal Intel CI System/merge/build`

[lgirdwood]

@wszypelt @lrudyX is this a false positive, can someone check ? Thanks !

[mengdonglin]

@wszypelt @lrudyX I checked QB CI log: https://quickbuild.igk.intel.com/build/12698102/log It seems building completed successfully for MTL and LNL, but CI cannot trigger smoke test. Can you help check where it fails?

[mengdonglin]

@RanderWang @lyakh @kv2019i @abonislawski Can you help evaluate whether setting CONFIG_ADSP_IMR_CONTEXT_SAVE = n will break IPC4 multi-core support?

I learned from @RanderWang that cavs2.5 platform needs an extra 'z_init_cpu' to support multi-core. But since in https://github.com/thesofproject/sof/blob/main/zephyr/lib/cpu.c, we don't know if the SoC is MTL or cavs2.5, so we depend on CONFIG_ADSP_IMR_CONTEXT_SAVE to distinguish MTL from cavs2.5:
9b08ce0 zephyr: cpu: use correct condition to init cpu (This is the fixup of the 1st commit)
84befa2 zephyr: cpu: init cpu if context save is not support

@RanderWang Can you please confirm MTL doesn't need 'z_init_cpu' no matter CONFIG_ADSP_IMR_CONTEXT_SAVE is set or not?

[RanderWang]

@RanderWang @lyakh @kv2019i @abonislawski Can you help evaluate whether setting CONFIG_ADSP_IMR_CONTEXT_SAVE = n will break IPC4 multi-core support?

I learned from @RanderWang that cavs2.5 platform needs an extra 'z_init_cpu' to support multi-core. But since in https://github.com/thesofproject/sof/blob/main/zephyr/lib/cpu.c, we don't know if the SoC is MTL or cavs2.5, so we depend on CONFIG_ADSP_IMR_CONTEXT_SAVE to distinguish MTL from cavs2.5: 9b08ce0 zephyr: cpu: use correct condition to init cpu (This is the fixup of the 1st commit) 84befa2 zephyr: cpu: init cpu if context save is not support

@RanderWang Can you please confirm MTL doesn't need 'z_init_cpu' no matter CONFIG_ADSP_IMR_CONTEXT_SAVE is set or not?

@tmleman MTL doesn't need 'z_init_cpu' for your comment ?

[wszypelt]

@mengdonglin @lgirdwood
There were problems yesterday, in the internal service, PR is in the queue for retests, there should be correct results soon

[lyakh]

But since in https://github.com/thesofproject/sof/blob/main/zephyr/lib/cpu.c, we don't know if the SoC is MTL or cavs2.5

sorry, don't understand why we cannot check the DSP version there?

[tmleman]

@tmleman MTL doesn't need 'z_init_cpu' for your comment ?

Yes, if we want to disable this feature we need to change this condition. Second cpu init will break multicore support on MTL.

[tmleman]

I'm blocking it so it doesn't get merge too early. Additional changes are required.

[mengdonglin]

Yes, if we want to disable this feature we need to change this condition. Second cpu init will break multicore support on MTL.

@tmleman Thanks for the confirmation! Can you please share more info why second cpu init will break multicore support on MTL (even w/o IMR context save?) but not on cavs2.5 platforms?

And can we make another PR to decide whether it's MTL or cavs2.5 in https://github.com/thesofproject/sof/blob/main/zephyr/lib/cpu.c#L151, something like this?

--- a/zephyr/lib/cpu.c
+++ b/zephyr/lib/cpu.c
@@ -148,7 +148,7 @@ int cpu_enable_core(int id)
         * initialization. By reinitializing the idle thread, we would overwrite the kernel structs
         * and the idle thread stack.
         */
-       if (!IS_ENABLED(CONFIG_ADSP_IMR_CONTEXT_SAVE) ||
+       if (!defined(CONFIG_ACE) ||
            pm_state_next_get(id)->state == PM_STATE_ACTIVE)
                z_init_cpu(id);
 #endif

CONFIG_ACE is defined for MTL in https://github.com/thesofproject/sof/blob/main/src/platform/Kconfig#L22

@RanderWang @lyakh What do you think?

[btian1]

is it for LNL PORed feature? if not, I would suggest disable on LNL as well.
intel_adsp_ace20_lnl.conf.

[mengdonglin]

is it for LNL PORed feature? if not, I would suggest disable on LNL as well. intel_adsp_ace20_lnl.conf.
@btian1 No, it's not POR for LNL. We can sort out the dependencies on MTL at first and then look at LNL.

[lyakh]

Yes, if we want to disable this feature we need to change this condition. Second cpu init will break multicore support on MTL.

@tmleman Thanks for the confirmation! Can you please share more info why second cpu init will break multicore support on MTL (even w/o IMR context save?) but not on cavs2.5 platforms?

And can we make another PR to decide whether it's MTL or cavs2.5 in https://github.com/thesofproject/sof/blob/main/zephyr/lib/cpu.c#L151, something like this?

--- a/zephyr/lib/cpu.c
+++ b/zephyr/lib/cpu.c
@@ -148,7 +148,7 @@ int cpu_enable_core(int id)
         * initialization. By reinitializing the idle thread, we would overwrite the kernel structs
         * and the idle thread stack.
         */
-       if (!IS_ENABLED(CONFIG_ADSP_IMR_CONTEXT_SAVE) ||
+       if (!defined(CONFIG_ACE) ||
            pm_state_next_get(id)->state == PM_STATE_ACTIVE)
                z_init_cpu(id);
 #endif

CONFIG_ACE is defined for MTL in https://github.com/thesofproject/sof/blob/main/src/platform/Kconfig#L22

@RanderWang @lyakh What do you think?

if that's what works (and after replacing defined with IS_ENABLED) - then yes. But I don't understand the logic now: so on MTL we don't need to initialise the idle thread on secondary cores even if not saving to IMR? And on TGL we will re-initialise even if saving to IMR is enabled (if it is available on TGL)?

[tmleman]

But I don't understand the logic now: so on MTL we don't need to initialise the idle thread on secondary cores even if not saving to IMR?

@mengdonglin @lyakh when you are doing set_dx only for secondary cores no IMR context save takes place. Context save to IMR is done only when we are disabling primary core.

In case of a secondary core, Idle thread stack is still in memory and by calling z_init_cpu(id) we would overwrite it.

I discussed this with the team and we would prefer this feature to be disabled only on release branches. I suggest we discuss this further on Tuesday's sync.

[lyakh]

Context save to IMR is done only when we are disabling primary core.

@tmleman I don't think that was my concern. I just meant that specific change from checking for CONFIG_ADSP_IMR_CONTEXT_SAVE to checking for CONFIG_ACE. Currently the code means, that

1. if IMR context saving is enabled, then only run z_init_cpu() on boot, not when waking up from D3
2. if IMR context saving is disabled, run z_init_cpu() both on boot and on wake up.

If we replace the check for IMR_CONTEXT_SAVE with a check for ACE this would mean:

1. on MTL only run z_init_cpu() on boot, not when waking up from D3 - regardless whether saving to IMR is enabled or not
2. on other platforms run z_init_cpu() both on boot and on wake up - regardless whether saving to IMR is enabled or not

[tmleman]

If we replace the check for IMR_CONTEXT_SAVE with a check for ACE this would mean:

1. on MTL only run z_init_cpu() on boot, not when waking up from D3 - regardless whether saving to IMR is enabled or not
2. on other platforms run z_init_cpu() both on boot and on wake up - regardless whether saving to IMR is enabled or not

This is how it should look like.

[mengdonglin]

@lyakh @tmleman Thanks for your comments! I updated the PR with a new commit to only run z_init_cpu() on boot for MTL. Can you please review?

@kv2019i I will make another PR for v2.7 to run stress test.

[mengdonglin]

@kv2019i #8156 is submitted to test if we can disable IMR context save for v2.7

[lyakh]

How about something like this for the comment in that file:

/*
 * Not every secondary core startup should execute z_init_cpu(). The condition pm_state_next_get(id)->state == 
 * PM_STATE_ACTIVE holds only when the DSP boots for the first time or the DSP resumes from D3 and saving
 * context to IMR isn't used. This is exactly the condition to execute z_init_cpu()  on ACE.
 * However, on other platforms z_init_cpu() should be run always when a secondary core is brought up -
 * regardless of whether it's the first boot and whether or not context saving to IMR is enabled.
 */

But please double-check that this is exactly what we need because it sounds strange.

[tmleman]

@mengdonglin Do I understand it correctly? We will disable CONFIG_ADSP_IMR_CONTEXT_SAVE on main branch for purpose of 2.7 release and then re-enable it?

[kv2019i]

@tmleman @mengdonglin Let's not merge this for main and instead just do a patch to stable-v2.7