[v2.7] Disable IMR context save for MTL #8156
Conversation
mengdonglin
requested review from
tmleman,
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
zephyr/lib/cpu.c
Outdated
| @@ -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) || | |||
There was a problem hiding this comment.
@mengdonglin looks good. Can we also update the comment above to call out the difference between cavs and ACE?
There was a problem hiding this comment.
@ranj063 Thanks for your review. I updated the comments in the code and also fix check on CONFIG_ACE
mengdonglin
force-pushed
the
stable-v2.7
branch
from
e951375
to
c1794e5
Compare
zephyr/lib/cpu.c
Outdated
| */ | ||
| #if defined(CONFIG_ACE) | ||
| /* On MTL+ platforms, only run z_init_cpu() on boot, not when waking up from D3, | ||
| * regardless whether saving to IMR is enabled or not. |
There was a problem hiding this comment.
this is not how I understood the flow in a discussion yesterday. If on MTL+ IMR context saving isn't enabled, then waking up from D3 will be equivalent to the first boot, so the pm_state_next_get(id)->state == PM_STATE_ACTIVE condition will be true and z_init_cpu() will be run.
There was a problem hiding this comment.
this is not how I understood the flow in a discussion yesterday. If on MTL+ IMR context saving isn't enabled, then waking up from D3 will be equivalent to the first boot, so the
pm_state_next_get(id)->state == PM_STATE_ACTIVEcondition will be true andz_init_cpu()will be run.
@tmleman Can you help clarify?
I also updated the PR according to your fa1572c for mtl-006.
There was a problem hiding this comment.
On MTL z_init_cpu(id) is called for secondary cores only for the first init. As I wrote in the comment in the code:
During kernel initialization, the next pm state is set to ACTIVE.
If core goes from D0 to D3, next state is set to PM_STATE_SOFT_OFF. Thanks to this we know if the core is enabled for the first time.
CONFIG_ADSP_IMR_CONTEXT_SAVE changes the behavior of the primary core during D3. If disabled, every exit from D3 for primary core is like first boot. Therefore, if we re-enable the secondary core after that, the function z_init_cpu(id) will be called.
mengdonglin
force-pushed
the
stable-v2.7
branch
from
c1794e5
to
958da58
Compare
Only run z_init_cpu() on boot for MTL by replacing the check for IMR_CONTEXT_SAVE with a check for ACE. Therefore, - on MTL, only run z_init_cpu() on boot, not when waking up from D3, regardless whether saving to IMR is enabled or not. - on other platforms, run z_init_cpu() both on boot and on waking up from D3. The difference is caused by differnet Zephyr implementations between MTL and other platforms. Moreover, saving context to IMR is a feature for MTL only, not available for other platorms. Signed-off-by: Mengdong Lin <[email protected]>
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 try this feature by setting CONFIG_ADSP_IMR_CONTEXT_SAVE=y Signed-off-by: Mengdong Lin <[email protected]>
| * different between MTL+ and other platforms. | ||
| * | ||
| * On MTL+ platforms, only run z_init_cpu() on boot, not when waking up from D3, | ||
| * regardless whether saving to IMR is enabled or not. |
There was a problem hiding this comment.
that seems wrong to me @mengdonglin
What is the difference between 'boot' and 'waking up from D3' if there is no context saved in IMR?
|
So seems we are only discussng about the comments now. We do really need to get this in to keep to the SOF2.7 timeline. Let me propose alternative wording based on comments so far. |
Ignoring... test failures? |
|
Merged via #8170 We'll follow-up to clarify the z_init_cpu() code (and comments) in mainline to get rid of platform specific checks on SOF side. |
This is a follow up of PR #7994. We want to make sure it's safe to disable IMR context save for MTL and other platforms in sof v2.7
Please find more background info in #7994 (comment)
We also want to check if disabling IMR context save can avoid the FW boot failure: