[DO NOT REVIEW YET]PM Support for BYT/CHT + module load/unload fix for BSW #1523

ranj063 · 2019-11-21T01:16:57Z

ranj063 · 2019-11-21T02:23:20Z

@xiulipan @fredoh9 why are all devices failing in this PR and the other PR's?

fredoh9 · 2019-11-21T03:35:27Z

@ranj063 There was a problem in test script. Will fix it now.

plbossart

That looks promising @ranj063, thanks!
the last two patches are a bit odd, maybe a bit more work is needed here. the byt-specific stuff looks ok with minor cosmetic details.

sound/soc/sof/sof-acpi-dev.c

sound/soc/sof/intel/byt.c

sound/soc/sof/ipc.c

plbossart · 2019-11-21T03:38:55Z

sound/soc/sof/pm.c

 	/* do nothing if dsp resume callbacks are not set */
-	if (!sof_ops(sdev)->resume || !sof_ops(sdev)->runtime_resume)
-		return 0;
+	if (runtime_resume)


This one is a bit controversial

I can see that you could support suspend/resume but not their pm_runtime versions.
would the opposite make any sense, support pm_runtime but not system suspend?

I would reword the commit message. This is not a fix, it's allowing for more granularity in what can be supported. this allows pm_runtime to be unsupported while normal resume is supported.
But I don't know if the other way around makes sense.

@plbossart this change makes no assumption about whether runtime PM or system PM is supported. sof_resume() and sof_suspend() are called for both the runtime PM and system PM cases. So all I am doing is if we're runtime suspending, check for the platform-specific runtime-suspend callback and so on for the other cases.
I called it a fix because previously, we didnt check for the appropriate op depending on whether it was runtime callback or system PM callback

@ranj063 when maintainers see a commit message starting with 'fix' they think it's a real problem that needs to be handled on previous -stable versions. What you are doing is 'split checks for platform-specific callbacks', not a real fix.

plbossart · 2019-11-21T03:41:02Z

sound/soc/sof/pm.c

 		if (!sof_ops(sdev)->suspend)
 			return 0;

+	/* remove pipelines */


so this one would be a fix of your previous work, and should be bundled with it rather than the byt pm fixes, right?

@plbossart yes, it should have been there too but it is all the more necessary for BYT/CHT. What's happening with BYT/CHT is that when performing runtime suspend, the DSP doesnt really get powered off and if I dont destroy the pipelines, runtime resume fails because we restore the pipelines in the resume callback.

do you mean to say that on BYT/CHT, the firmware still has all the context and when sending the restore pipeline IPCs you get error messages?

It seems like a case of not sending the IPC if they are not needed, instead of destroying the pipelines and having to recreate them again?

@plbossart that is true. But is it acceptable that on BYT/CHT the DSP is not really going to be in D3 when we runtime suspend and will only be done during system suspend?

@ranj063 whether you destroy the pipelines or not, the DSP will not be power gated on BYT/CHT. That's hardware, nothing we can do. In theory clock gating is still there but the leakage can't be controlled by power switches.

I know that on some Android platforms S0ix was enabled, but there are a ton of dependencies on PMC/BIOS, at this point it's not worth it. The legacy firmware doesn't do anything either for D3.

overall the fix should really be to deal with the topology only at boot and system resume for BYT/CHT, and for other devices in addition for each pm_runtime resume

I know that on some Android platforms S0ix was enabled, but there are a ton of dependencies on PMC/BIOS, at this point it's not worth it. The legacy firmware doesn't do anything either for D3.

@plbossart OK then should we just not bother enabling runtime PM for BYT/CHT at all?

overall the fix should really be to deal with the topology only at boot and system resume for BYT/CHT, and for other devices in addition for each pm_runtime resume

@plbossart what happens when we separate the audio client device from the core? We are going to need to take down the pipelines during suspend and restore them during resume right?

I know that on some Android platforms S0ix was enabled, but there are a ton of dependencies on PMC/BIOS, at this point it's not worth it. The legacy firmware doesn't do anything either for D3.

@plbossart OK then should we just not bother enabling runtime PM for BYT/CHT at all?

there's really not much value indeed. No one will see the difference.

