Re: [RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[Takashi Iwai <tiwai@suse.de>]

On Mon, 31 Mar 2025 12:56:31 +0200,
Peter Ujfalusi wrote:
> 
> 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 stopped state.
> 
> Explanation of the change:
> 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.
> 
> Link: https://github.com/thesofproject/linux/issues/5035
> Link: https://github.com/thesofproject/linux/issues/5341
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
> Hi,
> 
> Please see the problem statement and details of the issue in the commit
> message.
> 
> I'm not sure if this should be done in ALSA+ASoC level instead. My fear
> is that this is changing how things has been working since almost
> forever and it really puzzles me why it is not affecting other drivers.
> It is true that in SOF the PAUSED state is not equal to STOPED while
> it might be so for other vendors (it is for TI stuff for sure).
> 
> The main point is that when we do a system suspend and a stream is in
> PAUSED state, it will not be triggered (PAUSED == SUSPENDED/STOPPED
> assumption?). On resume, if the platform is not supporting RESUME then
> nothing will be done for the PAUSED stream, but a PAUSE_RELEASE will
> fail and all sorts of state machine assumption will break in SOF/ASoC
> stack.
> 
> I have a PR open for quite long [1] but we would like to find the best
> solution for us and possibly for others facing the same issue as well.
> 
> [1] https://github.com/thesofproject/linux/pull/5058

IMO, this kind of thing should be handled in ALSA core side.
If we want to avoid possible breakage, a flag can be introduced and
perform this conditionally, too.  It'll become a bit complex, but
that's because of the subtle hardware behavior differences,
unfortunately.

AFAIK, dmaengine PCM code assumes that the paused device can be safely
suspended / resumed as is.  There is a special handling to do pause at
snd_dmaengine_pcm_trigger(), for example.

Also, the suspend/resume after pause worked on other legacy devices,
as it seems, too.


thanks,

Takashi

> 
> Thank you,
> Peter
> ---
>  sound/soc/sof/pcm.c      | 76 ++++++++++++++++++++++++++++++++++++++++
>  sound/soc/sof/pm.c       | 11 ++++++
>  sound/soc/sof/sof-priv.h |  2 ++
>  3 files changed, 89 insertions(+)
> 
> diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
> index d584a72e6f52..0b78aa585cd5 100644
> --- a/sound/soc/sof/pcm.c
> +++ b/sound/soc/sof/pcm.c
> @@ -760,6 +760,82 @@ 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) {
> +				spcm_err(spcm, substream->stream,
> +					 "trigger %d failed\n", cmd);
> +				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
> +	 *
> +	 * 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);
> +}
> +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;
> diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
> index 8e3bcf602beb..e257d23d9d19 100644
> --- a/sound/soc/sof/pm.c
> +++ b/sound/soc/sof/pm.c
> @@ -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
> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index abbb5ee7e08c..5750d9038647 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -706,6 +706,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
>   */
> -- 
> 2.49.0
>