lps: restore secondary cores on wake up #4839

bkokoszx · 2021-10-04T11:39:44Z

Primary core disables all secondary cores in platform_pg_int_handler() function when going to D0ix state. During coming out from D0ix state (D0ix -> D0), we are not able to restore secondary cores properly. This PR add commits that allow us to enable LPS flow to be used in multicore scenario properly. This PR:

adds secondary cores data writeback before disabling them;
adds secondary cores restore during D0->D0ix transition (pm_runtime_get, enable idc interrupt and send power up idc message);
adds separate booting path in secondary_core_init() function i.e. in case when we have basic firmware structures already allocated (i.e. schedulers, notifier etc.), which means we are not during cold boot process (memory has not been powered off), we just register and enable required interrupts.

LPS functionality is limited to the primary core Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com> Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

src/arch/xtensa/lib/cpu.c

src/drivers/intel/cavs/idc.c

src/idc/idc.c

src/schedule/edf_schedule.c

src/init/init.c

lgirdwood · 2021-10-04T12:28:47Z

src/init/init.c

It may be better to have a global context flag to indicate boot type (as we may want to encode more information in it too) .

Good idea but I think we should still somehow validate all critical values in memory to make sure it is still valid to restore

so prior to core (not memory) power off, we really need to unregister our core from all these services, so that it's a clean shutdown and an automatic re-register upn reboot. This may just mean unregistering some lists and invalidating/wb the data,

src/arch/xtensa/lib/cpu.c

src/init/init.c

src/arch/xtensa/include/arch/lib/cpu.h

src/ipc/ipc3/handler.c

src/arch/xtensa/lib/cpu.c

src/drivers/intel/cavs/idc.c

src/platform/intel/cavs/lib/pm_runtime.c

lgirdwood · 2021-10-15T15:27:43Z

@bkokoszx @keyonjie @ranj063 lets have a proper shutdown sequence, this PR should not have to shutdown and reinitialize subsystems in the bringup. Topic for Tuesday call.

keyonjie · 2021-10-19T10:12:18Z

@bkokoszx @keyonjie @ranj063 lets have a proper shutdown sequence, this PR should not have to shutdown and reinitialize subsystems in the bringup. Topic for Tuesday call.

yes, we need to do more tearing down before shutting down a secondary core, I drafted a PR to showcase what I meant in the call here:
#4718

lgirdwood · 2021-10-19T13:16:27Z

@bkokoszx @keyonjie @ranj063 lets have a proper shutdown sequence, this PR should not have to shutdown and reinitialize subsystems in the bringup. Topic for Tuesday call.

yes, we need to do more tearing down before shutting down a secondary core, I drafted a PR to showcase what I meant in the call here: #4718

The PR says "do not review", best to rename this so it can be reviewed.

bkokoszx · 2021-10-21T08:52:49Z

@lgirdwood @keyonjie @abonislawski @lyakh
I've just updated this PR. I've added possibility for idc_free() and schedule_free() to disable only interrupts. I've also added flag for cpu_power_down_core() function i.e. CPU_POWER_DOWN_MEMORY_ON, which indicates not to free memory resources (basically it disables interrupts and perform writeback/invalidate of data) - cpu_power_down_core(CPU_POWER_DOWN_MEMORY_ON) is invoked for secondary cores in platform_wait_for_interrupt() when we should make a preparation before going to d0ix state.

To sum up:

before going to d0ix state secondary cores performs writeback and disables interrupts;
during booting up (after d0ix) secondary cores just enable idc and scheduler interrupts.

lgirdwood

Thanks @bkokoszx, I think this is in the right direction, but still some opens on the clean shutdown flows.

src/drivers/intel/cavs/idc.c

src/idc/idc.c

src/include/sof/schedule/schedule.h

src/schedule/edf_schedule.c

lgirdwood · 2021-10-22T14:15:13Z

src/init/init.c

so prior to core (not memory) power off, we really need to unregister our core from all these services, so that it's a clean shutdown and an automatic re-register upn reboot. This may just mean unregistering some lists and invalidating/wb the data,

