ASoC: SOF: refine suspend_ignored flag

[keyonjie]

ASoC: SOF: refine suspend_ignored flag

Move the flag from snd_sof_pcm_stream to snd_sof_dev, use count to store
the number of d0i3 compatible streams being suspended, and then we don't
need the helper snd_sof_dsp_d0i3_on_suspend() anymore.

[ranj063]

@keyonjie this PR does exactly the same as #1481 but with a different name for the count. But the second commit in my PR goes one step further in actually simplifying the sequence too. Why is this one better ?

[keyonjie]

@keyonjie this PR does exactly the same as #1481 but with a different name for the count. But the

Yes, it came from yours, sorry about missing your authorship here as I split it from the big local change here, correcting it now.

second commit in my PR goes one step further in actually simplifying the sequence too. Why is this one better ?

Yes, your 2nd commit goes further, but I want it to go even further when with S0 D0I3 support enabled, all these are already done on my side, I am refining remained local changes(there might be 5~8 commits) and will submit them soon.

[ranj063]

@keyonjie this PR does exactly the same as #1481 but with a different name for the count. But the

Yes, it came from yours, sorry about missing your authorship here as I split it from the big local change here, correcting it now.

second commit in my PR goes one step further in actually simplifying the sequence too. Why is this one better ?

Yes, your 2nd commit goes further, but I want it to go even further when with S0 D0I3 support enabled, all these are already done on my side, I am refining remained local changes(there might be 5~8 commits) and will submit them soon.

@keyonjie dont worry about authorship. not a big deal, we're the same team. lets put all patches in one PR. It will be hard to see the entire picture if you split them up.

[keyonjie]

@ranj063 @plbossart OK, I will submit S0 D0i3 support commits here together.

[keyonjie]

@ranj063 @plbossart S0 D0I3 is implemented now, please help review.

[ranj063]

@keyonjie this patch is just difficult to understand "ASoC: SOF: refine DSP power states". Care to break it down?

[keyonjie]

sure, let me move these macro definitions to a separated patch.

[ranj063]

the comment makes no sense. Is the DSP in D0 or D3?

[keyonjie]

maybe like this?
/* 1 -- dsp is in D0, 0 -- dsp is in D3 */

[ranj063]

I am not a big fan of bit masks for dsp state. Its not like you could have 2 bits set at the same time. Enum is a lot more intuitive.

[plbossart]

Plus don't invent new definitions. I3 does not exist, it's D0i3.

[keyonjie]

@ranj063 @plbossart let me do this in another way in the coming update version.

[ranj063]

I think you should move this to snd_sof_dsp_suspend() as well.

[ranj063]

please dont change this. let it be scomp->dev

[keyonjie]

oops, I don't know why I changed this, fix it now.

[ranj063]

same here. should be moved to snd_sof_dsp_resume()

[keyonjie]

Yes, actually.

[ranj063]

@keyonjie this looks incorrect. Typically when you remove the SOF device, you resume it and bring it back to full power. So the DSP cant be in D3.

Besides reset is hardly intuitive to convey the message that the DSP is in D3. please change the name.

[keyonjie]

@keyonjie this looks incorrect. Typically when you remove the SOF device, you resume it and bring it back to full power. So the DSP cant be in D3.

Really? Why we need to keep it in full power if it is not used?

Besides reset is hardly intuitive to convey the message that the DSP is in D3. please change the name.

@ranj063 what name do you suggest to use? My understanding "reset to xxx state" is quite common?

[ranj063]

@keyonjie I dont know in my opinion, the mutex isnt required, so just set the power state directly here and remove the init/reset functions

[ranj063]

I am also not able to fully appreciate the need for this mutex, at least just yet while reviewing only the second patch of the series. We only set the power states during suspend/resume and they can never overlap

[keyonjie]

they could be set in IPC sending and PCM open/close also in the subsequent patches.

[ranj063]

@keyonjie that was my other comment, why do we need to do it in PCM open/close, can we not do it just when we send the IPC?

[kv2019i]

