drivers: intel_adsp: IPC driver uses D3 power state #56168

aborisovich · 2023-03-24T02:44:12Z

This PR enables suspend/resume of the Intel ADSP IPC driver using Zephyr Power Manager Device API.
Options are enabled when CONFIG_PM_DEVICE=y.
New power control API performs hardware reinitialization after wake from D3 power state (PM_DEVICE_ACTION_SUSPEND):

enabled interrupts
resets register state to accept new messages
allows to register callbacks to Zephyr application that may perform additional actions

Consists of 4 commits (2 main commits and two dependent):

Commit 2ea0d25 sets up Power Manager calls for IPC driver
Commit fa839a9 adds returned error codes to IPC functions, returning -EBUSY when Device is during power transition.
Commit fc367d4 allows system timer re-initialization - this commit is needed to resume system from low power state.
Commit 8dcac84 allows to re-initialize memory window - this commit is also needed to complete IPC messages correctly after waking up from D3.

Blocked by bug:

pm: Subsystem is not keep tracking of active cpus correctly #58502

Signed-off-by: Andrey Borisovich andrey.borisovich@intel.com

soc/xtensa/intel_adsp/ace/power.c

soc/xtensa/intel_adsp/common/include/intel_adsp_ipc.h

pjdobrowolski · 2023-04-12T07:09:27Z

soc/xtensa/intel_adsp/common/mem_window.c

+	mem_win_init(DEVICE_DT_GET(MEM_WINDOW_NODE(0)));
+	mem_win_init(DEVICE_DT_GET(MEM_WINDOW_NODE(1)));
+	mem_win_init(DEVICE_DT_GET(MEM_WINDOW_NODE(2)));
+	mem_win_init(DEVICE_DT_GET(MEM_WINDOW_NODE(3)));


0-1-2-3 From where come that numbers?

From Devicetree instance API.
I've checked all .dtsi files from dts\xtensa\intel\ directory.
All boards have 4 memory_window instances.

aborisovich · 2023-04-12T09:59:15Z

@nashif, @ceolin please review

ceolin · 2023-06-14T05:41:38Z

soc/xtensa/intel_adsp/common/ipc.c

