MTL enters D3 power state using Zephyr Power Manager #7325

aborisovich · 2023-03-24T02:54:24Z

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

src/platform/intel/ace/platform.c

app/boards/intel_adsp_ace15_mtpm.conf

src/ipc/ipc-zephyr.c

src/platform/intel/ace/platform.c

gkbldcig · 2023-03-28T00:20:38Z

Can one of the admins verify this patch?

app/boards/intel_adsp_ace15_mtpm.conf

src/ipc/ipc-zephyr.c

src/platform/intel/ace/platform.c

kv2019i

A few minor nits (see comments inline), but nothing major.

app/boards/intel_adsp_ace15_mtpm.conf

src/platform/intel/ace/platform.c

kv2019i · 2023-03-28T11:18:06Z

src/platform/intel/ace/platform.c

+		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,


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.

src/ipc/ipc-zephyr.c

kv2019i · 2023-03-28T11:29:22Z

src/ipc/ipc-zephyr.c

+	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);


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

Ah, so this needs to be generically named.

kv2019i · 2023-03-28T11:34:09Z

src/ipc/ipc-zephyr.c

+	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);


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.

lgirdwood

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..

lgirdwood · 2023-03-30T09:23:38Z

src/ipc/ipc-zephyr.c

+	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);


Ah, so this needs to be generically named.

src/platform/intel/ace/platform.c

west.yml

lgirdwood · 2023-04-06T20:47:17Z

@aborisovich do the host drivers need to do anything different to enable IMR context save ?

aborisovich · 2023-04-06T21:01:16Z

@aborisovich do the host drivers need to do anything different to enable IMR context save ?

No, as far as I know they don't.
Please confirm @marcinszkudlinski .

aborisovich · 2023-06-27T09:27:48Z

I do not know how to address this checkpatch issue: When this is not split across the lines, checkpatch complains that I've exceeded the 100 characters in width... Moreover, I've seen similar usages of tr_err in code done in the similar way.

long strings in log prints are tolerated and preferred over split strings. The reasoning is that split strings are difficult to grep for after seeing them in a log. But would be good to at least take that string on the next line - splitting after &ipc_tr,

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 .

aborisovich · 2023-06-27T09:36:08Z

I wonder what was author thinking when was creating this rule...

Enjoy the 2013 discussion: https://lore.kernel.org/lkml/1365563834.27174.12.camel@joe-AO722/#r

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.
The question is why SOF project follows Linux Kernel. I do not think this is a requirement but the choice we made. And now the project undergoes convergence with Windows and Chrome OS.

kv2019i

Some comments but nothing major, so I'll put my approval on here. Let's see how the tests look like.

kv2019i · 2023-06-27T10:14:45Z

zephyr/lib/cpu.c

+		storage_buffer_size = tlb_api->get_storage_size();
+
+		/* add space for LPSRAM */
+		storage_buffer_size += LP_SRAM_SIZE;


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.

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.

zephyr/lib/cpu.c

src/ipc/ipc-zephyr.c

src/ipc/ipc4/handler.c

plbossart · 2023-06-27T10:57:16Z

I wonder what was author thinking when was creating this rule...

Enjoy the 2013 discussion: https://lore.kernel.org/lkml/1365563834.27174.12.camel@joe-AO722/#r

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. The question is why SOF project follows Linux Kernel. I do not think this is a requirement but the choice we made. And now the project undergoes convergence with Windows and Chrome OS.

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.

aborisovich · 2023-06-27T10:58:57Z

Push update:

addressed @kv2019i review - fixed whitespaces, typos and changed global_imr_ram_storage pointer to be allocated from the SYS_RUNTIME memory pool.

aborisovich · 2023-06-27T11:00:21Z

I wonder what was author thinking when was creating this rule...

Enjoy the 2013 discussion: https://lore.kernel.org/lkml/1365563834.27174.12.camel@joe-AO722/#r

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. The question is why SOF project follows Linux Kernel. I do not think this is a requirement but the choice we made. And now the project undergoes convergence with Windows and Chrome OS.

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.

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>

aborisovich · 2023-06-27T13:16:05Z

Is the test failure check-suspend-resume-with-capture-5.sh on MTL nocodec topology known issue?

kv2019i · 2023-06-27T13:53:43Z

@aborisovich wrote:

Is the test failure check-suspend-resume-with-capture-5.sh on MTL nocodec topology known issue?

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

kv2019i · 2023-06-28T12:32:13Z

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.

marc-hb · 2023-07-20T06:23:58Z

Suspected regression

[BUG] FW reported error: 13 - Requested power transition failed to complete #7969

tmleman requested changes Mar 27, 2023

View reviewed changes

aborisovich force-pushed the ipc-device-wakes-from-d3 branch 3 times, most recently from 412cf15 to d12d27a Compare March 27, 2023 23:11

aborisovich marked this pull request as ready for review March 28, 2023 08:04

aborisovich requested review from bardliao, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners March 28, 2023 08:04

aborisovich requested a review from abonislawski March 28, 2023 10:35

aborisovich force-pushed the ipc-device-wakes-from-d3 branch 4 times, most recently from 1f23080 to 744c2b0 Compare March 28, 2023 10:58