@keyonjie This is not a simple lock. In the comment this indicates the lock protects "dsp_power_state", but reading the code, the lock actually covers DSP power state as well. E.g. if you change the DSP power state, you need to hold this lock. Do we really need, or is a lock protecting just the dsp_power_state enough (and if yes, do we need a lock at all)?

[ranj063]

@keyonjie which "SOF specific information" do you need easy access to. Neither the patch nor the commit message make it clear

[ranj063]

"delay D0i3" you mean D0i3 in S0?

[keyonjie]

Yes, let me change the comment.

[ranj063]

Do you really need this here? You can do this after IPC TX no?

[keyonjie]

Here is PCM open, Do you mean doing it after trigger start IPC, in pcm_trigger?

[ranj063]

don't you also need to cancel this work during system suspend?

[keyonjie]

you mean D0I3 suspend? that is already covered by hda_dsp_suspend()->hda_dsp_set_power_state() here:

	case SOF_DSP_STATE_D0I3:
		/* First Cancel any pending attempt to put DSP to D0i3 */
		cancel_delayed_work_sync(&hda->d0i3_work);

		/* disable DMA trace for no delay D0I3 */
		ret = hda_dsp_set_d0i3(sdev, HDA_PM_NO_DMA_TRACE);

[ranj063]

There's too much in common between set D0i0 and set_D0i3. I think we can unify these 2 functions.

[keyonjie]

um, 3 biggest differences that why I split them, the 1st is that we only need delayed work for set D0I3(not for set D0I0), the 2nd one is that we need opposite sequence of IPC sending and register setting, and the 3rd one is that we need vary POWER_GATE IPC flags for D0I3 only.

[ranj063]

is this change really necessary? You'd always send the pm_date_ipc before the DSp is put in D0i3, isnt it?

[keyonjie]

It is needed, without this x_nopm() helpers, it will wake up the DSP to D0I0 before sending the pm_gate_ipc.

[keyonjie]

WoW, looks this will become another 200+ comments PR easily, if I reply them one by one, :).

I prefer to update to a new version firstly next week when I am back to Shanghai.

[plbossart]

WoW, looks this will become another 200+ comments PR easily, if I reply them one by one, :).

I prefer to update to a new version firstly next week when I am back to Shanghai.

I don't mean to sound rude but if you have 200+ comments you probably need to recheck this PR on your own and resubmit when it's mature.

[keyonjie]

WoW, looks this will become another 200+ comments PR easily, if I reply them one by one, :).
I prefer to update to a new version firstly next week when I am back to Shanghai.

I don't mean to sound rude but if you have 200+ comments you probably need to recheck this PR on your own and resubmit when it's mature.

That's true, thanks for reminding.

[keyonjie]

@plbossart @ranj063 Happy thanks giving day!

I think I addressed most of the comments in the updated version, hope you like this version.