overall the fix should really be to deal with the topology only at boot and system resume for BYT/CHT, and for other devices in addition for each pm_runtime resume

@plbossart what happens when we separate the audio client device from the core? We are going to need to take down the pipelines during suspend and restore them during resume right?

it's a good question - as in I don't know the answer.

we have a similar case with SoundWire where sometimes the parent keeps the context and sometimes it doesn't, and we pass this information to the children. So maybe on resume the child device need to be told that all context was lost and that it needs to recreate the whole shebang.

plbossart · 2019-11-21T03:42:50Z

sound/soc/sof/sof-audio.c

 	return snd_sof_dsp_hw_params_upon_resume(sdev);
 }

+int sof_destroy_pipelines(struct device *dev)


i am still confused why this needs to be done by hand and isn't done by topology and friends?

Good idea. Let me move it to topology.

@plbossart sorry I dont think it can be done by topology. When removing the SOF device, topology can handle this but when dealing with runtime PM, it has be done here.

plbossart · 2019-11-26T00:10:48Z

sound/soc/sof/pm.c


+	/* remove pipelines */
+	sof_destroy_pipelines(dev);
+


i still think this is controversial.

For system suspend-resume, this is not needed. The DSP will lose context and be powered off.

For pm_runtime suspend-resume, this is needed only on Baytrail.

It feels this should really be done conditionally to avoid doing unnecessary things on all platforms where this is useless.

RanderWang · 2019-11-27T08:32:15Z

sound/soc/sof/pm.c

+			return 0;
+	else
+		if (!sof_ops(sdev)->suspend)
+			return 0;


@ranj063 Do we need these checks here ? they are already checked in snd_sof_dsp_suspend and snd_sof_dsp_runtime_suspend

youling257 · 2019-11-28T07:56:07Z

This series patches cause dsp panic.
[ 230.669504] sof-audio-acpi 80860F28:00: error : DSP panic!
[ 230.669529] sof-audio-acpi 80860F28:00: panic: dsp_oops_offset 1329152 offset 1329152
[ 230.669591] sof-audio-acpi 80860F28:00: ipc tx succeeded: 0x40010000
[ 230.669600] sof-audio-acpi 80860F28:00: error: invalid wait state
[ 230.669615] sof-audio-acpi 80860F28:00: error: trace point 00000000
[ 230.669630] sof-audio-acpi 80860F28:00: error: panic at .../drivers/intel/baytrail/ipc.c0:48
[ 230.669643] sof-audio-acpi 80860F28:00: error: DSP Firmware Oops
[ 230.669661] sof-audio-acpi 80860F28:00: EXCCAUSE 0x0000003f EXCVADDR 0x00000000 PS 0x00060525 SAR 0x00000000
[ 230.669678] sof-audio-acpi 80860F28:00: EPC1 0x00000000 EPC2 0x00000000 EPC3 0x00000000 EPC4 0xff2cf783
[ 230.669695] sof-audio-acpi 80860F28:00: EPC5 0xff2cf783 EPC6 0x00000000 EPC7 0x00000000 DEPC 0x00000000
[ 230.669741] sof-audio-acpi 80860F28:00: EPS2 0x00000000 EPS3 0x00000000 EPS4 0x00060120 EPS5 0x00060120
[ 230.669761] sof-audio-acpi 80860F28:00: EPS6 0x00000000 EPS7 0x00000000 INTENABL 0x00000000 INTERRU 0x000000a2
[ 230.669776] sof-audio-acpi 80860F28:00: stack dump from 0xff31c550
[ 230.669797] sof-audio-acpi 80860F28:00: 0xff31c550: ff31c570 00000000 00060525 00060320
[ 230.669817] sof-audio-acpi 80860F28:00: 0xff31c554: 692f6c69 632e6370 00000030 00000000
[ 230.669837] sof-audio-acpi 80860F28:00: 0xff31c558: fea71900 749fe392 00144800 00000000
[ 230.669857] sof-audio-acpi 80860F28:00: 0xff31c55c: 80000800 00000000 7b0d0818 ffff9d89
[ 230.669876] sof-audio-acpi 80860F28:00: 0xff31c560: 7b0e7e00 ffff9d89 7b12ea00 ffff9d89
[ 230.669896] sof-audio-acpi 80860F28:00: 0xff31c564: 9b4c60cc ffffffff 00000004 00000000
[ 230.669915] sof-audio-acpi 80860F28:00: 0xff31c568: 7c29af40 ffff9d89 4005bb28 ffffa1f1
[ 230.669935] sof-audio-acpi 80860F28:00: 0xff31c56c: 7c29af40 ffff9d89 00000000 00000000
[ 230.669953] sof-audio-acpi 80860F28:00: error: ipc host -> DSP: pending no complete no raw 0x0
[ 230.669969] sof-audio-acpi 80860F28:00: error: mask host: pending yes complete yes raw 0xffffffffffffffff
[ 230.669984] sof-audio-acpi 80860F28:00: error: ipc DSP -> host: pending no complete no raw 0x800008000dead00a
[ 230.670000] sof-audio-acpi 80860F28:00: error: mask DSP: pending yes complete yes raw 0xffffffffffffffff

