Skip to content

put audio to D3 if no d0i3 compatible stream opened. #1418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 4 commits into from
Nov 11, 2019
Merged

put audio to D3 if no d0i3 compatible stream opened. #1418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f576241
ASoC: SOF: PM: add check before setting d0_substate
keyonjie Nov 1, 2019
537a5d7
ASoC: SOF: add helper to check if we should enter d0i3 suspend
keyonjie Nov 8, 2019
8765183
ASoC: SOF: PM: only suspend to D0I3 when needed
keyonjie Oct 31, 2019
d7ea746
ASoC: SOF: PM: add state machine to comments
keyonjie Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,19 @@ struct snd_sof_dai *snd_sof_find_dai(struct snd_sof_dev *sdev,
return NULL;
}

bool snd_sof_dsp_d0i3_on_suspend(struct snd_sof_dev *sdev)
{
struct snd_sof_pcm *spcm;

list_for_each_entry(spcm, &sdev->pcm_list, list) {
if (spcm->stream[SNDRV_PCM_STREAM_PLAYBACK].suspend_ignored ||
spcm->stream[SNDRV_PCM_STREAM_CAPTURE].suspend_ignored)
return true;
}

return false;
}

/*
* FW Panic/fault handling.
*/
Expand Down
39 changes: 37 additions & 2 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,9 @@ int snd_sof_set_d0_substate(struct snd_sof_dev *sdev,
{
int ret;

if (sdev->d0_substate == d0_substate)
return 0;

/* do platform specific set_state */
ret = snd_sof_dsp_set_power_state(sdev, d0_substate);
if (ret < 0)
Expand All @@ -428,12 +431,44 @@ int snd_sof_set_d0_substate(struct snd_sof_dev *sdev,
}
EXPORT_SYMBOL(snd_sof_set_d0_substate);

/*
* Audio DSP states may transform as below:-
*
* D0I3 compatible stream
* Runtime +---------------------+ opened only, timeout
* suspend | +--------------------+
* +------------+ D0(active) | |
* | | <---------------+ |
* | +--------> | | |
* | |Runtime +--^--+---------^--+--+ The last | |
* | |resume | | | | opened D0I3 | |
* | | | | | | compatible | |
* | | resume| | | | stream closed | |
* | | from | | D3 | | | |
* | | D3 | |suspend | | d0i3 | |
* | | | | | |suspend | |
* | | | | | | | |
* | | | | | | | |
* +-v---+-----------+--v-------+ | | +------+----v----+
* | | | +-----------> |
* | D3 (suspended) | | | D0I3 +-----+
* | | +--------------+ | |
* | | resume from | | |
* +-------------------^--------+ d0i3 suspend +----------------+ |
* | |
* | D3 suspend |
* +------------------------------------------------+
*
* d0i3_suspend = s0_suspend && D0I3 stream opened,
* D3 suspend = !d0i3_suspend,
*/

@ranj063 ranj063 Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie in my interpretation there are only 3 states for the DSP (D0, D0i3 and D3) So it is a bit confusing when your say D3 runtime-suspended and D3 suspended. Its not quite clear what it really means.

I made a simpler diagram. Let me know if you cant understand my handwriting. I may have missed something but I think this shows the transitions between the DSP states better.
IMG_20191104_161010

@ranj063 ranj063 Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above diagram only applies if the platform supports D0i3. The other thing is this diagram is intel specific. So needs to be moved there

@keyonjie keyonjie Nov 5, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keyonjie in my interpretation there are only 3 states for the DSP (D0, D0i3 and D3) So it is a bit confusing when your say D3 runtime-suspended and D3 suspended. Its not quite clear what it really means.

I made a simpler diagram. Let me know if you cant understand my handwriting. I may have missed something but I think this shows the transitions between the DSP states better.
IMG_20191104_161010

Thanks, basically, I think your diagram is good to describe to DSP state transitions, except that it doesn't include:

  1. the runtime PM ops handling (the left-upper part in my diagram), we can treat the DSP state in runtime-suspended as a sub-state of D0 in your diagram, and don't care about its internal transition?
  2. the difference of S0 D0i3 and S0Ix D0i3. In your diagram, you try to include both in D0<-->D0i3, but it doesn't describe how should we handle when the S0Ix suspend arrive if we are already in S0 D0i3, which is we need to do here in pm.c:snd_sof_suspend/resume(). Yes, again, we can treat both S0-D0i3 and S0Ix-D0i3 as sub-states of D0i3 and ignore its internal transition here.

Then should we draw another diagram to describe the internal transition for '2' and '1'?

@ranj063 ranj063 Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew I missed something and yes the runtime PM case is important. Let me add it and send you a new diagram in a bit

@plbossart plbossart Nov 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with Ranjani, there is no difference between runtime_suspended D3 and system_suspended D3. it's D3.

@keyonjie keyonjie Nov 5, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um, it depends on how do you define the DSP state, e.g. we may put DSP to lower power state D0i3 when the system is in S0ix suspended, comparing with the S0 D0i3, so the flags in PM_GATE IPC will be different for them.

I am fine to use the simpler diagram here to make it easier for understanding, but we need to keep in mind that we still have difference scenarios(or called sub-state) inside both the D3 and D0i3 state, when we are in coding of the detail logic handling there.

@ranj063 ranj063 Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um, it depends on how do you define the DSP state, e.g. we may put DSP to lower power state D0i3 when the system is in S0ix suspended, comparing with the S0 D0i3, so the flags in PM_GATE IPC will be different for them.

@keyonjie Why would it be different in S0? Why cant you put the DSP in the same lower power D0i3 in S0?

@keyonjie keyonjie Nov 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

um, it depends on how do you define the DSP state, e.g. we may put DSP to lower power state D0i3 when the system is in S0ix suspended, comparing with the S0 D0i3, so the flags in PM_GATE IPC will be different for them.

@keyonjie Why would it be different in S0? Why cant you put the DSP in the same lower power D0i3 in S0?

For example, per my observation, in S0 D0i3, access to memory windows is possible, and we are planning to keep DMA trace active in this state for debug purpose, but when the system is entering S0ix, everything is changed, I don't have enough knowledge to explain how PMC will handling to the audio domain on S0ix entering, but I am quite sure the behavior of the Audio DSP in this S0ix D0i3 has difference with the one in S0 D0i3.

And, from the system point of view, we might need more normal interaction between FW & host in S0 D0i3 without putting the DSP back to D0, which is impossible in S0ix D0i3. When we are in S0ix D0i3, the only interaction between DSP and host is that the FW will send an IPC to wake the host up.

@plbossart plbossart Nov 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you have different definitions of D0i3....
You should have one well defined behavior. If you have multiple behaviors, then you are not validating the same things.
I think the memory windows and DMA should only be allowed in D0i3_streaming. @mmaka1 and @lbetlej mentioned this state when we started this, and maybe it's time to formally recognize the difference between usages.

@keyonjie keyonjie Nov 5, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you have different definitions of D0i3....
You should have one well defined behavior. If you have multiple behaviors, then you are not validating the same things.
I think the memory windows and DMA should only be allowed in D0i3_streaming. @mmaka1 and @lbetlej mentioned this state when we started this, and maybe it's time to formally recognize the difference between usages.

Oh yes, that is a major scenario of S0 D0i3, I think.

int snd_sof_resume(struct device *dev)
{
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
int ret;

if (sdev->s0_suspend) {
if (snd_sof_dsp_d0i3_on_suspend(sdev)) {
/* resume from D0I3 */
dev_dbg(sdev->dev, "DSP will exit from D0i3...\n");
ret = snd_sof_set_d0_substate(sdev, SOF_DSP_D0I0);
Expand Down Expand Up @@ -462,7 +497,7 @@ int snd_sof_suspend(struct device *dev)
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
int ret;

if (sdev->s0_suspend) {
if (snd_sof_dsp_d0i3_on_suspend(sdev)) {
/* suspend to D0i3 */
dev_dbg(sdev->dev, "DSP is trying to enter D0i3...\n");
ret = snd_sof_set_d0_substate(sdev, SOF_DSP_D0I3);
Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,8 @@ struct snd_sof_pcm *snd_sof_find_spcm_dai(struct snd_sof_dev *sdev,
return NULL;
}

bool snd_sof_dsp_d0i3_on_suspend(struct snd_sof_dev *sdev);

struct snd_sof_pcm *snd_sof_find_spcm_name(struct snd_sof_dev *sdev,
const char *name);
struct snd_sof_pcm *snd_sof_find_spcm_comp(struct snd_sof_dev *sdev,
Expand Down