I talked with @RanderWang and realize that we might need to change to put Audio DSP to D0I3 for runtime suspend, but I think there is no conflict with what I implemented here, though it might be possible to combine these 2 scenarios with change in ASoC PCM handling(I did this try at the very beginning of my start of the D0Ix task in #1154), some people might not like this solution as it change the component device RT PM logic in soc-dpcm.c when most of their platform actually don't need that extra logic.

[ranj063]

@keyonjie I'm going to leave a long comment about the changes in this commit.

1. The checks for power state inside the set_D0i0/set_D0i3 are a bit of an overkill. In reality, there are only 2 options: enter D0i3 or exit D0i3. Entering is always from D0, you'd never really see the case where you're trying to enter D0i3 from D3. Likewise, exiting D0i3 is only possible if you're already in D0i3. Otherwise, it would be a system resume or a runtime resume.

2. I still do not appreciate the mutex. The commands to enter/exit D0i3 are already protected by waiting for the CIP timeout. Do we really need a mutex for just setting the hda->dsp_power_state? This is not really a register write. Maybe @kv2019i and @plbossart can correct me if I am wrong

3. The bit masks for the power states are also not pretty. There are 3 states, why cant we just simply use an enum instead of setting a D0 bit along with I3 bit? Makes it a lot easier to understand

[keyonjie]

@keyonjie I'm going to leave a long comment about the changes in this commit.

1. The checks for power state inside the set_D0i0/set_D0i3 are a bit of an overkill. In reality, there are only 2 options: enter D0i3 or exit D0i3. Entering is always from D0, you'd never really see the case where you're trying to enter D0i3 from D3. Likewise, exiting D0i3 is only possible if you're already in D0i3. Otherwise, it would be a system resume or a runtime resume.

That's true, but since the enum dsp_power_state can have the D3 value, we only implement the helpers here, we can't make sure the caller will use it correctly always?

Safe check is a compromise, some scenarios might never happen(e.g. D3->D0I3 entry, D3->D0I3 exit), some might happen via error used(e.g. D0I3->D0I3 entry, D0->D0I3 exit).

I preferred to add these check here to avoid critical errors happen or disordering the state(e.g. if we do try doing D3->D0I3), but I am fine if both you and @plbossart think we should remove the D3 check.

2. I still do not appreciate the mutex. The commands to enter/exit D0i3 are already protected by waiting for the CIP timeout. Do we really need a mutex for just setting the hda->dsp_power_state? This is not really a register write. Maybe @kv2019i and @plbossart can correct me if I am wrong

I introduced the mutex as I think we could have race in setting and getting this dsp_power_state, especially considering that the hda_dsp_set_d0i3() could be a delayed work which could run in another thread.

3. The bit masks for the power states are also not pretty. There are 3 states, why cant we just simply use an enum instead of setting a D0 bit along with I3 bit? Makes it a lot easier to understand

The key point here is that sometimes the caller(e.g. after a normal IPC sent) actually don't know which state it should put the DSP to, I introduce a command SOF_DSP_STATE_SET_AUTO here for these scenarios. So the rule here is only using command when calling hda_dsp_set_power_state(), and leave the DSP state change decision to the dsp state management logic(inside hda-dsp.c) only.

enum sof_dsp_state {
	SOF_DSP_STATE_D3 = 0,
	SOF_DSP_STATE_D0 = SOF_DSP_STATE_BIT_D0,
	SOF_DSP_STATE_D0I3 = SOF_DSP_STATE_BIT_D0 | SOF_DSP_STATE_BIT_I3,
};

enum sof_dsp_state_cmd {
	SOF_DSP_STATE_SET_D3 = SOF_DSP_STATE_D3,
	SOF_DSP_STATE_SET_D0 = SOF_DSP_STATE_D0,
	SOF_DSP_STATE_SET_D0I3 = SOF_DSP_STATE_D0I3,
	SOF_DSP_STATE_SET_AUTO = SOF_DSP_STATE_BIT_UPDATE,
};

[plbossart]

@keyonjie @ranj063 we need to look at the bigger picture here, the use of refcounts will not cut it for the general case.

[plbossart]

This direction is not quite right.
It turns out that for SoundWire we do have to keep the DSP powered if we want to avoid complete re-enumeration when exiting clock stop. This could happen in the complete absence of 'D0i3 compatible' streams.
this is really missing an idle loop detection that is based on
a) D0i3-compatible streams being opened
b) no IPC for a while
c) the state of child devices, e.g. if a SoundWire master remains 'active' preventing pm_runtime entry.

[ranj063]

@plbossart I didnt quite understand this ask. The change here only really affects system suspend when there are D0i3-compatible streams running in the DSP. Are you saying that we also need to account for the fact that the SDW master could be active?

[plbossart]

what I am saying is that while doing a system suspend, the device may already be in D0i3 and remain there (S0 target) or move to D3 (S3 target).

The use of refcounts would be quite misleading here, it's only one of the reasons why the device might remain in D0i3.

[ranj063]

@plbossart agreed wrt the WoV usecase this is the only reason that the DSP would remain in D0i3. Maybe we're missing the line of sight for the other possible indicators for this. Can we possibly think of adding this in a separate PR?

[plbossart]

well no. You remove a criterion to replace it by a refcount, we are not going to modify this again later to come back to another criterion. We need to respect the time of reviewers...