lgirdwood · 2021-10-22T14:16:11Z

src/init/init.c

+	err = idc_restore();
+	if (err < 0)
+		return err;
+


We should also be able to call task_restore() and notifier_restore() here too.

I am not sure about that - please look at: #4839 (comment)

ok, so we need to explain why this d0i3 bring up is different in the comments, i.e. each subsystem state so its obvious. Btw, what is the pipeline state on the D0i3 core ? is its stopped and resident in memory OR has it been unloaded with dynamic pipeline ?

I've added some more comments inside secondary_core_restore() function.
The pipeline state is the same as it was before going into D0ix state (memory has not been powered off). So if before going to D0ix we send trigger_stop, it will have STOPPED state.

lgirdwood · 2021-10-22T14:19:38Z

src/drivers/intel/cavs/idc.c

+		/* clear BUSY bits */
+		for (i = 0; i < CONFIG_CORE_COUNT; i++) {
+			idctfc = idc_read(IPC_IDCTFC(i), core);
+			if (idctfc & IPC_IDCTFC_BUSY)


Don't we need to mask all IRQs on this core ? Or do we assume that only IDC IRQs are unmasked ?

I'm not sure whether I understand this comment correctly. I have changed the flow in idc_free function into:

/* clear BUSY bits */ for (i = 0; i < CONFIG_CORE_COUNT; i++) { idctfc = idc_read(IPC_IDCTFC(i), core); if (idctfc & IPC_IDCTFC_BUSY) idc_write(IPC_IDCTFC(i), core, idctfc); } if (flags & IDC_FREE_IRQ_ONLY) return; schedule_task_free(&idc->idc_task); }

lgirdwood · 2021-10-22T14:23:21Z

src/schedule/ll_schedule.c