Add remove op that disables interrupts and reset the DSP for BYT and CHT platforms. Signed-off-by: Ranjani Sridharan <[email protected]>

Fix the sequence to enable/disable runtime PM for ACPI devices in the probe and remove callbacks. Signed-off-by: Ranjani Sridharan <[email protected]>

Add the PM callbacks for BYT/CHT platforms. Signed-off-by: Ranjani Sridharan <[email protected]>

We currently ignore the reply messages from the DSP when they are not expected but call it out as an error. Change the error message to a debug message. Signed-off-by: Ranjani Sridharan <[email protected]>

Fix the checks for the platform-specific DSP PM ops in the sof_suspend/sof_resume functions. Signed-off-by: Ranjani Sridharan <[email protected]>

plbossart · 2020-02-20T22:58:04Z

@ranj063 can we update this PR to just have suspend-resume for BYT/CHT (no pm_runtime support) and forget about the load/unload issue with BSW?
Thanks!

youling257 · 2020-04-09T02:52:29Z

cherry-pick these five commit with linux kernel master branch, build failed.

sound/soc/sof/intel/byt.c:822:13: error: initialization of ‘int (*)(struct snd_sof_dev , u32)’ {aka ‘int ()(struct snd_sof_dev , unsigned int)’} from incompatible pointer type ‘int ()(struct snd_sof_dev )’ [-Werror=incompatible-pointer-types]
822 | .suspend = byt_suspend,
| ^~~~~~~~~~~
sound/soc/sof/intel/byt.c:822:13: note: (near initialization for ‘sof_byt_ops.suspend’)
sound/soc/sof/intel/byt.c:903:13: error: initialization of ‘int ()(struct snd_sof_dev , u32)’ {aka ‘int ()(struct snd_sof_dev , unsigned int)’} from incompatible pointer type ‘int ()(struct snd_sof_dev *)’ [-Werror=incompatible-pointer-types]
903 | .suspend = byt_suspend,
| ^~~~~~~~~~~
sound/soc/sof/intel/byt.c:903:13: note: (near initialization for ‘sof_cht_ops.suspend’)
cc1: some warnings being treated as errors

youling257 · 2020-04-09T02:54:43Z

can you fix pm support for BYT/CHT?

eballetbo · 2020-05-11T17:14:00Z

Fixing the above build issue is simply this:

-static int byt_suspend(struct snd_sof_dev *sdev)
+static int byt_suspend(struct snd_sof_dev *sdev, u32 target_state)

Although the patches apply on current mainline (with the above fix), suspend/resume doesn't seem to work:

[ 1299.061192] sof-audio-acpi 808622A8:00: error: ipc timed out for 0x60010000 size 108
[ 1299.061255] sof-audio-acpi 808622A8:00: invalid header size 0x2088002. FW oops is bogus                                                                    
[ 1299.061272] sof-audio-acpi 808622A8:00: error: unexpected fault 0x00000000 trace 0x00000000                                                                
[ 1299.061290] sof-audio-acpi 808622A8:00: error: ipc host -> DSP: pending no complete no raw 0x8000000000000000                                              
[ 1299.061303] sof-audio-acpi 808622A8:00: error: mask host: pending no complete no raw 0x0                                                                   
[ 1299.061315] sof-audio-acpi 808622A8:00: error: ipc DSP -> host: pending no complete no raw 0x0                                                             
[ 1299.061328] sof-audio-acpi 808622A8:00: error: mask DSP: pending no complete no raw 0x0 
[ 1299.061343] sof-audio-acpi 808622A8:00: error: hw params ipc failed for stream 0                                                                           
[ 1299.061358] sof-audio-acpi 808622A8:00: ASoC: 808622A8:00 hw params failed: -110                                                                           
[ 1299.061374]  Low Latency: ASoC: hw_params FE failed -110