[keyonjie]

Hi @plbossart @ranj063 so the state machine could be updated like this:

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

[ranj063]

@keyonjie are you saying that you'd never enter D3 during runtime suspend then>?

[keyonjie]

Yes, @ranj063 enter to D0I3 only for runtime suspend, if the platform doesn't support D0I3, the power gating will still work when it is not used, for power saving.

[plbossart]

NAK, runtime suspend and D0i3 are mutually incompatible.

[keyonjie]

@plbossart can you help guide how the state machine should look like in your brain?

[kv2019i]

Late to the review, but a few specific comments (on new locks and refcounts), please see inline.

[kv2019i]

Hey, I wasn't on the review list, so I'm a bit late to this. I'm not thrilled about adding a refcount, especially as the places in code where it is incremented and decremented, have a lot of additional conditional logic of their own. When/if something goes wrong, it seems painful to debug where the refcount went wrong. With old snd_sof_dsp_d0i3_on_suspend(), you at least have a single place to check which stream is in unexpected state. And a place to add new logic that can keep DSP at D0i3.

[keyonjie]

Hey, I wasn't on the review list, so I'm a bit late to this. I'm not thrilled about adding a refcount, especially as the places in code where it is incremented and decremented, have a lot of additional conditional logic of their own. When/if something goes wrong, it seems painful to debug where the refcount went wrong. With old snd_sof_dsp_d0i3_on_suspend(), you at least have a single place to check which stream is in unexpected state. And a place to add new logic that can keep DSP at D0i3.

Yes, @plbossart also mentioned about this, my opinion is that if the criteria we can foresee is only that "the count of D0I3 compatible streams being suspend > 0", I agree with @ranj063 that using this simple refcount is fine, but if we can foresee that we need to add some other criteria (e.g. for SDW) for D0I3 entry, yes, I agree with you and @plbossart here.

[kv2019i]

@keyonjie This is not a simple lock. In the comment this indicates the lock protects "dsp_power_state", but reading the code, the lock actually covers DSP power state as well. E.g. if you change the DSP power state, you need to hold this lock. Do we really need, or is a lock protecting just the dsp_power_state enough (and if yes, do we need a lock at all)?

[keyonjie]

@keyonjie This is not a simple lock. In the comment this indicates the lock protects "dsp_power_state", but reading the code, the lock actually covers DSP power state as well. E.g. if you change the DSP power state, you need to hold this lock. Do we really need, or is a lock protecting just the dsp_power_state enough (and if yes, do we need a lock at all)?

Yes, the intention is to protect the whold DSP power state change process.

[keyonjie]

Late to the review, but a few specific comments (on new locks and refcounts), please see inline.

Thanks for review @kv2019i, it comes in time we are still in discussion and I just get myself free from other higher priority issues.

[keyonjie]

@plbossart @ranj063 @kv2019i the PR is updated with removing adding the d0i3_suspend_cnt(and keep using the helper snd_sof_dsp_d0i3_on_suspend() which can be extended for SDW later), I keep the usage of the mutex to protect the DSP power state access and the state changing itself as the state change can take long time during the delayed_work, e.g. if we forget to cancel the delayed work before trying to clear/reset the state, this mutex will help to reject this kind of operation.

[plbossart]

@plbossart @ranj063 @kv2019i the PR is updated with removing adding the d0i3_suspend_cnt(and keep using the helper snd_sof_dsp_d0i3_on_suspend() which can be extended for SDW later), I keep the usage of the mutex to protect the DSP power state access and the state changing itself as the state change can take long time during the delayed_work, e.g. if we forget to cancel the delayed work before trying to clear/reset the state, this mutex will help to reject this kind of operation.

@keyonjie I am not going to review 2 PRs for the same things. Please work with @ranj063 and suggest a common solution.

[keyonjie]

@keyonjie I am not going to review 2 PRs for the same things. Please work with @ranj063 and suggest a common solution.

Sorry for the confusion, yes, please hold on for a moment, will let you know once we are aligned.

[keyonjie]

#1631 will be the replacement for this, closing this one.