+{
+	if (k_is_in_isr()) {
+		return -ESRCH;
+	}


Do you really need this ??

Well, you started a subject on TOCTOU race and did not answer my question... Please read my comments above.

Nope, it does not solve it. If you have two threads that call pm_device_action_run() is possible that both threads pass the check pm_device_state_is_locked() and both ends up calling ipc_pm_action and in that case you don't know exactly what will happen. That is a problem that device_runtime solves, pm_device_state_lock() was not thought to be used this way (though there is nothing disallowing it). It was simply not thought.

My suggestion is to remove it for now (since is not really protecting) and ensure that you are calling it from a safe context. I will probably change pm_device_lock() to return a boolean and fails when the device state is already locked and you can use here it and that will probably be a solution.

Does it work for you ?

subsys/logging/backends/log_backend_adsp_hda.c

ceolin

Look inline comments please.

aborisovich · 2023-06-14T13:21:02Z

Addressed @ceolin issues except 1 where I am not sure how to proceed.
@ceolin can you please elaborate on TACTOU race you mentioned and my comments on ISR?
Tests are passing now, submitted full scope and waiting for results.

subsys/logging/backends/log_backend_adsp_hda.c

soc/xtensa/intel_adsp/common/include/intel_adsp_ipc.h

ceolin · 2023-06-20T05:02:55Z

@aborisovich please fix the return error code information that I commented and if you agree just drop that check if it is from ISR (though I won't block this commit if you decided to keep that).

Once you fix that error code mismatch it is good to me.

aborisovich · 2023-06-20T09:52:38Z

Push update - addressed @ceolin issues found:

fixed return codes (-ESHUTDOWN is used where it should).
removed check k_is_in_isr() from the ipc_pm_action along with the documentation that the action is not supported in ISR.

tmleman · 2023-06-21T08:05:04Z

@aborisovich SOF CI looks good thesofproject/sof#7325. How did our internal full scope tests go?

Generic header for system clock allows to define a sys_clock_idle_exit function for the clock implementation. Implemented the function in the intel_adsp_timer to reinitialize device driver after the idle exit state. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>

Exiting idle state requires to reinitialize all memory window instances to flush the cached memory. Added function that calls initialization of devices during runtime. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>

When option CONFIG_PM_DEVICE is enabled, IPC driver is capable of entering D3 power state described as PM_DEVICE_ACTION_SUSPEND (and leaving that state - powering back to PM_DEVICE_ACTION_ACTIVE). New power control callbacks 'ipc_power_control_api' are introduced for use during power state transisions. They allow Zephyr application specific code to be executed. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>

When CONFIG_PM_DEVICE is enabled IPC Device may be during power transition during a call to intel_adsp_ipc_send_message function. Changed signatures of intel_adsp_ipc_send_message and its sync version to return int and negative error codes on error. On attempt to send IPC message during Device power transition -ESHUTDOWN error code is returned, on busy state -EBUSY. Updated all function references. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>

aborisovich · 2023-06-21T08:40:01Z

Rebased to newest Zephyr main.

@aborisovich SOF CI looks good thesofproject/sof#7325. How did our internal full scope tests go?

We have improvement of ~1.5% passrate.
New tests passing:
hda_codec_loopback_rtd3 ✔️
channel_remapping_mode_dynamic ✔️
mixers_max_inputs ✔️
cpa_toggle_without_fw_logs ✔️
Other D3 related tests are passing too.
There is regression in 1 test:
load_fw_stress_graceful_shutdown ❌
I'll investigate on which iteration we have failures.
I'll restart full scope tests today with just done rebase.

aborisovich · 2023-06-22T08:50:16Z

Last run after Zephyr main rebase gave us ~3% passrate improvement.
Comparing to previous run, new tests started to pass:
ace_generic_dmic ✔️
ace_raw_capture ✔️
kd_topology_dmic ✔️
Previously run test passed now:
load_fw_stress_graceful_shutdown ❌ -> ✔️
Unfortunately, some of the:
hda_codec_loopback_rtd3 tests failed with audio glitches after D3.
What keeps me wonder is why only the half of the tests in this group failed and second half passed...
Anyway, even if those are unstable for some reason, core D3 tests are passing so I would merge this and look for the causes later.

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

aborisovich mentioned this pull request Mar 28, 2023

MTL enters D3 power state using Zephyr Power Manager thesofproject/sof#7325

Merged

2 tasks

aborisovich marked this pull request as ready for review March 28, 2023 09:55

aborisovich requested review from dcpleung, andyross and nashif as code owners March 28, 2023 09:55

zephyrbot added area: Timer Timer platform: Intel ADSP Intel Audio platforms labels Mar 28, 2023

zephyrbot requested review from ceolin, jxstelter, kv2019i, lgirdwood, lyakh, marc-hb, marcinszkudlinski, MaureenHelm, softwarecki, teburd and tmleman March 28, 2023 09:55

zephyrbot assigned nashif Mar 28, 2023

jxstelter previously approved these changes Mar 28, 2023

View reviewed changes

tmleman requested changes Mar 29, 2023

View reviewed changes

soc/xtensa/intel_adsp/ace/power.c Outdated Show resolved Hide resolved

soc/xtensa/intel_adsp/ace/power.c Outdated Show resolved Hide resolved

soc/xtensa/intel_adsp/common/include/intel_adsp_ipc.h Outdated Show resolved Hide resolved

aborisovich requested a review from tmleman March 30, 2023 09:44

aborisovich dismissed jxstelter’s stale review via 82e5150 March 30, 2023 11:15

aborisovich force-pushed the ipc-device-wakes-from-d3 branch 2 times, most recently from 82e5150 to afa98c5 Compare April 5, 2023 09:37

pjdobrowolski reviewed Apr 12, 2023

View reviewed changes

ceolin reviewed Jun 14, 2023

View reviewed changes

subsys/logging/backends/log_backend_adsp_hda.c Show resolved Hide resolved

ceolin requested changes Jun 14, 2023

View reviewed changes

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 4a7818d to 0e98166 Compare June 14, 2023 13:17

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 0e98166 to 1b4d5aa Compare June 14, 2023 13:34

aborisovich mentioned this pull request Jun 15, 2023

[BUG][Multi-core] ipc timed out for 0x13000004|0x1GLB_SET_PIPELINE_STATE when testing non-core 0 pipeline on MTL-NOCODEC thesofproject/sof#7649

Closed

ceolin reviewed Jun 20, 2023

View reviewed changes

subsys/logging/backends/log_backend_adsp_hda.c Outdated Show resolved Hide resolved

ceolin reviewed Jun 20, 2023

View reviewed changes

soc/xtensa/intel_adsp/common/include/intel_adsp_ipc.h Outdated Show resolved Hide resolved

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 1b4d5aa to 3f61533 Compare June 20, 2023 09:48

aborisovich requested review from ceolin and tmleman and removed request for pjdobrowolski and tmleman June 20, 2023 09:53

ceolin approved these changes Jun 20, 2023

View reviewed changes

Andrey Borisovich added 4 commits June 21, 2023 10:31

intel_adsp: mem_window: reinitialize after idle exit

97bab1c

Exiting idle state requires to reinitialize all memory window instances to flush the cached memory. Added function that calls initialization of devices during runtime. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>

aborisovich force-pushed the ipc-device-wakes-from-d3 branch from 3f61533 to 2bf3358 Compare June 21, 2023 08:31

marcinszkudlinski approved these changes Jun 22, 2023

View reviewed changes

tmleman approved these changes Jun 22, 2023

View reviewed changes

jxstelter approved these changes Jun 22, 2023

View reviewed changes

softwarecki approved these changes Jun 22, 2023

View reviewed changes

nashif merged commit 23b3cae into zephyrproject-rtos:main Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: intel_adsp: IPC driver uses D3 power state #56168

drivers: intel_adsp: IPC driver uses D3 power state #56168

aborisovich commented Mar 24, 2023 •

edited

Loading

pjdobrowolski Apr 12, 2023

aborisovich Apr 12, 2023

aborisovich commented Apr 12, 2023

ceolin Jun 14, 2023

aborisovich Jun 14, 2023

ceolin Jun 20, 2023

ceolin left a comment

aborisovich commented Jun 14, 2023 •

edited

Loading

ceolin commented Jun 20, 2023

aborisovich commented Jun 20, 2023

tmleman commented Jun 21, 2023

aborisovich commented Jun 21, 2023 •

edited

Loading

aborisovich commented Jun 22, 2023 •

edited

Loading

drivers: intel_adsp: IPC driver uses D3 power state #56168

drivers: intel_adsp: IPC driver uses D3 power state #56168

Conversation

aborisovich commented Mar 24, 2023 • edited Loading

pjdobrowolski Apr 12, 2023

Choose a reason for hiding this comment

aborisovich Apr 12, 2023

Choose a reason for hiding this comment

aborisovich commented Apr 12, 2023

ceolin Jun 14, 2023

Choose a reason for hiding this comment

aborisovich Jun 14, 2023

Choose a reason for hiding this comment

ceolin Jun 20, 2023

Choose a reason for hiding this comment

ceolin left a comment

Choose a reason for hiding this comment

aborisovich commented Jun 14, 2023 • edited Loading

ceolin commented Jun 20, 2023

aborisovich commented Jun 20, 2023

tmleman commented Jun 21, 2023

aborisovich commented Jun 21, 2023 • edited Loading

aborisovich commented Jun 22, 2023 • edited Loading

aborisovich commented Mar 24, 2023 •

edited

Loading

aborisovich commented Jun 14, 2023 •

edited

Loading

aborisovich commented Jun 21, 2023 •

edited

Loading

aborisovich commented Jun 22, 2023 •

edited

Loading