Skip to content
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.

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

ASoC: SOF: sof-pcm/pm: On system suspend, move the paused streams to … #5040

Conversation

ujfalusi
Copy link
Collaborator

@ujfalusi ujfalusi commented Jun 5, 2024

…suspended state

Paused streams will not receive a suspend trigger, they will be marked by ALSA core as suspended and it's state is saved.
Since the pause stream is not in a fully stopped state, for example DMA might be still enabled (just the trigger source is removed/disabled) we need to make sure that the hardware is ready to handle the suspend.

This involves a bit more than just stopping a DMA since we also need to communicate with the firmware in a delicate sequence to follow IP programming flows.
To make things a bit more challenging, these flows are different between IPC versions due to the fact that they use different messages to implement the same functionality.

To avoid adding yet another path, callbacks and sequencing for handling the corner case of suspending while a stream is paused, and do this for each IPC versions and platforms, we can move the stream back to running just to put it to suspended state.

In this way we will essentially reduce the suspend cases to two: no audio and running audio (the paused audio becomes a running audio case), cutting down on the probability of misaligned handling of cases.

Link: #5035

…suspended state

Paused streams will not receive a suspend trigger, they will be marked by
ALSA core as suspended and it's state is saved.
Since the pause stream is not in a fully stopped state, for example DMA
might be still enabled (just the trigger source is removed/disabled) we
need to make sure that the hardware is ready to handle the suspend.

This involves a bit more than just stopping a DMA since we also need to
communicate with the firmware in a delicate sequence to follow IP
programming flows.
To make things a bit more challenging, these flows are different between
IPC versions due to the fact that they use different messages to implement
the same functionality.

To avoid adding yet another path, callbacks and sequencing for handling the
corner case of suspending while a stream is paused, and do this for each
IPC versions and platforms, we can move the stream back to running just to
put it to suspended state.

In this way we will essentially reduce the suspend cases to two:
no audio and running audio (the paused audio becomes a running audio case),
cutting down on the probability of misaligned handling of cases.

Link: thesofproject#5035
Signed-off-by: Peter Ujfalusi <[email protected]>
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Patch looks good. I'm still wondering why the existing code to sof_suspend() -> sof_ipc4_tear_down_all_pipelines() -> sof_pcm_stream_free() doesn't cover this. And related, when did this broke down (Fixes tag would be nice).


/*
* Paused streams will not receive a suspend trigger
* but moved to suspended state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: "but are moved to"

@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Jun 5, 2024

Patch looks good. I'm still wondering why the existing code to sof_suspend() -> sof_ipc4_tear_down_all_pipelines() -> sof_pcm_stream_free() doesn't cover this. And related, when did this broke down (Fixes tag would be nice).

It will fail to reset all pipelines as the pause_count (or the started_count) is no zero (lost the old logs), but anyways, currently you will not see that as the device locks up on suspend.
There is a bug in hda_dai_suspend() or not a bug, but the code and flow around it has changed and it is not doing things correctly anymore -> hard lockup.
Doing this for the paused PCM:

			ret = hda_dai_trigger(hext_stream->link_substream,
					      SNDRV_PCM_TRIGGER_SUSPEND,
					      cpu_dai);

can fix the system freeze, but then the IPC messages are failing as we have the imbalance and the pipelines are in wrong state.

@plbossart
Copy link
Member

I am starting to wonder if this shouldn't be a supported functionality in ALSA/ASoC frameworks, rather than requiring work-around in SoundWire and now PCM/HDaudio? The missing trigger seems like a true design miss.

@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Jun 6, 2024

I am starting to wonder if this shouldn't be a supported functionality in ALSA/ASoC frameworks, rather than requiring work-around in SoundWire and now PCM/HDaudio? The missing trigger seems like a true design miss.

The transition from PAUSED to SUSPENDED needs the stream to be taken to RUNNING for some reason for things to actually work, if you look at aplay, if you terminate it while the stream was PAUSED then we will first transition to RUNNING then to STOPPED, but need to look up if there are rules for transitions..

I'm sure if we change the core to send a SUSPEND trigger to a PAUSED stream we will see all sorts of issues with other drivers, so that would need some alignment and care. There must be a reason why it is like this...

@ujfalusi
Copy link
Collaborator Author

Looks like I have found several bugs and likely the issue can be fixed in a more complicated way...
I'll close this PR for now, it can be resurrected if needed.
IMHO, it is a bit better to just avoid the suspend while paused corner case in the first place, but opinions vary.

"%s: SUSPEND failed for %s (id: %d): %d\n",
__func__, substream->name,
spcm->pcm.pcm_id, ret);
return ret;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to have separate runs to first send the PAUSE_RELEASE then in next scan, the SUSPEND to handle the case with multiple paused streams with shared pipeline section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants