S0ix rework by ranj063 · Pull Request #1631 · thesofproject/linux

ranj063 · 2019-12-16T18:08:15Z

This PR replaces #1524

ranj063 · 2019-12-16T23:38:33Z

SOFCI TEST

keyonjie

@ranj063 Thank you for working on this, it looks work to me, my biggest open is that should we put these D0I3 related stuff in SOF generic or better to put them into Intel cAVS platform only.

The idea of scheduling D0I3 delayed work on each IPC sent worth more discussions from stakeholders, concerns from me include how should we handle those received IPC(from FW) and the power state changes during the delayed timeout or concurrent read/write without protection.

ghost · 2019-12-17T17:56:55Z

@keyonjie I have added the mutex now and consolidated the DSP power state transition to one place in hda_dsp_set_power_state(). There should be no other place where we should be handling the DSP state transition other than this. Let me know what you think of this and then we can take it to Pierre

ghost · 2019-12-17T18:54:08Z

@plbossart I think this PR is ready for review now. Keyon and I agree upon everything except the placement of the d0i3_work. While I have it as part of the top-level SOF device, he'd like it to be in the hda device. We could use your advise on what you think.

ghost

Sorry @ranj063 and @keyonjie you are not going to like my feedback. I realize there's a lot of work but I wonder if some of the directions are the right ones.

My main concern is that you chose design patterns that lead to mistakes, such as a 3-value state which you test with an if/else, or overloading state and transitions. if you test state != D3, then the else covers two cases. if you are IN S0, it's different from doing a suspend TO S0 (which really means S0ix which oddly is not even defined)

I am also not sold on the notion of removing the D0 substrates, this is a convenient change on paper that leads to quite a few confusions. If you want to store all the states in one variable, then maybe flags are required.

I really expected more discipline in hardening concepts, this needs a lot more work I am afraid.

ghost · 2019-12-17T22:57:39Z

@plbossart Thanks for your feedback but I have a simple request from you. This series is not easy to review without understanding the entire flow. Some of the patches probably need squashing but I Ieft them separate to make it easier to read. The first few patches only prepare for whats coming later without actually meddling with the flow. So what looks like hard-coded values will be later on changed as and when the flow changes. I could add some comments to maybe be easier to follow. Please stay tuned for a better commented version.

ghost · 2019-12-18T01:04:02Z

@plbossart Thanks for your feedback but I have a simple request from you. This series is not easy to review without understanding the entire flow. Some of the patches probably need squashing but I Ieft them separate to make it easier to read. The first few patches only prepare for whats coming later without actually meddling with the flow. So what looks like hard-coded values will be later on changed as and when the flow changes. I could add some comments to maybe be easier to follow. Please stay tuned for a better commented version.

well that's not a simple request if
a) i need to understand everything before commenting
b) some of the code needs to be squashed and I need to anticipate on your next moves.

ghost · 2019-12-18T02:12:40Z

@plbossart @keyonjie I've update this PR now with the suggested flow changes, name changes and some comments to make it a bit easier to follow. Let me know what you think

ghost · 2019-12-18T03:17:54Z

well that's not a simple request if
a) i need to understand everything before commenting
b) some of the code needs to be squashed and I need to anticipate on your next moves.

@plbossart well unfortunately with this series there are too many fixes/additions/removals. So yes, a bit of overview of the overall flow is needed. I said this before as well, as far as the S0ix implementation goes, with the number of flags and helpers we have, its hard for anyone outside of the 3 of us to tell whats going on.

Anyway, with the latest version of the patches, I've added some FIXME's in places where the changes are made in the subsequent patches. Let me know if this makes better sense

ghost · 2019-12-18T06:12:08Z

@keyonjie please take a look now. I've modified IPC tx to use mod_delayed_work instead of schedule_delayed_work to avoid multiple workqueue items.

I've not added the kernel module parameter yet. I'll add it after discussing it with Pierre.

ghost

The initial code rework could be done differently.
What you want first is to remove the D0i3-specific parts form snd_sof_suspend /snd_sod_resume
That would really unify the code first, and then you can start changing the functionality.
I am not happy that I let this initial code go through, it was a bad design to have this multi-stage decision making. Anyway, the past is the past, let's fix this.

ghost · 2020-01-03T15:11:13Z

@mengdonglin @keyonjie are there any validation results on this PR?

@plbossart We haven't made topic/sof-dev base run on Chrome yet, but we are almost ready to run topic/sof-dev on Ubuntu which S0Ix entry success. We need at least one of these ready to make the validation with S0Ix entry and WoV running to do full test for this PR. Hopefully this will be available next week.

