[keyonjie]

When there is d0i3_compatible stream opened, mark the d0i3_suspend flag
which will help to put audio to D0I3 state in the coming PM suspend
callback, and clear the flag once the stream are triggered resumed.

[ranj063]

@keyonjie one more flag to add to the list of flags we already for D0i3 support?
How about you iterate through the spcm list and see if any of them have their suspend_ignored flag set and then make the decision to enter S0i3 or D3 accordingly?

[plbossart]

@keyonjie I don't know how many times I have explained that you can be in D0i3 in both S0 and S0ix states. Please don't take shortcuts again and lump S0ix and D0ix together.

[plbossart]

intended S state is S0ix.

[keyonjie]

OK.

[keyonjie]

@keyonjie I don't know how many times I have explained that you can be in D0i3 in both S0 and S0ix states. Please don't take shortcuts again and lump S0ix and D0ix together.

Yes, as explained in the replying to your detail comment, this was in my to-do list, if the conclusion is that we still need support D0ix in S0, then please help shed lights on:

1. what kind of solution do you prefer? I have thought about:
   a. using our existed runtime PM ops for the S0 D0ix enter/exit when the runtime suspend/resume are invoked. This will require changes in soc-pcm.c, I did investigate in the very beginning of the D0ix task and managed it work, but the changing in ASoC core may not be liked by the community.
   b. creating another platform device for this specific task which I posted it PR#1234, that will simplify the logic handling, but someone might not like the idea that creating an extra platform device just for this d0<->d0i3 dynamic transition task.
   c. using ref_count and delayed work to handle this D0<->D0I3 transition, which I posted early in PR [RFC DO NOT MERGE]SOF D0ix support #1154, it can also work, except the concerned about the handling to the cancellation and updating to the delayed work, it could be error-prone there.

2. after multiple ssp and icelake support #1 is chosen, we need clear state machines for both D0(including D0I3) <->D3 and D0<->D0I3, I drew them in the comments inside my commits in the PR#1154 {which actually took quite long time to do that in text only), we need to align on these state machines before doing the real coding.

3. the customization and reconfigurability. It could be customer flavor choice for these kinds of transition, e.g. some customers don't want the D0I3 in S0, some customers want to enter to D0I3 for either S0 suspend or S0Ix suspend to provide a fast audio response (we can get a fast audio resume from a suspending). So, we need to make them configurable, what's your suggestion here?

[ranj063]

@plbossart @keyonjie there are 2 different asks here:

1. Putting the DSP in D3 when the system is entering into S0ix and there are no active pipelines
2. Putting the DSP in D0i3 when the system is in S0

"1" is a minor change in our current flow and easy to implement.

"2" is a bigger ask and of course a mandatory one. I think we already discussed this and we want to go with the delayed work solution that the skylake driver has implemented.

But I don't think we need to tie these two together at all.

[ranj063]

@keyonjie I don't know how many times I have explained that you can be in D0i3 in both S0 and S0ix states. Please don't take shortcuts again and lump S0ix and D0ix together.

Yes, as explained in the replying to your detail comment, this was in my to-do list, if the conclusion is that we still need support D0ix in S0, then please help shed lights on:

1. what kind of solution do you prefer? I have thought about:
   a. using our existed runtime PM ops for the S0 D0ix enter/exit when the runtime suspend/resume are invoked. This will require changes in soc-pcm.c, I did investigate in the very beginning of the D0ix task and managed it work, but the changing in ASoC core may not be liked by the community.
   b. creating another platform device for this specific task which I posted it PR#1234, that will simplify the logic handling, but someone might not like the idea that creating an extra platform device just for this d0<->d0i3 dynamic transition task.
   c. using ref_count and delayed work to handle this D0<->D0I3 transition, which I posted early in PR [RFC DO NOT MERGE]SOF D0ix support #1154, it can also work, except the concerned about the handling to the cancellation and updating to the delayed work, it could be error-prone there.
2. after multiple ssp and icelake support #1 is chosen, we need clear state machines for both D0(including D0I3) <->D3 and D0<->D0I3, I drew them in the comments inside my commits in the PR#1154 {which actually took quite long time to do that in text only), we need to align on these state machines before doing the real coding.
3. the customization and reconfigurability. It could be customer flavor choice for these kinds of transition, e.g. some customers don't want the D0I3 in S0, some customers want to enter to D0I3 for either S0 suspend or S0Ix suspend to provide a fast audio response (we can get a fast audio resume from a suspending). So, we need to make them configurable, what's your suggestion here?

