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

[Takashi Iwai <tiwai@suse.de>]

On Tue, 01 Apr 2025 14:20:14 +0200,
Péter Ujfalusi wrote:
> 
> 
> 
> On 01/04/2025 14:28, Péter Ujfalusi wrote:
> > 
> > 
> > On 31/03/2025 14:09, Takashi Iwai wrote:
> >> 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.
> > 
> > I had hard time to define that flag to act upon for months ;)
> > 
> > It is something of a mix of conditions that needs to be present and
> > _might_ not really need a new flag:
> > 
> > The PCM device must support SNDRV_PCM_INFO_PAUSE and must not support
> > SNDRV_PCM_INFO_RESUME.
> > Given that ALSA core will not trigger the PAUSED streams on suspend,
> > they just moved to SUSPENDED.
> > On resume the RESUME is going to be skipped as well as it is not
> > supported, the PAUSED stream remains SUSPENDED.
> > If the RESUME is supported then the driver will receive the trigger and
> > _might_ be able to move things around to match the paused state it was
> > before suspend.
> > 
> > This patch in a way plays with these rules knowing that on resume the
> > paused stream is going to fail to release and we will go to a new start,
> > so internally it would bluntly stops anything which is paused, they will
> > never going to PAUSE_RELEASE, they will be started w/o the driver's
> > knowledge of a skipped pause release.
> > 
> > What I'm not sure is how this can be done legitimately in core. Move the
> > stream from PAUSED to STOPPED without user space knowing it? So after
> > resume the it thinks that the stream is paused, but the kernel has moved
> > it to stopped?
> > 
> > In SOF we need a bit higher level triggers to have the delicate
> > sequencing right for the BE/FE and for the IPC versions.
> > 
> > I think this is where I thought that this is a bit more complicated than
> > just add a flag and do the trigger.
> 
> Having said all of this, the following small diff works well with SOF
> stack, for aplay this is fine at least ;)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 6c2b6a62d9d2..6d8389642e37 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,14 @@ static int snd_pcm_do_suspend(struct
> snd_pcm_substream *substream,
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	if (runtime->trigger_master != substream)
>  		return 0;
> -	if (! snd_pcm_running(substream))
> -		return 0;
> +	if (! snd_pcm_running(substream)) {
> +		if (runtime->info & SNDRV_PCM_INFO_RESUME ||
> +		    runtime->state != SNDRV_PCM_STATE_PAUSED)
> +			return 0;
> +
> +		/* release the paused stream to suspend */
> +		snd_pcm_pause(substream, false);
> +	}
>  	substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>  	runtime->stop_operating = true;
>  	return 0; /* suspend unconditionally */
> 
> So, if the PCM device does not support resuming, then release the pause
> before going the suspend.

Yeah, I had this kind of change in mind, too.  I thought a bit
different conditional, but the check of SNDRV_PCM_INFO_RESUME bit
should be enough.

But, calling snd_pcm_pause() from that point is dangerous; it's in a
messy loop of linked substream traversals.  I'd call in
snd_pcm_suspend() before the suspend action instead, something like:

--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1731,6 +1731,9 @@ static const struct action_ops snd_pcm_action_suspend = {
 static int snd_pcm_suspend(struct snd_pcm_substream *substream)
 {
 	guard(pcm_stream_lock_irqsave)(substream);
+	if (runtime->state == SNDRV_PCM_STATE_PAUSED &&
+	    !(runtime->info & SNDRV_PCM_INFO_RESUME))
+		snd_pcm_pause(substream, false);
 	return snd_pcm_action(&snd_pcm_action_suspend, substream,
 			      ACTION_ARG_IGNORE);
 }


thanks,

Takashi