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

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

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.

Note: this workaround do have a small drawback due to what it does: it
will release the pause on the stream for a short period, which might or
might not have audible consequence, but that is still better than broken
audio stack.
> AFAIK, dmaengine PCM code assumes that the paused device can be safely
> suspended / resumed as is.

Yes, but that is a bit different as the DMAengine drivers will handle
the suspend independently and save context, with SOF (on Intel/AMD at
least) we don't use DMAengine for HD-DMA...

> There is a special handling to do pause at snd_dmaengine_pcm_trigger(), for example.

Yes, DMAengine have dmaengine_pause(), but that is optional, not all DMA
drivers have it implemented / supported.
Some DMA driver I still maintain the pause is either a stop or a a
genuine pause and on suspend the context is saved to be restored on resume.
> Also, the suspend/resume after pause worked on other legacy devices,
> as it seems, too.

Yep, they do, but we have the DSP and firmware with it's own state and
BE/FE triggering fun with the addition of the difference of sequencing
IPCs and host operations between IPC versions.

I was surprised when I invested time to figure out why this is happening
to be honest.

---
Péter