-	notifier_unregister(sch, NULL,
-			    NOTIFIER_CLK_CHANGE_ID(sch->domain->clk));
+		notifier_unregister(sch, NULL,


We may need to invalidate lists on unregister, please look at the fix @keyonjie did for free() with caches.

@lgirdwood
I think we should not unregister notifier in our scenario.

In our scenario (D0->D0ix, when we have two pipelines: one on primary core and another on secondary core), for DAI component, we register dai_dma_cb (notifier_register(dev, dd->chan, NOTIFIER_ID_DMA_COPY, dai_dma_cb, 0);) on prepare request (in dai_prepare). If we unregister notifier and free its lists before going to D0ix, we will not have dai_dma_cb registered after D0ix->D0 (we do not send prepare after D0ix, just only trigger resume). As a result it will probably cause xrun error, because we will not update dai buffers pointers.

BTW
Regarding @keyonjie 's changes - please look at:
#4718 (comment)

src/arch/xtensa/include/arch/lib/cpu.h

src/ipc/ipc3/handler.c

src/init/init.c

lyakh · 2021-10-26T08:57:26Z

src/init/init.c

+	trace_point(TRACE_BOOT_PLATFORM);
+
+	while (1)
+		wait_for_interrupt(0);


Not sure I understand: shouldn't we end up doing

schedule_task(*task_main_get(), 0, UINT64_MAX);

just like now in secondary_core_init(), then shouldn't this function be a SOF_NORETURN? As well as our main() in fact

In our flow D0ix->D0, task_main is already added to the scheduler list, we do not have to do it one more time (in our case memory has not been powered off), so we only want to end on wait_for_interrupt() - I can add comment to the code.

src/drivers/intel/cavs/idc.c

src/schedule/ll_schedule.c

lyakh · 2021-10-26T09:09:11Z

src/arch/xtensa/lib/cpu.c

+		schedule_free(SOF_SCHEDULER_FREE_IRQ_ONLY);
+
+		/* data writeback/invalidate */
+		dcache_writeback_invalidate_all();


add a comment, that after this point we aren't "polluting" any more cache lines?..

I do not understand what do you mean by "polluting" here?

making clean cache lines dirty (is there an official term for this?). I.e. writing to cached memory, without forcing cache write-back.

lgirdwood

Looks like we are almost ready, to a few opens. It's a complex area so always well worth more comment and trace where we need it.

src/schedule/edf_schedule.c

src/init/init.c

lgirdwood · 2021-10-27T14:19:50Z

src/init/init.c

+	err = idc_restore();
+	if (err < 0)
+		return err;
+


ok, so we need to explain why this d0i3 bring up is different in the comments, i.e. each subsystem state so its obvious. Btw, what is the pipeline state on the D0i3 core ? is its stopped and resident in memory OR has it been unloaded with dynamic pipeline ?

lgirdwood · 2021-11-02T11:30:22Z

@bkokoszx the DMIC CI failures look unrelated for this PM related PR ?

This will add missing secondary cores restore on wake up, simple pm_runtime_get(PM_RUNTIME_DSP, PLATFORM_PRIMARY_CORE_ID) in lps is not enough in multicore scenario Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

Restores idc interrupt if all is ready Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

EDF scheduler restores interrupt Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

In some multicore scenarios dsp can try to restore secondary core state if all memory is still powered up and there was no external request for a core shutdown before For example multicore case with d0ix sleep where secondary core will be put to sleep along with primary core and after wakeup there will be a possibility to restore it Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com> Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

This commit: - adds flags argument to idc_free() function; - adds IDC_FREE_IRQ_ONLY flags, which allows idc_free() to disable only interrupts. Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

This commit: - adds flags argument into scheduler_free function pointer in scheduler_ops struct; - adds SOF_SCHEDULER_FREE_IRQ_ONLY flag, which indicates to disable only interrupts in scheduler_free() functions. Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

Primary core during going to D0i3 state disables all another cores in platform_pg_int_handler() function. Before it secondary cores should be prepared (disable interrupts, perform writeback) in order to make proper restore flow after D0i3->D0 transition. This commit adds cpu_secondary_cores_prepare_d0ix() function in ipc_pm_gate() handler, which sends idc to secondary cores with information that they should perform preparation for power down in the next platform_wait_for_interrupt() invocation. In platform_wait_for_interrupt() there is cpu_power_down_core() invocation with CPU_POWER_DOWN_MEMORY_ON (performs writeback/invalidate, disables interrupts), when proper flag is set. Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

bkokoszx · 2021-11-03T12:09:48Z

@lgirdwood
I am not sure whether TIMEOUT on CML_RVP_SDW in check-suspend-resume-with-playback-5.sh test case is releated. If test just pauses and resumes pipelines without any D0ix flow then it should not be related with above change.
BTW
I have made one little, additional change:

static int ipc_pm_gate(uint32_t header)
{
	struct sof_ipc_pm_gate pm_gate;

	IPC_COPY_CMD(pm_gate, ipc_get()->comp_data);

	tr_info(&ipc_tr, "ipc: pm gate flags 0x%x", pm_gate.flags);

	/* pause dma trace firstly if needed */
	if (pm_gate.flags & SOF_PM_NO_TRACE)
		trace_off();

	if (pm_gate.flags & SOF_PM_PPG) {
		pm_runtime_disable(PM_RUNTIME_DSP, PLATFORM_PRIMARY_CORE_ID);
#if (CONFIG_CAVS_LPS)
		cpu_restore_secondary_cores();
#endif
	} else {
		/* before we enable pm runtime and perform D0->D0ix flow
		 * (primary core powers off secondary cores in
		 * platform_pg_int_handler) we have to prepare all secondary
		 * cores data for powering off (disable interrupt, perform
		 * cache writeback).
		 */
#if (CONFIG_CAVS_LPS)
		cpu_secondary_cores_prepare_d0ix();
#endif
		pm_runtime_enable(PM_RUNTIME_DSP, PLATFORM_PRIMARY_CORE_ID);
	}

	/* resume dma trace if needed */
	if (!(pm_gate.flags & SOF_PM_NO_TRACE))
		trace_on();

	return 0;
}

I invoke cpu_restore_secondary_cores() and cpu_secondary_cores_prepare_d0ix() only in case when LPS (CONFIG_CAVS_LPS) is enabled.

bkokoszx · 2021-11-05T08:01:23Z

@lgirdwood Do you require some other changes?

platform: lps_wait_for_interrupt entry core check

c7a23e2

LPS functionality is limited to the primary core Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com> Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

bkokoszx requested review from dbaluta, lbetlej, lgirdwood, mmaka1, mrajwa and plbossart as code owners October 4, 2021 11:39

bkokoszx mentioned this pull request Oct 4, 2021

[FEATURE] Integrate LPS secondary core fixes on main #4834

Closed

bkokoszx force-pushed the secondary_core_restore branch 5 times, most recently from c201cb4 to 55ade89 Compare October 4, 2021 14:19

lgirdwood reviewed Oct 4, 2021

View reviewed changes

bkokoszx force-pushed the secondary_core_restore branch 4 times, most recently from 10279a9 to d77ec02 Compare October 4, 2021 16:14

lyakh reviewed Oct 5, 2021

View reviewed changes

src/init/init.c Outdated Show resolved Hide resolved

src/arch/xtensa/include/arch/lib/cpu.h Show resolved Hide resolved

src/ipc/ipc3/handler.c Outdated Show resolved Hide resolved

src/ipc/ipc3/handler.c Outdated Show resolved Hide resolved

lgirdwood reviewed Oct 5, 2021

View reviewed changes

src/arch/xtensa/lib/cpu.c Outdated Show resolved Hide resolved

src/drivers/intel/cavs/idc.c Outdated Show resolved Hide resolved

src/platform/intel/cavs/lib/pm_runtime.c Outdated Show resolved Hide resolved

bkokoszx force-pushed the secondary_core_restore branch from d77ec02 to d2369ca Compare October 21, 2021 08:44

bkokoszx requested a review from ranj063 as a code owner October 21, 2021 08:44

lgirdwood reviewed Oct 22, 2021

View reviewed changes

bkokoszx force-pushed the secondary_core_restore branch 2 times, most recently from 3c09599 to 4abda00 Compare October 25, 2021 21:16

lyakh reviewed Oct 26, 2021

View reviewed changes

bkokoszx force-pushed the secondary_core_restore branch 2 times, most recently from 09ad076 to 808b242 Compare October 27, 2021 13:53

lgirdwood reviewed Oct 27, 2021

View reviewed changes

bkokoszx force-pushed the secondary_core_restore branch 11 times, most recently from 4713ee8 to f88674e Compare October 28, 2021 21:12

abonislawski and others added 6 commits November 2, 2021 23:58

lps: restore secondary cores on wake up

0452b85

This will add missing secondary cores restore on wake up, simple pm_runtime_get(PM_RUNTIME_DSP, PLATFORM_PRIMARY_CORE_ID) in lps is not enough in multicore scenario Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

idc: add idc_restore

b507987

Restores idc interrupt if all is ready Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

schedule: add possibility to restore all schedulers

28c23a4

EDF scheduler restores interrupt Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>

idc: allow idc_free() to disable only irqs

cd59a28

This commit: - adds flags argument to idc_free() function; - adds IDC_FREE_IRQ_ONLY flags, which allows idc_free() to disable only interrupts. Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

bkokoszx force-pushed the secondary_core_restore branch 3 times, most recently from 3cb6493 to f78a572 Compare November 3, 2021 09:48

bkokoszx force-pushed the secondary_core_restore branch from f78a572 to 5314d67 Compare November 3, 2021 10:33

lgirdwood approved these changes Nov 8, 2021

View reviewed changes

lgirdwood merged commit ec63f3a into thesofproject:main Nov 8, 2021

lps: restore secondary cores on wake up #4839

lps: restore secondary cores on wake up #4839

Uh oh!

Conversation

bkokoszx commented Oct 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Oct 15, 2021

Uh oh!

keyonjie commented Oct 19, 2021

Uh oh!

lgirdwood commented Oct 19, 2021

Uh oh!

bkokoszx commented Oct 21, 2021

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkokoszx Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

bkokoszx Oct 25, 2021 •

edited

Loading

bkokoszx commented Nov 3, 2021 •

edited

Loading