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: Stop paused streams before the system suspend #5058

Open
wants to merge 1 commit into
base: topic/sof-dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
77 changes: 77 additions & 0 deletions sound/soc/sof/pcm.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Yes

Copy link
Collaborator Author

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?

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.

Copy link
Member

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?

Copy link
Collaborator Author

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?

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

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
Copy link
Member

@plbossart plbossart Jul 16, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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;

@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 */
}

and likewise IPC4 transitions are fine with a PAUSE->STOP.

Furthermore, we in sof do not handle well the PAUSE->STOP if we handle it all.
Yes, I have tested this as the first thing.

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.

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);
Copy link

Choose a reason for hiding this comment

The 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.
Also, doesn't the commit description have to be updated - it's still saying that we just release paused streams

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.
I can re-test it again, but it was failing either with IPC3 or IPC4.

Also, doesn't the commit description have to be updated - it's still saying that we just release paused streams

Not sure what you mean, I think it is saying that we stop them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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. I can re-test it again, but it was failing either with IPC3 or IPC4.

@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

Copy link

Choose a reason for hiding this comment

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

Also, doesn't the commit description have to be updated - it's still saying that we just release paused streams

Not sure what you mean, I think it is saying that we stop them.

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;
Expand Down
11 changes: 11 additions & 0 deletions sound/soc/sof/pm.c
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,17 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
return 0;

/*
* On system suspend we need special handling of paused streams
* For more details, see the comment section in
* sof_pcm_stop_paused_on_suspend)(
*/
if (!runtime_suspend) {
ret = sof_pcm_stop_paused_on_suspend(sdev);
if (ret < 0)
return ret;
}

/* we need to tear down pipelines only if the DSP hardware is
* active, which happens for PCI devices. if the device is
* suspended, it is brought back to full power and then
Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/sof-priv.h
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,8 @@ void snd_sof_complete(struct device *dev);

void snd_sof_new_platform_drv(struct snd_sof_dev *sdev);

int sof_pcm_stop_paused_on_suspend(struct snd_sof_dev *sdev);

/*
* Compress support
*/
Expand Down
Loading