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

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

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.

-- 
Péter