-
Notifications
You must be signed in to change notification settings - Fork 314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MTL enters D3 power state using Zephyr Power Manager #7325
MTL enters D3 power state using Zephyr Power Manager #7325
Conversation
412cf15
to
d12d27a
Compare
Can one of the admins verify this patch? |
1f23080
to
744c2b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor nits (see comments inline), but nothing major.
src/platform/intel/ace/platform.c
Outdated
storage_buffer_size += LP_SRAM_SIZE; | ||
|
||
/* allocate IMR buffer and store it in the global pointer */ | ||
global_imr_ram_storage = rmalloc(SOF_MEM_ZONE_SYS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit funky to have Zephyr store state into address that is allocated elsewhere. But I guess as long as the L3 heap is implemented on SOF side, we have to do it like this.
pm_policy_state_lock_get(PM_STATE_SOFT_OFF, PM_ALL_SUBSTATES); | ||
|
||
/* attach handlers */ | ||
intel_adsp_ipc_set_message_handler(INTEL_ADSP_IPC_HOST_DEV, message_handler, ipc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this patch, but realized this interface is not really well named, this should be used for all SOF platforms, not just Intel DSPs, so "intel_adsp_" prefix is not good here. But alas this is not added by this PR. FYI to @juimonen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so this needs to be generically named.
intel_adsp_ipc_set_suspend_handler(INTEL_ADSP_IPC_HOST_DEV, | ||
ipc_device_suspend_handler, ipc); | ||
intel_adsp_ipc_set_resume_handler(INTEL_ADSP_IPC_HOST_DEV, | ||
ipc_device_resume_handler, ipc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these are no-ops on platforms where RAM context is not maintained in D3? I.e. it's ok to still set them.
744c2b0
to
3e6d01a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think we need to revisist some API naming to be more generic as a subsequent PR and synchronise the west ID after dependencues are merged..
pm_policy_state_lock_get(PM_STATE_SOFT_OFF, PM_ALL_SUBSTATES); | ||
|
||
/* attach handlers */ | ||
intel_adsp_ipc_set_message_handler(INTEL_ADSP_IPC_HOST_DEV, message_handler, ipc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so this needs to be generically named.
3e6d01a
to
07e066c
Compare
07e066c
to
6c98bb3
Compare
@aborisovich do the host drivers need to do anything different to enable IMR context save ? |
No, as far as I know they don't. |
f945d10
to
76d0380
Compare
Resolved issue with the too long log by removing function name prefix. Turned out Zephyr logging provides this name prefix by default so it is not needed, thanks @tmleman . |
It had been proved over and over again on many technical and non-technical subjects, that Linux Kernel project leadership is not an entity listening to anyone's reasoning so, no thank you. I will restrain myself from providing references with links because it can be easily found in the internet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments but nothing major, so I'll put my approval on here. Let's see how the tests look like.
storage_buffer_size = tlb_api->get_storage_size(); | ||
|
||
/* add space for LPSRAM */ | ||
storage_buffer_size += LP_SRAM_SIZE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: it's not so obvious this is coming from Zephyr adsp_memory.h . Might be more readable to just directly use "DT_REG_SIZE(DT_NODELABEL(sram1)" as this a Zephyr-only file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I would leave this matter to the author @marcinszkudlinski since it is his commit.
He is currently on vacation. Can we address this later? I will let him know.
The Linux kernel has been a rather successful project and likely the most successful open-source project. Please keep your rants to yourself, it's not helpful really. |
76d0380
to
4b86668
Compare
Push update:
|
My rants are my rants. My argumentation is my argumentation. The former is not needed, the latter is valid. |
During PowerOff (D3) transition Zephyr Power Manager must have a pointer in IMR to save the LP/HPSRAM memory before powering off. As zephyr has no access to IMR heap, the memory must be allocated by SOF Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
…SIVE Zephyr turns on by default CONFIG_PM_DEVICE_RUNTIME_EXCLUSIVE option what causes Devices using Zephyr Device System Power Manager to be ignored during SoC power transition. Disabled this option so we can use default Zephyr kernel behavior to shut down Devices. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
Added IPC4 callbacks 'suspend_handler' and 'resume_handler' to control Zephyr IPC driver. Co-developed-by: Tomasz Leman <tomasz.m.leman@intel.com> Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com> Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
Enabled CONFIG_ADSP_IMR_CONTEXT_SAVE option in Kconfig in the board configuration. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
During IPC Device power transition errors may be encountered related to invalid system state or IPC messages waiting to be send. In this case we are going to send an IPC response to Host bypassing schedulers. IPC Device will not change power state when errors occur preventing whole system from the suspend. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
4b86668
to
d1aee88
Compare
Is the test failure |
@aborisovich wrote:
Yes, it's a known sporadic fail we see. So https://sof-ci.01.org/sofpr/PR7325/build10138/devicetest/index.html looks good. Of course given your PR changes the D3 flow, we'd ideally have a reliable pass as the baseline to compare against, but alas we don't have that here. Let's see the "System/merge/build" results. |
Marcin's comments have been addressed and he is not available for re-review this week
Known fail in https://sof-ci.01.org/sofpr/PR7325/build10139/devicetest/index.html . https://sof-ci.01.org/sofpr/PR7325/build10138/devicetest/index.html actually looks better than other PRs today (no hits of #7482 ), so this might actually improve the situation. Proceeding with merge. |
Suspected regression |
This PR enables Zephyr IPC driver to enter/leave D3 power state using Zephyr Power Manager.
Commit 98f8cf214844700585fa277fc66245f212e20a27 adds pm notifiers that are triggered every time power state is changed.
Commit 8e4eeab67ebb8baf9e3ceb5db01f5301e889a34d adds IPC4 driver resume/suspend capability.
Commit a6e531b81d0b0ecea7881941e046dc8969bd3d1d enables IMR context save/restore.
Commit e17fd43847da9f61836406b85990ef4d97c8f3c1 provides error handling mechanism for IPC response to Host when errors during IPC power transition are encountered.
Dependecies:
Signed-off-by: Andrey Borisovich andrey.borisovich@intel.com