tmleman requested changes Mar 28, 2023

View reviewed changes

app/boards/intel_adsp_ace15_mtpm.conf Show resolved Hide resolved

src/ipc/ipc-zephyr.c Outdated Show resolved Hide resolved

src/platform/intel/ace/platform.c Outdated Show resolved Hide resolved

kv2019i requested changes Mar 28, 2023

View reviewed changes

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 744c2b0 to 3e6d01a Compare March 29, 2023 18:25

aborisovich changed the title ~~MTL enters D3 power state using Zephyr Power Manager~~ [DNM] MTL enters D3 power state using Zephyr Power Manager Mar 30, 2023

lgirdwood reviewed Mar 30, 2023

View reviewed changes

tmleman requested changes Apr 4, 2023

View reviewed changes

src/platform/intel/ace/platform.c Show resolved Hide resolved

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 3e6d01a to 07e066c Compare April 5, 2023 11:55

marcinszkudlinski previously requested changes Apr 6, 2023

View reviewed changes

west.yml Outdated Show resolved Hide resolved

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 07e066c to 6c98bb3 Compare April 6, 2023 09:23

aborisovich force-pushed the ipc-device-wakes-from-d3 branch 2 times, most recently from f945d10 to 76d0380 Compare June 27, 2023 09:25

tmleman approved these changes Jun 27, 2023

View reviewed changes

pjdobrowolski approved these changes Jun 27, 2023

View reviewed changes

aborisovich requested a review from softwarecki June 27, 2023 09:47

kv2019i approved these changes Jun 27, 2023

View reviewed changes

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 76d0380 to 4b86668 Compare June 27, 2023 10:57

marcinszkudlinski and others added 5 commits June 27, 2023 13:12

board: intel_adsp_ace15_mtpm: Enabled MTL IMR context save

5d54785

Enabled CONFIG_ADSP_IMR_CONTEXT_SAVE option in Kconfig in the board configuration. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 4b86668 to d1aee88 Compare June 27, 2023 11:12

aborisovich mentioned this pull request Jun 27, 2023

Update mtl 005 ipc D3 #7865

Merged

softwarecki approved these changes Jun 28, 2023

View reviewed changes

kv2019i merged commit 53d471b into thesofproject:main Jun 28, 2023

marc-hb mentioned this pull request Jun 28, 2023

[Draft] Revert/pr7700 tdfb #7869

Closed

This was referenced Jun 28, 2023

[DRAFT] Revert/pr7857 pr7325 test #7873

Closed

[BUG][MTL] MOD_SET_DX causes DSP exception (very recent regression) #7875

Closed

keqiaozhang mentioned this pull request Jul 20, 2023

[BUG] FW reported error: 13 - Requested power transition failed to complete #7969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MTL enters D3 power state using Zephyr Power Manager #7325

MTL enters D3 power state using Zephyr Power Manager #7325

aborisovich commented Mar 24, 2023 •

edited

Loading

gkbldcig commented Mar 28, 2023

kv2019i left a comment

kv2019i Mar 28, 2023

kv2019i Mar 28, 2023

lgirdwood Mar 30, 2023

kv2019i Mar 28, 2023

lgirdwood left a comment

lgirdwood Mar 30, 2023

lgirdwood commented Apr 6, 2023

aborisovich commented Apr 6, 2023

aborisovich commented Jun 27, 2023

aborisovich commented Jun 27, 2023 •

edited

Loading

kv2019i left a comment

kv2019i Jun 27, 2023

aborisovich Jun 27, 2023

plbossart commented Jun 27, 2023

aborisovich commented Jun 27, 2023

aborisovich commented Jun 27, 2023

aborisovich commented Jun 27, 2023

kv2019i commented Jun 27, 2023

kv2019i commented Jun 28, 2023

marc-hb commented Jul 20, 2023

MTL enters D3 power state using Zephyr Power Manager #7325

MTL enters D3 power state using Zephyr Power Manager #7325

Conversation

aborisovich commented Mar 24, 2023 • edited Loading

gkbldcig commented Mar 28, 2023

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Mar 28, 2023

Choose a reason for hiding this comment

kv2019i Mar 28, 2023

Choose a reason for hiding this comment

lgirdwood Mar 30, 2023

Choose a reason for hiding this comment

kv2019i Mar 28, 2023

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood Mar 30, 2023

Choose a reason for hiding this comment

lgirdwood commented Apr 6, 2023

aborisovich commented Apr 6, 2023

aborisovich commented Jun 27, 2023

aborisovich commented Jun 27, 2023 • edited Loading

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Jun 27, 2023

Choose a reason for hiding this comment

aborisovich Jun 27, 2023

Choose a reason for hiding this comment

plbossart commented Jun 27, 2023

aborisovich commented Jun 27, 2023

aborisovich commented Jun 27, 2023

aborisovich commented Jun 27, 2023

kv2019i commented Jun 27, 2023

kv2019i commented Jun 28, 2023

marc-hb commented Jul 20, 2023

aborisovich commented Mar 24, 2023 •

edited

Loading

aborisovich commented Jun 27, 2023 •

edited

Loading