Sorry @keyonjie I don't see the need for S0ix, there was the opportunistic D0i3 support that could be tested... I can't figure out why we are so fixated on S0ix when there is a way to test without it...

ghost · 2020-01-06T03:10:49Z

@plbossart opportunistic D0i3 is one scenario, we have S0ix scenarios also. We have a test case list to verify this PR, in which S0ix WoV ones included.

ghost · 2020-01-06T15:11:18Z

@plbossart opportunistic D0i3 is one scenario, we have S0ix scenarios also. We have a test case list to verify this PR, in which S0ix WoV ones included.

We had two weeks to test this PR. Nothing happened apparently, so I am leaning on merging this PR to force the validation work to happen...

ghost · 2020-01-06T15:24:06Z

@plbossart ack

ghost · 2020-01-07T07:09:21Z

@plbossart opportunistic D0i3 is one scenario, we have S0ix scenarios also. We have a test case list to verify this PR, in which S0ix WoV ones included.

We had two weeks to test this PR. Nothing happened apparently, so I am leaning on merging this PR to force the validation work to happen...

I am fine if you are aligned on that.

ghost

Overall well written and well commented code. Just questions on the D0I3 entry.

ghost · 2020-01-09T04:42:50Z

SOFCI TEST

ghost · 2020-01-09T17:39:55Z

@fredoh9 @xiulipan CI is stuck, can you help?