@keyonjie I dont think the latency of resuming from D0i3 will be noticeable enough to disable D0i3 in S0. Its as good or probably lesser than resuming from D3 in the case of runtime PM. I wouldnt worry about making this configurable if you ask me. If the DSP supports D0i3, it will be the only option.

[keyonjie]

@plbossart @keyonjie there are 2 different asks here:

1. Putting the DSP in D3 when the system is entering into S0ix and there are no active pipelines
2. Putting the DSP in D0i3 when the system is in S0

"1" is a minor change in our current flow and easy to implement.

"2" is a bigger ask and of course a mandatory one. I think we already discussed this and we want to go with the delayed work solution that the skylake driver has implemented.

But I don't think we need to tie these two together at all.

Thanks for summarizing @ranj063 Yes, @plbossart this PR only aims to fix "1".

[plbossart]

@keyonjie what I am asking is an infrastructure in place to deal with all possible transitions. It's ok if we don't implement all transitions, e.g. we could do the idle detection last, but I don't want to keep changing the code and adding new flags all the time. We have to show in our upstream contributions we understand what we are doing and the development follows a logical progression. This specific PR is an ad-hoc fix and it doesn't scale to the future cases, so let's do things right please.

[keyonjie]

@keyonjie what I am asking is an infrastructure in place to deal with all possible transitions. It's ok if we don't implement all transitions, e.g. we could do the idle detection last, but I don't want to keep changing the code and adding new flags all the time. We have to show in our upstream contributions we understand what we are doing and the development follows a logical progression. This specific PR is an ad-hoc fix and it doesn't scale to the future cases, so let's do things right please.

Got your point and understood your concern about adding flags now and removing them later, back and forth, let me try to make this PR more clear and scalable for adding runtime D0I3 support.

[keyonjie]

@keyonjie one more flag to add to the list of flags we already for D0i3 support?
How about you iterate through the spcm list and see if any of them have their suspend_ignored flag set and then make the decision to enter S0i3 or D3 accordingly?

@ranj063 that should work also, my point here is that using the flag d0i3_suspend is more straight-forward and efficient, so, with this new added one, we have 3 flags in total:

d0_substate: to indicate if we are in D0 or D0I3 when the system is in S0, this will be useful for the coming D0I3 in S0 support.
s0_suspend: to indicate if the system is entering s2idle or s3, it is needed for us to make decision of stopping d0i3_compatible pipelines or not during the audio suspending.
d0i3_suspend: to indicate if we are going to put audio to d0i3 or d3 during suspending.

Using a separated d0i3_suspend flag will help for the configurable suspend/resume logic in the future, e.g. we could force to put audio DSP to d0i3 (if the platform support d0i3) for suspending always if it is required, then the logic might not be the suspend_ignored flag set there.

[ranj063]

d0_substate: to indicate if we are in D0 or D0I3 when the system is in S0, this will be useful for the coming D0I3 in S0 support.
s0_suspend: to indicate if the system is entering s2idle or s3, it is needed for us to make decision of stopping d0i3_compatible pipelines or not during the audio suspending.
d0i3_suspend: to indicate if we are going to put audio to d0i3 or d3 during suspending.

@keyonjie you forgot "d0i3_compatible".

For you and I, these all make perfect sense. For someone new to the implementation, it can be quite confusing to understand. In my opinion: only 2 flags are mandatory:

1. S0_suspend: to indicate whether the system is entering S0ix or S3
2. d0i3_compatible: to indicate whether the pipeline can be active in D0i3 or not.

Everything else should be handled with appropriate logic.

[plbossart]

1. S0_suspend: to indicate whether the system is entering S0ix or S3

I think we are missing information on whether the platform supports S0ix or not. This is BIOS dependent and the kernel should know about it.

[ranj063]

I think we are missing information on whether the platform supports S0ix or not. This is BIOS dependent and the kernel should know about it.

@plbossart if the platform doesnt support S0ix, then S0_suspend would always be false isnt it?
Are your saying that the kernel should be aware of this beforehand and ignore everything to do with the S0ix entry logic?

[plbossart]

I think we are missing information on whether the platform supports S0ix or not. This is BIOS dependent and the kernel should know about it.