plbossart · 2020-05-22T23:02:33Z

replaced by PR #2138

ranj063 requested review from RanderWang, dbaluta, juimonen, keyonjie, lgirdwood, lyakh and plbossart as code owners November 21, 2019 01:16

plbossart requested changes Nov 21, 2019

View reviewed changes

ranj063 force-pushed the fix/acpi_pm branch from 1928e03 to bc6f6a5 Compare November 21, 2019 05:32

This was referenced Nov 21, 2019

[BUG] WFI Panic when processing CTX_SAVE IPC thesofproject/sof#2123

Closed

[RFC]ASoC: SOF: acpi: enable PM for ACPI platforms #1295

Closed

ranj063 force-pushed the fix/acpi_pm branch from bc6f6a5 to 68ded75 Compare November 25, 2019 19:39

plbossart requested changes Nov 26, 2019

View reviewed changes

RanderWang reviewed Nov 27, 2019

View reviewed changes

ranj063 added 5 commits December 3, 2019 15:47

ASoC: SOF: Intel: BYT: add remove op

add60d7

Add remove op that disables interrupts and reset the DSP for BYT and CHT platforms. Signed-off-by: Ranjani Sridharan <[email protected]>

ASoC: SOF: ACPI: fix the runtime enable/disable sequence

8d4dd37

Fix the sequence to enable/disable runtime PM for ACPI devices in the probe and remove callbacks. Signed-off-by: Ranjani Sridharan <[email protected]>

ASoC: SOF: Intel: byt: Add PM callbacks

2ba5e92

Add the PM callbacks for BYT/CHT platforms. Signed-off-by: Ranjani Sridharan <[email protected]>

ASoC: SOF: ipc: ignore DSP replies received when they are not expected

c43a5b5

We currently ignore the reply messages from the DSP when they are not expected but call it out as an error. Change the error message to a debug message. Signed-off-by: Ranjani Sridharan <[email protected]>

ASoC: SOF: pm: Fix checks for platform-specific callbacks

911388b

Fix the checks for the platform-specific DSP PM ops in the sof_suspend/sof_resume functions. Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 force-pushed the fix/acpi_pm branch from 68ded75 to 911388b Compare December 5, 2019 21:29

ranj063 changed the title ~~PM Support for BYT/CHT + module load/unload fix for BSW~~ [DO NOT REVIEW YET]PM Support for BYT/CHT + module load/unload fix for BSW Dec 5, 2019

eballetbo mentioned this pull request May 11, 2020

ASoC: SOF: Intel: byt: Add PM callbacks #2094

Closed

plbossart mentioned this pull request May 22, 2020

Add Baytrail/Cherrytrail suspend/resume #2137

Closed

plbossart closed this May 22, 2020

ranj063 deleted the fix/acpi_pm branch February 13, 2022 05:08

[DO NOT REVIEW YET]PM Support for BYT/CHT + module load/unload fix for BSW #1523

[DO NOT REVIEW YET]PM Support for BYT/CHT + module load/unload fix for BSW #1523

Uh oh!

Conversation

ranj063 commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Nov 21, 2019

Uh oh!

fredoh9 commented Nov 21, 2019

Uh oh!

plbossart 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!

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!

ranj063 Nov 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youling257 commented Nov 28, 2019

Uh oh!

plbossart commented Feb 20, 2020

Uh oh!

youling257 commented Apr 9, 2020

Uh oh!

youling257 commented Apr 9, 2020

Uh oh!

eballetbo commented May 11, 2020

Uh oh!

plbossart commented May 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ranj063 commented Nov 21, 2019 •

edited

Loading

ranj063 Nov 21, 2019 •

edited

Loading