-
Notifications
You must be signed in to change notification settings - Fork 133
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: Stop paused streams before the system suspend #5058
base: topic/sof-dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -689,6 +689,83 @@ static snd_pcm_sframes_t sof_pcm_delay(struct snd_soc_component *component, | |
return 0; | ||
} | ||
|
||
static int sof_pcm_trigger_suspended_paused_streams(struct snd_sof_dev *sdev, | ||
int cmd) | ||
{ | ||
struct snd_pcm_substream *substream; | ||
struct snd_pcm_runtime *runtime; | ||
struct snd_sof_pcm *spcm; | ||
int dir, ret; | ||
|
||
list_for_each_entry(spcm, &sdev->pcm_list, list) { | ||
for_each_pcm_streams(dir) { | ||
substream = spcm->stream[dir].substream; | ||
if (!substream || !substream->runtime) | ||
continue; | ||
|
||
/* | ||
* The stream supports RESUME, it is expected that it | ||
* is handling the corner case of suspending while | ||
* a stream is paused | ||
*/ | ||
runtime = substream->runtime; | ||
if (runtime->info & SNDRV_PCM_INFO_RESUME) | ||
continue; | ||
|
||
/* Only send the trigger to a paused and suspended stream */ | ||
if (runtime->state != SNDRV_PCM_STATE_SUSPENDED || | ||
runtime->suspended_state != SNDRV_PCM_STATE_PAUSED) | ||
continue; | ||
|
||
ret = substream->ops->trigger(substream, cmd); | ||
if (ret) { | ||
dev_err(sdev->dev, | ||
"%s: trigger %d failed for stream %d, dir: %d\n", | ||
__func__, cmd, spcm->pcm.pcm_id, dir); | ||
return ret; | ||
} | ||
} | ||
} | ||
|
||
return 0; | ||
} | ||
|
||
int sof_pcm_stop_paused_on_suspend(struct snd_sof_dev *sdev) | ||
{ | ||
int ret; | ||
|
||
/* | ||
* Handle the corner case of system suspend while at least one stream is | ||
* paused. | ||
* Paused streams will not receive the SUSPEND triggers, they are | ||
* 'silently' moved to SUSPENDED state. | ||
* | ||
* The workaround for the corner case is applicable for streams not | ||
* supporting RESUME. | ||
* | ||
* First we need to move (trigger) the paused streams to RUNNING state, | ||
* then we need to stop them | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this necessary? it's perfectly legal to go from PAUSED to STOPPED. it's not required to go back to RUNNING This is the case for soc-pcm.c: case SNDRV_PCM_TRIGGER_STOP:
if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_START) &&
(be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
goto next; and likewise IPC4 transitions are fine with a PAUSE->STOP. IMHO the transition from PAUSED to RUNNING creates a racy behavior where the state can change and some samples might be generated before stopping again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@plbossart, it might be so in ASoC DPCM, but if a stream is PAUSED then the core would not send a STOP trigger: static int snd_pcm_do_stop(struct snd_pcm_substream *substream,
snd_pcm_state_t state)
{
if (substream->runtime->trigger_master == substream &&
snd_pcm_running(substream)) {
substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
substream->runtime->stop_operating = true;
}
return 0; /* unconditionally stop all substreams */
}
Furthermore, we in sof do not handle well the PAUSE->STOP if we handle it all.
Yes, that might happen. We are talking about a rare corner case. The options is to end up with a broken stack or a possible glitch. |
||
* | ||
* Explanation: Streams moved to SUSPENDED state from PAUSED without | ||
* trigger. If a stream does not support RESUME then on system resume | ||
* the RESUME trigger is not sent, the stream's state and suspended_state | ||
* remains untouched. When the user space releases the pause then the | ||
* core will reject this because the state of the stream is _not_ PAUSED, | ||
* it is still SUSPENDED. | ||
* From this point user space will do the normal (hw_params) prepare and | ||
* START, PAUSE_RELEASE trigger will not be sent by the core after the | ||
* system has resumed. | ||
*/ | ||
ret = sof_pcm_trigger_suspended_paused_streams(sdev, | ||
SNDRV_PCM_TRIGGER_PAUSE_RELEASE); | ||
if (ret) | ||
return ret; | ||
|
||
return sof_pcm_trigger_suspended_paused_streams(sdev, | ||
SNDRV_PCM_TRIGGER_STOP); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it actually important to first release and then to stop all streams or would it also work if you send release + stop to each of them in turn? That would be easier technically (one loop instead of 2) and the window to get some audio in or out for individual streams would get smaller. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
One loop does not work for some reason, if I do that then some refcounting will got crazy and we will end up with all sorts of errors.
Not sure what you mean, I think it is saying that we stop them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@lyakh, the old #5040 implemented a single loop (it was with SUSPEND instead of STOP, but used STOP later) and there were a reason I added this comment: https://github.com/thesofproject/linux/pull/5040/files/358f4ee05c54dfa8fb74b6235ca6d3e08dc947ae#r1634986648 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, sorry, I misunderstood |
||
} | ||
EXPORT_SYMBOL(sof_pcm_stop_paused_on_suspend); | ||
|
||
void snd_sof_new_platform_drv(struct snd_sof_dev *sdev) | ||
{ | ||
struct snd_soc_component_driver *pd = &sdev->plat_drv; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the core still thinks that the stream is in the paused state when in fact we went ahead and stopped it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core thinks that the stream is suspended, the state it was before it got suspended was paused as far as the core knows, but if the SNDRV_PCM_INFO_RESUME is not set then on system resume the stream will stay in this state and on an attempt to un-pause it, it will fail as the stream is not in paused state, it is still in suspended.
Might be a bug in core, but this is how it has been, so it is a feature now.
We don't support RESUME, so after resume the stream must be restarted, this applies to running or paused streams, so to cut down on things, we just stop the paused ones to avoid dealing with different flows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ujfalusi will this handle both the BE and FE pipelines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ujfalusi the other concern I have with this is that when you stop during suspend and then after system resumne, the user releases the pause, the started_count will not be incremented right? what happens during stop after pause/release then? Wont the started_count go below zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user releasing the pause will not going to be propagated down here, it is going to error out in the core because the stream is in SUSPENDED state.
Since the release of pause failed, user space will start the stream again.
This patch handles this correctly as well, counters are all well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not move the state back to PAUSED on resume? It seems like a better idea that building on a design that relies on an error concealment by userspace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are able to do that easily. To move to PAUSED we need to start first right after the resume and since we don't support RESUME we would need someone to call prepare() then trigger:START.
This is what happens if we suspend with active audio as well. prepare() followed by trigger() and this is what happens if we were in PAUSED state: core will reject a PAUSE_RELEASE after the system has resumed, so user space will do a complete restart (or close the PCM and gives up).