@plbossart if the platform doesnt support S0ix, then S0_suspend would always be false isnt it?
Are your saying that the kernel should be aware of this beforehand and ignore everything to do with the S0ix entry logic?

If the platform does not support S0ix, then we should remain in S0 if there is a D0ix_compatible stream. That means not changing the PCI state and stuff.

[ranj063]

If the platform does not support S0ix, then we should remain in S0 if there is a D0ix_compatible stream. That means not changing the PCI state and stuff.

Ahh I see. If S0ix is not supported, the audio power domain will not be power gated with the DSP in D0i3. This will prevent the S3 entry automatically right?

[plbossart]

If the platform does not support S0ix, then we should remain in S0 if there is a D0ix_compatible stream. That means not changing the PCI state and stuff.

Ahh I see. If S0ix is not supported, the audio power domain will not be power gated with the DSP in D0i3. This will prevent the S3 entry automatically right?

not many double negatives for me...

S0: D0, D0ix, D3
S0ix: D0ix, D3
S3: D3

if S0ix is not supported, we still need to support S0 and S3.

What's misleading is that 'suspend to S0' means different things depending if S0ix is supported or not. We don't have a 'suspend to S0ix', we have to infer it based on platform information when we get the 'suspend to S0' request.

[keyonjie]

d0_substate: to indicate if we are in D0 or D0I3 when the system is in S0, this will be useful for the coming D0I3 in S0 support.
s0_suspend: to indicate if the system is entering s2idle or s3, it is needed for us to make decision of stopping d0i3_compatible pipelines or not during the audio suspending.
d0i3_suspend: to indicate if we are going to put audio to d0i3 or d3 during suspending.

@keyonjie you forgot "d0i3_compatible".

That is for stream, not sdev. for snd_sof_pcm_stream, we have one more actually :-), suspend_ignored.

For you and I, these all make perfect sense. For someone new to the implementation, it can be quite confusing to understand. In my opinion: only 2 flags are mandatory:

1. S0_suspend: to indicate whether the system is entering S0ix or S3
2. d0i3_compatible: to indicate whether the pipeline can be active in D0i3 or not.

If we don't have d0i3, how can people understand d0i3_compatible?

Everything else should be handled with appropriate logic.

[keyonjie]

If the platform does not support S0ix, then we should remain in S0 if there is a D0ix_compatible stream. That means not changing the PCI state and stuff.

Ahh I see. If S0ix is not supported, the audio power domain will not be power gated with the DSP in D0i3. This will prevent the S3 entry automatically right?

not many double negatives for me...

S0: D0, D0ix, D3
S0ix: D0ix, D3
S3: D3

if S0ix is not supported, we still need to support S0 and S3.

These make sense to me.

What's misleading is that 'suspend to S0' means different things depending if S0ix is supported or not. We don't have a 'suspend to S0ix', we have to infer it based on platform information when we get the 'suspend to S0' request.

Sorry, @ranj063 don't catch your means by 'if the platform doesnt support S0ix' and 'don't have a suspend to S0ix', do you mean the case our .suspend() will not be called in some cases on some platforms? or we can't set 'freeze' to /sys/power/state, or can't set 's2idle' to /sys/power/mem_sleep ?

[ranj063]

Sorry, @ranj063 don't catch your means by 'if the platform doesnt support S0ix' and 'don't have a suspend to S0ix', do you mean the case our .suspend() will not be called in some cases on some platforms? or we can't set 'freeze' to /sys/power/state, or can't set 's2idle' to /sys/power/mem_sleep ?

@keyonjie that was Pierre's comment. But basically there can be platforms that only support S0 and S3. In such cases, if there are any D0i3 compatible streams running, we should suspend them and enter S3 when it is requested.

[keyonjie]

Sorry, @ranj063 don't catch your means by 'if the platform doesnt support S0ix' and 'don't have a suspend to S0ix', do you mean the case our .suspend() will not be called in some cases on some platforms? or we can't set 'freeze' to /sys/power/state, or can't set 's2idle' to /sys/power/mem_sleep ?

@keyonjie that was Pierre's comment. But basically there can be platforms that only support S0 and S3. In such cases, if there are any D0i3 compatible streams running, we should suspend them and enter S3 when it is requested.

In this case, the s0_suspend will be false, we will enter D3 with today's code.

[ranj063]

Sorry, @ranj063 don't catch your means by 'if the platform doesnt support S0ix' and 'don't have a suspend to S0ix', do you mean the case our .suspend() will not be called in some cases on some platforms? or we can't set 'freeze' to /sys/power/state, or can't set 's2idle' to /sys/power/mem_sleep ?

@keyonjie that was Pierre's comment. But basically there can be platforms that only support S0 and S3. In such cases, if there are any D0i3 compatible streams running, we should suspend them and enter S3 when it is requested.

In this case, the s0_suspend will be false, we will enter D3 with today's code.

@keyonjie perfect thats what the question was about.

[ranj063]

@keyonjie in your next update for S0ix/D0i3, lets try to draw the state machine to show the transitions that are permitted to make it clearer

[ranj063]

@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.

[ranj063]

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 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.

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]

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]

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

[keyonjie]

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]

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]

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]

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]

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.

[keyonjie]

Hi @ranj063 , how about this diagram?

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

[ranj063]

Hi @ranj063 , how about this diagram?

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

@keyonjie I would not separate D3 and D0i3 (based on runtime PM or system suspend) at all. That gives an impression of them being 2 separate states when in fact there is no difference.

[plbossart]

@keyonjie I would not separate D3 and D0i3 (based on runtime PM or system suspend) at all. That gives an impression of them being 2 separate states when in fact there is no difference.

Not to mention that D0i3 runtime does not exist - the transitions are not handled by pm_runtime but by an idle detection at the device level (orthogonal to pm_runtime).

[plbossart]

@keyonjie can you try and prioritize this, we have a number of QA tests failing because of the S0ix issues and s2idle being broken

[keyonjie]

@keyonjie can you try and prioritize this, we have a number of QA tests failing because of the S0ix issues and s2idle being broken

Sure, let me do this now.

[keyonjie]

@plbossart @ranj063 updated.

[ranj063]

@keyonjie even though sequence you have seems OK, I am a bit uncomfortable with setting the sdev->d0i3_suspend flag every time a D0i3-compatible pipeline executing its TRIGGER suspend callback.

If I were you, I would just check if we're entering S0ix and a D0i3-compatible pipeline is running in the suspend callback and make the decision

[keyonjie]

@ranj063 makes sense, adding a helper to iterate the list, to remove usage of this extra flag now.

[ranj063]

@keyonjie fix commit message:
"Add helper to check if the DSP should be put in D0i3. This function returns true if a stream has ignored
the SUSPEND trigger to keep the pipeline running in the DSP"

[ranj063]

and why call it pcm_D0i3_suspend? I think snd_sof_dsp_D0i3_suspend() makes better sense no?

[dbaluta]

@ranj063 @keyonjie is there a goto document to understand PM states. I must admit I have little experience with this.

[keyonjie]

and why call it pcm_D0i3_suspend? I think snd_sof_dsp_D0i3_suspend() makes better sense no?

The intention was to emphasize that it is from the view of the stream/pcm point as what we were doing inside is iterating the stream list only.

[keyonjie]

@ranj063 @keyonjie is there a goto document to understand PM states. I must admit I have little experience with this.

Oh yes @dbaluta it is in my last commit, we draw a state machine diagram there, see?

[ranj063]

The intention was to emphasize that it is from the view of the stream/pcm point as what we were doing inside is iterating the stream list only.

Right but what you're setting is the state for the DSP and not the pcm. So I dont think pcm in the name is relevant

[keyonjie]

OK, let me change it.

[ranj063]

@keyonjie forgot to change the name here? and below?

[keyonjie]

oops, my bad, shouldn't ignore compiling test for each change.

[ranj063]

looks good now, thanks!

[plbossart]

@keyonjie minor change on the naming needed, see below.
Can you take care of this ASAP, I'd like to merge this so that all suspend/resume tests can be checked with SoundWire overnight.
Thanks!

[plbossart]

the changes look odd.

in the resume() you call a suspend() operation, which means the naming isn't quite right.

snd_sof_dsp_d0i3_used() would make more sense to me.

[keyonjie]

How about snd_sof_is_d0i3_suspend(), @plbossart ?

[plbossart]

it's also confusing to use snd_sof_is_d0i3_suspend() in a resume() routine.

maybe snd_sof_dsp_d0i3_on_suspend() ?

[keyonjie]

it's also confusing to use snd_sof_is_d0i3_suspend() in a suspend() routine.

maybe snd_sof_dsp_d0i3_on_suspend() ?

Sounds good, changing it now.

[plbossart]

thanks @keyonjie