Unify the suspend/resume routines for both the D0I3/D3 DSP targets in sof_suspend()/sof_resume(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Setting the prepared flag to false marks the streams for the hw_params to be reset upon resuming. In the case of the D0i3-compatible streams that ignored suspend to keep the pipeline active in the DSP during suspend, this should not be done. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add the system_suspend_target field to struct snd_sof_dev to track the intended system suspend power target. This will be used as one of the criteria for determining the final DSP power state. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add a new enum sof_dsp_power_states for all the possible the DSP device states. The SOF driver currently handles only the D0 and D3 states and support for other states will be added later as needed. Also, add a helper to determine the target DSP power state based on the system suspend target. The snd_sof_dsp_d0i3_on_suspend() function is renamed to snd_sof_stream_suspend_ignored() to be more indicative of what it does and it used to determine the target DSP state during system suspend. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The DSP device substates such as D0I0/D0I3 are platform-specific. Therefore, the d0_substate field of struct snd_sof_dev is replaced with the dsp_power_state field which represents the current state of the DSP. This field holds both the device state and the platform-specific substate values. With the DSP device substates being platform-specific, the DSP power state transitions need to be performed in the platform-specific suspend/resume ops as well. In order to achieve this, the ops signature has to be modified to pass the target device state as an argument. The target substate will be determined by the platform-specific ops before performing the transition. For example, in the case of the system suspending to S0IX, the top-level SOF device suspend callback needs to only determine if the DSP will be entering D3 or remain in D0. The target substate in case the device needs to remain in D0 (D0I0 or D0I3) will be determined by the platform-specific suspend op. With the addition of the extended set of power states for the DSP, the set_power_state op for HDA platforms has to be extended to handle only the appropriate state transitions. So, the implementation for the Intel HDA platforms is also modified. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add a helper function to check if only D0i3-compatible streams are active. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Amend the DSP state transition diagram in preparation for introducing the feature to support opportunistic DSP D0I3 state when the system is in S0. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

This patch implements support for DSP D0i3 when the system is in S0. The basic idea is to schedule a delayed work after every successful IPC TX that checks if there are only D0I3-compatible streams active and if so transition the DSP to D0I3. With the introduction of DSP D0I3 in S0, we need to ensure that the DSP is in D0I0 before sending any new IPCs. The exception for this would be the compact IPCs that are used to set the DSP in D0I3/D0I0 states. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…debug Trace DMA is disabled by default when the DSP is in D0I3. Add a debug option to keep trace DMA enabled when the DSP is in D0I3 during S0. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

ghost · 2020-01-10T03:50:20Z

SOFCI TEST

ghost · 2020-01-10T04:46:39Z

Tested on several platforms:

CML Chromebook with rt1011 & rt5682 : No regression found
Up2 Board with PCM512x: No regression found
GLK Chromebook with da7219: No regression found
TGL-U with RT711: No regression found
Dell CML with ALC3204: No regression found

ghost · 2020-01-10T04:48:21Z

ok, let's merge, it's been reviewed to death already.

ranj063 requested review from dbaluta, juimonen, keyonjie, lgirdwood, lyakh and plbossart as code owners December 16, 2019 18:08

ranj063 changed the title ~~S0ix rework~~ S0ix rework[DO NOT REVIEW][TEST ONLY] Dec 16, 2019

ranj063 force-pushed the fix/s0ix_work branch 3 times, most recently from 4055f3e to 32dd5f0 Compare December 16, 2019 22:22

keyonjie reviewed Dec 17, 2019

View reviewed changes

Comment thread sound/soc/sof/pm.c Resolved

Comment thread sound/soc/sof/sof-audio.c Resolved

Comment thread sound/soc/sof/ipc.c Resolved

ranj063 force-pushed the fix/s0ix_work branch from 32dd5f0 to d58d4d6 Compare December 17, 2019 17:52

ranj063 force-pushed the fix/s0ix_work branch 2 times, most recently from 8b2c8d4 to 46152b8 Compare December 17, 2019 18:22

ranj063 changed the title ~~S0ix rework[DO NOT REVIEW][TEST ONLY]~~ S0ix rework Dec 17, 2019

plbossart requested changes Dec 17, 2019

View reviewed changes

ranj063 force-pushed the fix/s0ix_work branch from 46152b8 to cf54b20 Compare December 18, 2019 02:03

keyonjie mentioned this pull request Dec 18, 2019

ASoC: SOF: refine suspend_ignored flag #1524

Closed

ranj063 force-pushed the fix/s0ix_work branch from cf54b20 to eb8a873 Compare December 18, 2019 06:03

ranj063 force-pushed the fix/s0ix_work branch 2 times, most recently from 9cc864c to 5849e10 Compare December 18, 2019 06:58

plbossart requested changes Dec 18, 2019

View reviewed changes

ranj063 force-pushed the fix/s0ix_work branch 2 times, most recently from ff67bc2 to bf00b5e Compare January 7, 2020 17:26

ranj063 mentioned this pull request Jan 7, 2020

Merge/upstream merge 20200107 #1663

Merged

lgirdwood reviewed Jan 7, 2020

View reviewed changes

Comment thread sound/soc/sof/sof-priv.h Resolved

lgirdwood reviewed Jan 7, 2020

View reviewed changes

ranj063 force-pushed the fix/s0ix_work branch from bf00b5e to f0a1e6d Compare January 9, 2020 03:46

xiulipan mentioned this pull request Jan 9, 2020

Bug fix: sof-kernel-log-check DSP reset thesofproject/sof-test#16

Merged

Ranjani Sridharan added 9 commits January 9, 2020 11:45

ASoC: SOF: pm: Unify suspend/resume routines

b9e873d

Unify the suspend/resume routines for both the D0I3/D3 DSP targets in sof_suspend()/sof_resume(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: audio: Add helper to check if only D0i3 streams are active

7cb4a25

Add a helper function to check if only D0i3-compatible streams are active. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: hda: Amend the DSP state transition diagram

f71ad2c

Amend the DSP state transition diagram in preparation for introducing the feature to support opportunistic DSP D0I3 state when the system is in S0. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/s0ix_work branch from f0a1e6d to 0289044 Compare January 9, 2020 19:46

plbossart merged commit ac42b8a into thesofproject:topic/sof-dev Jan 10, 2020

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

Conversation

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

Uh oh!

Uh oh!

ranj063 commented Dec 16, 2019

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 17, 2019

Uh oh!

ghost commented Dec 17, 2019

Uh oh!

ghost left a comment

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!

Uh oh!

Uh oh!

ghost commented Dec 17, 2019

Uh oh!

ghost commented Dec 18, 2019

Uh oh!

ghost commented Dec 18, 2019

Uh oh!

ghost commented Dec 18, 2019

Uh oh!

ghost commented Dec 18, 2019

Uh oh!

ghost left a comment

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!

Uh oh!

Uh oh!

ghost commented Jan 3, 2020

Uh oh!

ghost commented Jan 6, 2020

Uh oh!

ghost commented Jan 6, 2020

Uh oh!

ghost commented Jan 6, 2020

Uh oh!

ghost commented Jan 7, 2020

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 9, 2020

Uh oh!

ghost commented Jan 9, 2020

Uh oh!

ghost commented Jan 10, 2020

Uh oh!

ghost commented Jan 10, 2020 • edited by aiChaoSONG Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 10, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

ranj063 commented Dec 16, 2019 •

edited

Loading

ghost commented Jan 10, 2020 •

edited by aiChaoSONG

Loading