[PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Peter Ujfalusi]

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.

To fix this issue, release the paused stream before handling the suspend
if the resume is not supported.
This will ensure that the stream is properly suspended for suspend entry.
On resume no attempt will be taken to resume the suspended stream as the
resume is not supported, userspace will eventually going restart the audio.

Link: https://github.com/thesofproject/linux/issues/5035
Link: https://github.com/thesofproject/linux/issues/5341
Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/core/pcm_native.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 6c2b6a62d9d2..a16d15ee98fa 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1730,7 +1730,20 @@ static const struct action_ops snd_pcm_action_suspend = {
  */
 static int snd_pcm_suspend(struct snd_pcm_substream *substream)
 {
+	struct snd_pcm_runtime *runtime = substream->runtime;
+
 	guard(pcm_stream_lock_irqsave)(substream);
+	/*
+	 * Release the paused stream before suspending if resume is not
+	 * supported, because:
+	 * PAUSED streams will not receive the suspend trigger and since the
+	 * driver does not support resuming streams, it is not going to be able
+	 * to handle them properly when the system resumes.
+	 */
+	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);
 }
-- 
2.49.0

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Tue, 01 Apr 2025 15:36:52 +0200,
Peter Ujfalusi wrote:
> 
> 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.
> 
> To fix this issue, release the paused stream before handling the suspend
> if the resume is not supported.
> This will ensure that the stream is properly suspended for suspend entry.
> On resume no attempt will be taken to resume the suspended stream as the
> resume is not supported, userspace will eventually going restart the audio.
> 
> Link: https://github.com/thesofproject/linux/issues/5035
> Link: https://github.com/thesofproject/linux/issues/5341
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Applied now.  Thanks.


Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 01. 04. 25 16:49, Takashi Iwai wrote:
> On Tue, 01 Apr 2025 15:36:52 +0200,
> Peter Ujfalusi wrote:
>>
>> 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.

Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger when resume 
is not supported ? I think that the driver has complete information:

PAUSE -> SUSPEND [-> no RESUME]

The driver should stay in the suspend state until a new stream 
re-initialization is invoked.

It looks like that other logic should be moved to the end driver (if some 
parts must be reinitialized in the suspend phase when the resume is not 
supported). I don't think that this is job for PCM core nor SoC PCM core routines.

The SUSPEND trigger is always invoked, isn't?

>> To fix this issue, release the paused stream before handling the suspend
>> if the resume is not supported.
>> This will ensure that the stream is properly suspended for suspend entry.
>> On resume no attempt will be taken to resume the suspended stream as the
>> resume is not supported, userspace will eventually going restart the audio.
>>
>> Link: https://github.com/thesofproject/linux/issues/5035
>> Link: https://github.com/thesofproject/linux/issues/5341
>> Suggested-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> 
> Applied now.  Thanks.

I would consider revert if my objection is appropriate.

				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Tue, 01 Apr 2025 18:58:40 +0200,
Jaroslav Kysela wrote:
> 
> On 01. 04. 25 16:49, Takashi Iwai wrote:
> > On Tue, 01 Apr 2025 15:36:52 +0200,
> > Peter Ujfalusi wrote:
> >> 
> >> 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.
> 
> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
> when resume is not supported ? I think that the driver has complete
> information:
> 
> PAUSE -> SUSPEND [-> no RESUME]
> 
> The driver should stay in the suspend state until a new stream
> re-initialization is invoked.
> 
> It looks like that other logic should be moved to the end driver (if
> some parts must be reinitialized in the suspend phase when the resume
> is not supported). I don't think that this is job for PCM core nor SoC
> PCM core routines.
> 
> The SUSPEND trigger is always invoked, isn't?

No, that's a part of the problems.  The ops->suspend is called only
for the running state, while at snd_pcm_post_suspend(), the state is
changed to SUSPENDED.  The original state is saved as
suspended_state, and this can be restored via snd_pcm_resume(), but
only if the driver supports the full resume.

We may allow calling ops->suspend also for PAUSED state, too.  But
then each driver would need to have to handle this state change, too.
Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
PAUSED state, the core automatically releases the pause beforehand.

> >> To fix this issue, release the paused stream before handling the suspend
> >> if the resume is not supported.
> >> This will ensure that the stream is properly suspended for suspend entry.
> >> On resume no attempt will be taken to resume the suspended stream as the
> >> resume is not supported, userspace will eventually going restart the audio.
> >> 
> >> Link: https://github.com/thesofproject/linux/issues/5035
> >> Link: https://github.com/thesofproject/linux/issues/5341
> >> Suggested-by: Takashi Iwai <tiwai@suse.de>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> > 
> > Applied now.  Thanks.
> 
> I would consider revert if my objection is appropriate.

OK, I dropped the patch for now until we get agreement.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 01. 04. 25 19:19, Takashi Iwai wrote:
> On Tue, 01 Apr 2025 18:58:40 +0200,
> Jaroslav Kysela wrote:
>>
>> On 01. 04. 25 16:49, Takashi Iwai wrote:
>>> On Tue, 01 Apr 2025 15:36:52 +0200,
>>> Peter Ujfalusi wrote:
>>>>
>>>> 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.
>>
>> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
>> when resume is not supported ? I think that the driver has complete
>> information:
>>
>> PAUSE -> SUSPEND [-> no RESUME]
>>
>> The driver should stay in the suspend state until a new stream
>> re-initialization is invoked.
>>
>> It looks like that other logic should be moved to the end driver (if
>> some parts must be reinitialized in the suspend phase when the resume
>> is not supported). I don't think that this is job for PCM core nor SoC
>> PCM core routines.
>>
>> The SUSPEND trigger is always invoked, isn't?
> 
> No, that's a part of the problems.  The ops->suspend is called only
> for the running state, while at snd_pcm_post_suspend(), the state is
> changed to SUSPENDED.  The original state is saved as
> suspended_state, and this can be restored via snd_pcm_resume(), but
> only if the driver supports the full resume.
> 
> We may allow calling ops->suspend also for PAUSED state, too.  But
> then each driver would need to have to handle this state change, too.
> Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
> PAUSED state, the core automatically releases the pause beforehand.

Thanks for those hints. I don't think that the pause release is sufficient
for this case - the stream must be stopped, too. Actually, all mentioned
prepare/drop/drain routines immediately changes state when the pause is
released.

Maybe a new trigger may be added to notify drivers like:

--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1694,8 +1694,10 @@ 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))
+       if (! snd_pcm_running(substream)) {
+               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
                 return 0;
+       }
         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
         runtime->stop_operating = true;
         return 0; /* suspend unconditionally */

With this, the states won't be changed - the driver without resume support
can shut down all necessary things.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Tue, 01 Apr 2025 20:46:15 +0200,
Jaroslav Kysela wrote:
> 
> On 01. 04. 25 19:19, Takashi Iwai wrote:
> > On Tue, 01 Apr 2025 18:58:40 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 01. 04. 25 16:49, Takashi Iwai wrote:
> >>> On Tue, 01 Apr 2025 15:36:52 +0200,
> >>> Peter Ujfalusi wrote:
> >>>> 
> >>>> 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.
> >> 
> >> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
> >> when resume is not supported ? I think that the driver has complete
> >> information:
> >> 
> >> PAUSE -> SUSPEND [-> no RESUME]
> >> 
> >> The driver should stay in the suspend state until a new stream
> >> re-initialization is invoked.
> >> 
> >> It looks like that other logic should be moved to the end driver (if
> >> some parts must be reinitialized in the suspend phase when the resume
> >> is not supported). I don't think that this is job for PCM core nor SoC
> >> PCM core routines.
> >> 
> >> The SUSPEND trigger is always invoked, isn't?
> > 
> > No, that's a part of the problems.  The ops->suspend is called only
> > for the running state, while at snd_pcm_post_suspend(), the state is
> > changed to SUSPENDED.  The original state is saved as
> > suspended_state, and this can be restored via snd_pcm_resume(), but
> > only if the driver supports the full resume.
> > 
> > We may allow calling ops->suspend also for PAUSED state, too.  But
> > then each driver would need to have to handle this state change, too.
> > Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
> > PAUSED state, the core automatically releases the pause beforehand.
> 
> Thanks for those hints. I don't think that the pause release is sufficient
> for this case - the stream must be stopped, too. Actually, all mentioned
> prepare/drop/drain routines immediately changes state when the pause is
> released.

Actually, this patch follows the same pattern, too.  It calls
snd_pcm_pause(false) to set the state to RUNNING again, then proceed
to the suspend action immediately that sets to SUSPENDED.

> Maybe a new trigger may be added to notify drivers like:
> 
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,10 @@ 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))
> +       if (! snd_pcm_running(substream)) {
> +               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
>                 return 0;
> +       }
>         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>         runtime->stop_operating = true;
>         return 0; /* suspend unconditionally */
> 
> With this, the states won't be changed - the driver without resume support
> can shut down all necessary things.

That's another possibility, yes.  Though, IMO, it makes the
pause-handling a bit more cumbersome.  With Peter's proposal, the
pause state change is always paired, so the driver doesn't consider
about that too much; that's the biggest merit.

But I'm all ears about this issue.  Let's see.

(BTW, I thought the state change PAUSED -> XRUN were possible, but
actually it's not; this is another corner case to be addressed, I
guess.)


Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

Hi,

On 01/04/2025 22:27, Takashi Iwai wrote:
> On Tue, 01 Apr 2025 20:46:15 +0200,
> Jaroslav Kysela wrote:
>>
>> On 01. 04. 25 19:19, Takashi Iwai wrote:
>>> On Tue, 01 Apr 2025 18:58:40 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 01. 04. 25 16:49, Takashi Iwai wrote:
>>>>> On Tue, 01 Apr 2025 15:36:52 +0200,
>>>>> Peter Ujfalusi wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
>>>> when resume is not supported ? I think that the driver has complete
>>>> information:
>>>>
>>>> PAUSE -> SUSPEND [-> no RESUME]
>>>>
>>>> The driver should stay in the suspend state until a new stream
>>>> re-initialization is invoked.
>>>>
>>>> It looks like that other logic should be moved to the end driver (if
>>>> some parts must be reinitialized in the suspend phase when the resume
>>>> is not supported). I don't think that this is job for PCM core nor SoC
>>>> PCM core routines.
>>>>
>>>> The SUSPEND trigger is always invoked, isn't?
>>>
>>> No, that's a part of the problems.  The ops->suspend is called only
>>> for the running state, while at snd_pcm_post_suspend(), the state is
>>> changed to SUSPENDED.  The original state is saved as
>>> suspended_state, and this can be restored via snd_pcm_resume(), but
>>> only if the driver supports the full resume.
>>>
>>> We may allow calling ops->suspend also for PAUSED state, too.  But
>>> then each driver would need to have to handle this state change, too.
>>> Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
>>> PAUSED state, the core automatically releases the pause beforehand.
>>
>> Thanks for those hints. I don't think that the pause release is sufficient
>> for this case - the stream must be stopped, too. Actually, all mentioned
>> prepare/drop/drain routines immediately changes state when the pause is
>> released.
> 
> Actually, this patch follows the same pattern, too.  It calls
> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
> to the suspend action immediately that sets to SUSPENDED.

My original RFC patch did the PAUSE_RELEASE followed by STOP in SOF code
to clear out the paused streams on suspend:
https://lore.kernel.org/linux-sound/20250331105631.7436-1-peter.ujfalusi@linux.intel.com/

Which if I convert it on top of the applied patch would look like this
in core (+ pending comment update):

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index a16d15ee98fa..c1bd993c650c 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1741,8 +1741,10 @@ static int snd_pcm_suspend(struct
snd_pcm_substream *substream)
 	 * to handle them properly when the system resumes.
 	 */
 	if (runtime->state == SNDRV_PCM_STATE_PAUSED &&
-	    !(runtime->info & SNDRV_PCM_INFO_RESUME))
+	    !(runtime->info & SNDRV_PCM_INFO_RESUME)) {
 		snd_pcm_pause(substream, false);
+		return snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
+	}

 	return snd_pcm_action(&snd_pcm_action_suspend, substream,
 			      ACTION_ARG_IGNORE);


But what I like about the current way is that it is bringing the paused
stream handling in line with other triggers as Takashi-san mentioned.
Paused cannot be directly stopped for example, it needs to be released,
then stopped. Drivers don't need special state machines to handle all
permutations of state changes.

>> Maybe a new trigger may be added to notify drivers like:
>>
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -1694,8 +1694,10 @@ 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))
>> +       if (! snd_pcm_running(substream)) {
>> +               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
>>                 return 0;
>> +       }
>>         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>>         runtime->stop_operating = true;
>>         return 0; /* suspend unconditionally */
>>
>> With this, the states won't be changed - the driver without resume support
>> can shut down all necessary things.
> 
> That's another possibility, yes.  Though, IMO, it makes the
> pause-handling a bit more cumbersome.  With Peter's proposal, the
> pause state change is always paired, so the driver doesn't consider
> about that too much; that's the biggest merit.

I agree with this sentiment. If we do something like this then all
drivers (which might have similar issue, a note on this later) needs to
be updated and have some magic done, and in case of SOF that would be:
substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP/SUSPEND);

to handle the messy FE/BE/DSP sequencing.
> But I'm all ears about this issue.  Let's see.
> 
> (BTW, I thought the state change PAUSED -> XRUN were possible, but
> actually it's not; this is another corner case to be addressed, I
> guess.)

Overnight I was thinking of how the suspend while a stream is handled or
can be handled if the PCM supports RESUME.
The paused stream is not a stopped stream and it is not receiving
SUSPEND as it is not running either, but a paused stream might be only
having it's DMA trigger source disabled to stop the hw_ptr moving while
a stopped one have the DMA disabled along with all analog/digital
components.
I'm not sure if sending the SUSPEND trigger to a paused stream is a
solution either as that will surely need different handling as it is not
expected currently (and again SOF have a delicate dance around triggers
starting in ASoC level, but might work).

Note on the pause in general: for few years now pause is in reality a
niche feature and most drivers will never see it used. PA. PW, Chrome,
Android don't use it much or at all, so hitting a suspend while the
stream is paused is a CI thing mostly. mplayer/mpv (kodi might as well)
do pause directly when using ALSA and the PCM supports it, but they work
w/o SNDRV_PCM_INFO_PAUSE just fine.

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 02. 04. 25 8:41, Péter Ujfalusi wrote:

> I'm not sure if sending the SUSPEND trigger to a paused stream is a
> solution either as that will surely need different handling as it is not
> expected currently (and again SOF have a delicate dance around triggers
> starting in ASoC level, but might work).

It's unfortunate, but things should be resolved in the appropriate code levels 
IMHO. The core code should allow all mechanisms.

> Note on the pause in general: for few years now pause is in reality a
> niche feature and most drivers will never see it used. PA. PW, Chrome,
> Android don't use it much or at all, so hitting a suspend while the
> stream is paused is a CI thing mostly. mplayer/mpv (kodi might as well)
> do pause directly when using ALSA and the PCM supports it, but they work
> w/o SNDRV_PCM_INFO_PAUSE just fine.

We are providing API, so we should not judge, if things are widely used. I 
believe that some users use the perfect pause instead full stream restart.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 01. 04. 25 21:27, Takashi Iwai wrote:
> On Tue, 01 Apr 2025 20:46:15 +0200,
> Jaroslav Kysela wrote:
>>
>> On 01. 04. 25 19:19, Takashi Iwai wrote:
>>> On Tue, 01 Apr 2025 18:58:40 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 01. 04. 25 16:49, Takashi Iwai wrote:
>>>>> On Tue, 01 Apr 2025 15:36:52 +0200,
>>>>> Peter Ujfalusi wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
>>>> when resume is not supported ? I think that the driver has complete
>>>> information:
>>>>
>>>> PAUSE -> SUSPEND [-> no RESUME]
>>>>
>>>> The driver should stay in the suspend state until a new stream
>>>> re-initialization is invoked.
>>>>
>>>> It looks like that other logic should be moved to the end driver (if
>>>> some parts must be reinitialized in the suspend phase when the resume
>>>> is not supported). I don't think that this is job for PCM core nor SoC
>>>> PCM core routines.
>>>>
>>>> The SUSPEND trigger is always invoked, isn't?
>>>
>>> No, that's a part of the problems.  The ops->suspend is called only
>>> for the running state, while at snd_pcm_post_suspend(), the state is
>>> changed to SUSPENDED.  The original state is saved as
>>> suspended_state, and this can be restored via snd_pcm_resume(), but
>>> only if the driver supports the full resume.
>>>
>>> We may allow calling ops->suspend also for PAUSED state, too.  But
>>> then each driver would need to have to handle this state change, too.
>>> Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
>>> PAUSED state, the core automatically releases the pause beforehand.
>>
>> Thanks for those hints. I don't think that the pause release is sufficient
>> for this case - the stream must be stopped, too. Actually, all mentioned
>> prepare/drop/drain routines immediately changes state when the pause is
>> released.
> 
> Actually, this patch follows the same pattern, too.  It calls
> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
> to the suspend action immediately that sets to SUSPENDED.

The previous state (suspended_state) will be confusing from the application 
with the proposed patch, because there will be RUNNING state instead PAUSED. 
This previous state is exported via API.

The pause release / stop patterns provide a short time window to process some 
samples which may cause unwanted pops (especially for playback). So perhaps,
we can address this in core (new triggers like PAUSE_RELEASE_AND_STOP), too.

>> Maybe a new trigger may be added to notify drivers like:
>>
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -1694,8 +1694,10 @@ 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))
>> +       if (! snd_pcm_running(substream)) {
>> +               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
>>                  return 0;
>> +       }
>>          substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>>          runtime->stop_operating = true;
>>          return 0; /* suspend unconditionally */
>>
>> With this, the states won't be changed - the driver without resume support
>> can shut down all necessary things.
> 
> That's another possibility, yes.  Though, IMO, it makes the
> pause-handling a bit more cumbersome.  With Peter's proposal, the
> pause state change is always paired, so the driver doesn't consider
> about that too much; that's the biggest merit.

It depends, if we agree that releasing pause is a extra thing to do when we 
know that this step is just to minimize state changes for drivers.

For my view, the drivers may also receive those triggers:

SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE

This will allow to handle all states properly without any side effects (like 
short unwanted DMA transfers).

The drivers should probably activate those triggers conditionally to not break 
current state sequences.

> (BTW, I thought the state change PAUSED -> XRUN were possible, but
> actually it's not; this is another corner case to be addressed, I
 > guess.)

Which code (lines) do you mean?

					Jaroslav



-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 10:09:36 +0200,
Jaroslav Kysela wrote:
> 
> On 01. 04. 25 21:27, Takashi Iwai wrote:
> > On Tue, 01 Apr 2025 20:46:15 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 01. 04. 25 19:19, Takashi Iwai wrote:
> >>> On Tue, 01 Apr 2025 18:58:40 +0200,
> >>> Jaroslav Kysela wrote:
> >>>> 
> >>>> On 01. 04. 25 16:49, Takashi Iwai wrote:
> >>>>> On Tue, 01 Apr 2025 15:36:52 +0200,
> >>>>> Peter Ujfalusi wrote:
> >>>>>> 
> >>>>>> 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.
> >>>> 
> >>>> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
> >>>> when resume is not supported ? I think that the driver has complete
> >>>> information:
> >>>> 
> >>>> PAUSE -> SUSPEND [-> no RESUME]
> >>>> 
> >>>> The driver should stay in the suspend state until a new stream
> >>>> re-initialization is invoked.
> >>>> 
> >>>> It looks like that other logic should be moved to the end driver (if
> >>>> some parts must be reinitialized in the suspend phase when the resume
> >>>> is not supported). I don't think that this is job for PCM core nor SoC
> >>>> PCM core routines.
> >>>> 
> >>>> The SUSPEND trigger is always invoked, isn't?
> >>> 
> >>> No, that's a part of the problems.  The ops->suspend is called only
> >>> for the running state, while at snd_pcm_post_suspend(), the state is
> >>> changed to SUSPENDED.  The original state is saved as
> >>> suspended_state, and this can be restored via snd_pcm_resume(), but
> >>> only if the driver supports the full resume.
> >>> 
> >>> We may allow calling ops->suspend also for PAUSED state, too.  But
> >>> then each driver would need to have to handle this state change, too.
> >>> Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
> >>> PAUSED state, the core automatically releases the pause beforehand.
> >> 
> >> Thanks for those hints. I don't think that the pause release is sufficient
> >> for this case - the stream must be stopped, too. Actually, all mentioned
> >> prepare/drop/drain routines immediately changes state when the pause is
> >> released.
> > 
> > Actually, this patch follows the same pattern, too.  It calls
> > snd_pcm_pause(false) to set the state to RUNNING again, then proceed
> > to the suspend action immediately that sets to SUSPENDED.
> 
> The previous state (suspended_state) will be confusing from the
> application with the proposed patch, because there will be RUNNING
> state instead PAUSED. This previous state is exported via API.

No, it won't happen.  The condition of the new behavior is only when
SNDRV_PCM_INFO_RESUME isn't set -- that is, no resume is possible,
hence no state recovery happens after resume.  From the application
POV, it won't change.

> The pause release / stop patterns provide a short time window to
> process some samples which may cause unwanted pops (especially for
> playback). So perhaps,
> we can address this in core (new triggers like PAUSE_RELEASE_AND_STOP), too.

That's true.  To be noted, the very same thing is applied to other
operations like snd_pcm_drop() or snd_pcm_prepare(), so it's not only
about the case of suspend.

> >> Maybe a new trigger may be added to notify drivers like:
> >> 
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -1694,8 +1694,10 @@ 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))
> >> +       if (! snd_pcm_running(substream)) {
> >> +               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
> >>                  return 0;
> >> +       }
> >>          substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
> >>          runtime->stop_operating = true;
> >>          return 0; /* suspend unconditionally */
> >> 
> >> With this, the states won't be changed - the driver without resume support
> >> can shut down all necessary things.
> > 
> > That's another possibility, yes.  Though, IMO, it makes the
> > pause-handling a bit more cumbersome.  With Peter's proposal, the
> > pause state change is always paired, so the driver doesn't consider
> > about that too much; that's the biggest merit.
> 
> It depends, if we agree that releasing pause is a extra thing to do
> when we know that this step is just to minimize state changes for
> drivers.
> 
> For my view, the drivers may also receive those triggers:
> 
> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
> 
> This will allow to handle all states properly without any side effects
> (like short unwanted DMA transfers).
> 
> The drivers should probably activate those triggers conditionally to
> not break current state sequences.

Hmm, that looks too complex, IMO.

Another slightly more straightforward fix would be just to allow the
direct state transition from PAUSED to any state.  Then the remaining
piece is all about driver implementations.  And, this can be
conditionally operated, e.g. only when some extra flag is set.  When
no flag is set, we keep applying PAUSE_RELEASE before the transition
like now, including Peter's patch, that can work generically (but not
always ideally).

> > (BTW, I thought the state change PAUSED -> XRUN were possible, but
> > actually it's not; this is another corner case to be addressed, I
> > guess.)
> 
> Which code (lines) do you mean?

snd_pcm_xrun() and snd_pcm_stop_xrun().  As far as I read the code,
the state transition won't happen when it's in PAUSED state.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 02. 04. 25 10:39, Takashi Iwai wrote:
> On Wed, 02 Apr 2025 10:09:36 +0200,
> Jaroslav Kysela wrote:
>>
>> On 01. 04. 25 21:27, Takashi Iwai wrote:
>>> On Tue, 01 Apr 2025 20:46:15 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> On 01. 04. 25 19:19, Takashi Iwai wrote:
>>>>> On Tue, 01 Apr 2025 18:58:40 +0200,
>>>>> Jaroslav Kysela wrote:
>>>>>>
>>>>>> On 01. 04. 25 16:49, Takashi Iwai wrote:
>>>>>>> On Tue, 01 Apr 2025 15:36:52 +0200,
>>>>>>> Peter Ujfalusi wrote:
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
>>>>>> when resume is not supported ? I think that the driver has complete
>>>>>> information:
>>>>>>
>>>>>> PAUSE -> SUSPEND [-> no RESUME]
>>>>>>
>>>>>> The driver should stay in the suspend state until a new stream
>>>>>> re-initialization is invoked.
>>>>>>
>>>>>> It looks like that other logic should be moved to the end driver (if
>>>>>> some parts must be reinitialized in the suspend phase when the resume
>>>>>> is not supported). I don't think that this is job for PCM core nor SoC
>>>>>> PCM core routines.
>>>>>>
>>>>>> The SUSPEND trigger is always invoked, isn't?
>>>>>
>>>>> No, that's a part of the problems.  The ops->suspend is called only
>>>>> for the running state, while at snd_pcm_post_suspend(), the state is
>>>>> changed to SUSPENDED.  The original state is saved as
>>>>> suspended_state, and this can be restored via snd_pcm_resume(), but
>>>>> only if the driver supports the full resume.
>>>>>
>>>>> We may allow calling ops->suspend also for PAUSED state, too.  But
>>>>> then each driver would need to have to handle this state change, too.
>>>>> Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
>>>>> PAUSED state, the core automatically releases the pause beforehand.
>>>>
>>>> Thanks for those hints. I don't think that the pause release is sufficient
>>>> for this case - the stream must be stopped, too. Actually, all mentioned
>>>> prepare/drop/drain routines immediately changes state when the pause is
>>>> released.
>>>
>>> Actually, this patch follows the same pattern, too.  It calls
>>> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
>>> to the suspend action immediately that sets to SUSPENDED.
>>
>> The previous state (suspended_state) will be confusing from the
>> application with the proposed patch, because there will be RUNNING
>> state instead PAUSED. This previous state is exported via API.
> 
> No, it won't happen.  The condition of the new behavior is only when
> SNDRV_PCM_INFO_RESUME isn't set -- that is, no resume is possible,
> hence no state recovery happens after resume.  From the application
> POV, it won't change.

The suspended_state can be obtained in snd_pcm_status64(). With Peter's patch, 
there will be RUNNING instead PAUSED, don't?

>> The pause release / stop patterns provide a short time window to
>> process some samples which may cause unwanted pops (especially for
>> playback). So perhaps,
>> we can address this in core (new triggers like PAUSE_RELEASE_AND_STOP), too.
> 
> That's true.  To be noted, the very same thing is applied to other
> operations like snd_pcm_drop() or snd_pcm_prepare(), so it's not only
> about the case of suspend.

Yes, I meant all places.

>>>> Maybe a new trigger may be added to notify drivers like:
>>>>
>>>> --- a/sound/core/pcm_native.c
>>>> +++ b/sound/core/pcm_native.c
>>>> @@ -1694,8 +1694,10 @@ 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))
>>>> +       if (! snd_pcm_running(substream)) {
>>>> +               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
>>>>                   return 0;
>>>> +       }
>>>>           substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>>>>           runtime->stop_operating = true;
>>>>           return 0; /* suspend unconditionally */
>>>>
>>>> With this, the states won't be changed - the driver without resume support
>>>> can shut down all necessary things.
>>>
>>> That's another possibility, yes.  Though, IMO, it makes the
>>> pause-handling a bit more cumbersome.  With Peter's proposal, the
>>> pause state change is always paired, so the driver doesn't consider
>>> about that too much; that's the biggest merit.
>>
>> It depends, if we agree that releasing pause is a extra thing to do
>> when we know that this step is just to minimize state changes for
>> drivers.
>>
>> For my view, the drivers may also receive those triggers:
>>
>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
>> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
>> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
>>
>> This will allow to handle all states properly without any side effects
>> (like short unwanted DMA transfers).
>>
>> The drivers should probably activate those triggers conditionally to
>> not break current state sequences.
> 
> Hmm, that looks too complex, IMO.
> 
> Another slightly more straightforward fix would be just to allow the
> direct state transition from PAUSED to any state.  Then the remaining
> piece is all about driver implementations.  And, this can be
> conditionally operated, e.g. only when some extra flag is set.  When
> no flag is set, we keep applying PAUSE_RELEASE before the transition
> like now, including Peter's patch, that can work generically (but not
> always ideally).

It looks also feasible. My proposal just allows the straight state 
identification from the trigger value in the drivers.

>>> (BTW, I thought the state change PAUSED -> XRUN were possible, but
>>> actually it's not; this is another corner case to be addressed, I
>>> guess.)
>>
>> Which code (lines) do you mean?
> 
> snd_pcm_xrun() and snd_pcm_stop_xrun().  As far as I read the code,
> the state transition won't happen when it's in PAUSED state.

Yes, paused state should be handled there, too. But the pause release 
operation looks really suspicious for xrun.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 10:52:16 +0200,
Jaroslav Kysela wrote:
> 
> On 02. 04. 25 10:39, Takashi Iwai wrote:
> > On Wed, 02 Apr 2025 10:09:36 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 01. 04. 25 21:27, Takashi Iwai wrote:
> >>> On Tue, 01 Apr 2025 20:46:15 +0200,
> >>> Jaroslav Kysela wrote:
> >>>> 
> >>>> On 01. 04. 25 19:19, Takashi Iwai wrote:
> >>>>> On Tue, 01 Apr 2025 18:58:40 +0200,
> >>>>> Jaroslav Kysela wrote:
> >>>>>> 
> >>>>>> On 01. 04. 25 16:49, Takashi Iwai wrote:
> >>>>>>> On Tue, 01 Apr 2025 15:36:52 +0200,
> >>>>>>> Peter Ujfalusi wrote:
> >>>>>>>> 
> >>>>>>>> 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.
> >>>>>> 
> >>>>>> Why you expect to see PAUSE_RELEASE trigger before SUSPEND trigger
> >>>>>> when resume is not supported ? I think that the driver has complete
> >>>>>> information:
> >>>>>> 
> >>>>>> PAUSE -> SUSPEND [-> no RESUME]
> >>>>>> 
> >>>>>> The driver should stay in the suspend state until a new stream
> >>>>>> re-initialization is invoked.
> >>>>>> 
> >>>>>> It looks like that other logic should be moved to the end driver (if
> >>>>>> some parts must be reinitialized in the suspend phase when the resume
> >>>>>> is not supported). I don't think that this is job for PCM core nor SoC
> >>>>>> PCM core routines.
> >>>>>> 
> >>>>>> The SUSPEND trigger is always invoked, isn't?
> >>>>> 
> >>>>> No, that's a part of the problems.  The ops->suspend is called only
> >>>>> for the running state, while at snd_pcm_post_suspend(), the state is
> >>>>> changed to SUSPENDED.  The original state is saved as
> >>>>> suspended_state, and this can be restored via snd_pcm_resume(), but
> >>>>> only if the driver supports the full resume.
> >>>>> 
> >>>>> We may allow calling ops->suspend also for PAUSED state, too.  But
> >>>>> then each driver would need to have to handle this state change, too.
> >>>>> Currently, when snd_pcm_prepare(), *_drop() or *_drain() is called at
> >>>>> PAUSED state, the core automatically releases the pause beforehand.
> >>>> 
> >>>> Thanks for those hints. I don't think that the pause release is sufficient
> >>>> for this case - the stream must be stopped, too. Actually, all mentioned
> >>>> prepare/drop/drain routines immediately changes state when the pause is
> >>>> released.
> >>> 
> >>> Actually, this patch follows the same pattern, too.  It calls
> >>> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
> >>> to the suspend action immediately that sets to SUSPENDED.
> >> 
> >> The previous state (suspended_state) will be confusing from the
> >> application with the proposed patch, because there will be RUNNING
> >> state instead PAUSED. This previous state is exported via API.
> > 
> > No, it won't happen.  The condition of the new behavior is only when
> > SNDRV_PCM_INFO_RESUME isn't set -- that is, no resume is possible,
> > hence no state recovery happens after resume.  From the application
> > POV, it won't change.
> 
> The suspended_state can be obtained in snd_pcm_status64(). With
> Peter's patch, there will be RUNNING instead PAUSED, don't?

I see, that should be better fixed.

> >> The pause release / stop patterns provide a short time window to
> >> process some samples which may cause unwanted pops (especially for
> >> playback). So perhaps,
> >> we can address this in core (new triggers like PAUSE_RELEASE_AND_STOP), too.
> > 
> > That's true.  To be noted, the very same thing is applied to other
> > operations like snd_pcm_drop() or snd_pcm_prepare(), so it's not only
> > about the case of suspend.
> 
> Yes, I meant all places.
> 
> >>>> Maybe a new trigger may be added to notify drivers like:
> >>>> 
> >>>> --- a/sound/core/pcm_native.c
> >>>> +++ b/sound/core/pcm_native.c
> >>>> @@ -1694,8 +1694,10 @@ 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))
> >>>> +       if (! snd_pcm_running(substream)) {
> >>>> +               substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND_WHEN_IDLE);
> >>>>                   return 0;
> >>>> +       }
> >>>>           substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
> >>>>           runtime->stop_operating = true;
> >>>>           return 0; /* suspend unconditionally */
> >>>> 
> >>>> With this, the states won't be changed - the driver without resume support
> >>>> can shut down all necessary things.
> >>> 
> >>> That's another possibility, yes.  Though, IMO, it makes the
> >>> pause-handling a bit more cumbersome.  With Peter's proposal, the
> >>> pause state change is always paired, so the driver doesn't consider
> >>> about that too much; that's the biggest merit.
> >> 
> >> It depends, if we agree that releasing pause is a extra thing to do
> >> when we know that this step is just to minimize state changes for
> >> drivers.
> >> 
> >> For my view, the drivers may also receive those triggers:
> >> 
> >> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
> >> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
> >> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
> >> 
> >> This will allow to handle all states properly without any side effects
> >> (like short unwanted DMA transfers).
> >> 
> >> The drivers should probably activate those triggers conditionally to
> >> not break current state sequences.
> > 
> > Hmm, that looks too complex, IMO.
> > 
> > Another slightly more straightforward fix would be just to allow the
> > direct state transition from PAUSED to any state.  Then the remaining
> > piece is all about driver implementations.  And, this can be
> > conditionally operated, e.g. only when some extra flag is set.  When
> > no flag is set, we keep applying PAUSE_RELEASE before the transition
> > like now, including Peter's patch, that can work generically (but not
> > always ideally).
> 
> It looks also feasible. My proposal just allows the straight state
> identification from the trigger value in the drivers.

On the second thought, yet another variant would be to introduce only
one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
PAUSED state as a step before stopping the stream.  This can be
applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
If the driver doesn't support this mode, the PCM core may fall back to
SNDRV_PCM_TRIGGER_PAUSE_RELEASE.

> >>> (BTW, I thought the state change PAUSED -> XRUN were possible, but
> >>> actually it's not; this is another corner case to be addressed, I
> >>> guess.)
> >> 
> >> Which code (lines) do you mean?
> > 
> > snd_pcm_xrun() and snd_pcm_stop_xrun().  As far as I read the code,
> > the state transition won't happen when it's in PAUSED state.
> 
> Yes, paused state should be handled there, too. But the pause release
> operation looks really suspicious for xrun.

Yeah, I noticed it after sending out my previous post, too.
The engine is stopped, so practically the XRUN must not happen.
I believe we can forget about that.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 02. 04. 25 11:16, Takashi Iwai wrote:

>>>> For my view, the drivers may also receive those triggers:
>>>>
>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
>>>> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
>>>> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
>>>>
>>>> This will allow to handle all states properly without any side effects
>>>> (like short unwanted DMA transfers).
>>>>
>>>> The drivers should probably activate those triggers conditionally to
>>>> not break current state sequences.
>>>
>>> Hmm, that looks too complex, IMO.
>>>
>>> Another slightly more straightforward fix would be just to allow the
>>> direct state transition from PAUSED to any state.  Then the remaining
>>> piece is all about driver implementations.  And, this can be
>>> conditionally operated, e.g. only when some extra flag is set.  When
>>> no flag is set, we keep applying PAUSE_RELEASE before the transition
>>> like now, including Peter's patch, that can work generically (but not
>>> always ideally).
>>
>> It looks also feasible. My proposal just allows the straight state
>> identification from the trigger value in the drivers.
> 
> On the second thought, yet another variant would be to introduce only
> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
> PAUSED state as a step before stopping the stream.  This can be
> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
> If the driver doesn't support this mode, the PCM core may fall back to
> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.

This looks identical to my proposed SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP 
which is more explanatory. It's not clear what PAUSE_RESET should do (probably 
the stream should be kept inactive).

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 12:45:36 +0200,
Jaroslav Kysela wrote:
> 
> On 02. 04. 25 11:16, Takashi Iwai wrote:
> 
> >>>> For my view, the drivers may also receive those triggers:
> >>>> 
> >>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
> >>>> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
> >>>> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
> >>>> 
> >>>> This will allow to handle all states properly without any side effects
> >>>> (like short unwanted DMA transfers).
> >>>> 
> >>>> The drivers should probably activate those triggers conditionally to
> >>>> not break current state sequences.
> >>> 
> >>> Hmm, that looks too complex, IMO.
> >>> 
> >>> Another slightly more straightforward fix would be just to allow the
> >>> direct state transition from PAUSED to any state.  Then the remaining
> >>> piece is all about driver implementations.  And, this can be
> >>> conditionally operated, e.g. only when some extra flag is set.  When
> >>> no flag is set, we keep applying PAUSE_RELEASE before the transition
> >>> like now, including Peter's patch, that can work generically (but not
> >>> always ideally).
> >> 
> >> It looks also feasible. My proposal just allows the straight state
> >> identification from the trigger value in the drivers.
> > 
> > On the second thought, yet another variant would be to introduce only
> > one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
> > PAUSED state as a step before stopping the stream.  This can be
> > applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
> > If the driver doesn't support this mode, the PCM core may fall back to
> > SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
> 
> This looks identical to my proposed
> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
> explanatory. It's not clear what PAUSE_RESET should do (probably the
> stream should be kept inactive).

There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
doesn't have to stop the stream by itself, but it's only a preparation
for stopping.  That is, a proper TRIGGER_STOP always follows after
TRIGGER_PAUSE_RESET.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 02/04/2025 14:21, Takashi Iwai wrote:
>>> On the second thought, yet another variant would be to introduce only
>>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
>>> PAUSED state as a step before stopping the stream.  This can be
>>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
>>> If the driver doesn't support this mode, the PCM core may fall back to
>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
>>
>> This looks identical to my proposed
>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
>> explanatory. It's not clear what PAUSE_RESET should do (probably the
>> stream should be kept inactive).
> 
> There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
> doesn't have to stop the stream by itself, but it's only a preparation
> for stopping.  That is, a proper TRIGGER_STOP always follows after
> TRIGGER_PAUSE_RESET.

But what state that we move after we reset the PAUSE? is it RUNNING
(iow, equals to PAUSE_RELEASE) or something else?

In case of suspend while the state is paused we will:
TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?

Most drivers are doing the same thing for STOP and SUSPEND trigger,
however it is possible to do extra steps for SUSPEND to make sure that
the whole system is in a lowest power state.

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 13:43:57 +0200,
Péter Ujfalusi wrote:
> 
> 
> 
> On 02/04/2025 14:21, Takashi Iwai wrote:
> >>> On the second thought, yet another variant would be to introduce only
> >>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
> >>> PAUSED state as a step before stopping the stream.  This can be
> >>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
> >>> If the driver doesn't support this mode, the PCM core may fall back to
> >>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
> >>
> >> This looks identical to my proposed
> >> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
> >> explanatory. It's not clear what PAUSE_RESET should do (probably the
> >> stream should be kept inactive).
> > 
> > There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
> > doesn't have to stop the stream by itself, but it's only a preparation
> > for stopping.  That is, a proper TRIGGER_STOP always follows after
> > TRIGGER_PAUSE_RESET.
> 
> But what state that we move after we reset the PAUSE? is it RUNNING
> (iow, equals to PAUSE_RELEASE) or something else?

It'll be temporarily set to RUNNING.

> In case of suspend while the state is paused we will:
> TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?

For suspend, PAUSE_RESET, then SUSPEND.
For dropping, PAUSE_RESET, then STOP.

> Most drivers are doing the same thing for STOP and SUSPEND trigger,
> however it is possible to do extra steps for SUSPEND to make sure that
> the whole system is in a lowest power state.

Yes, that's the reason I proposed PAUSE_RESET.  Then the driver can
handle the following stop or suspend operation differently.

That said, PAUSE_RESET is just another variant of PAUSE_RELEASE.
But this shouldn't actually enable the audio output again, but prepare
for the next SUSPEND or STOP trigger.


Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 02/04/2025 14:50, Takashi Iwai wrote:
> On Wed, 02 Apr 2025 13:43:57 +0200,
> Péter Ujfalusi wrote:
>>
>>
>>
>> On 02/04/2025 14:21, Takashi Iwai wrote:
>>>>> On the second thought, yet another variant would be to introduce only
>>>>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
>>>>> PAUSED state as a step before stopping the stream.  This can be
>>>>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
>>>>> If the driver doesn't support this mode, the PCM core may fall back to
>>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
>>>>
>>>> This looks identical to my proposed
>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
>>>> explanatory. It's not clear what PAUSE_RESET should do (probably the
>>>> stream should be kept inactive).
>>>
>>> There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
>>> doesn't have to stop the stream by itself, but it's only a preparation
>>> for stopping.  That is, a proper TRIGGER_STOP always follows after
>>> TRIGGER_PAUSE_RESET.
>>
>> But what state that we move after we reset the PAUSE? is it RUNNING
>> (iow, equals to PAUSE_RELEASE) or something else?
> 
> It'll be temporarily set to RUNNING.
> 
>> In case of suspend while the state is paused we will:
>> TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?
> 
> For suspend, PAUSE_RESET, then SUSPEND.
> For dropping, PAUSE_RESET, then STOP.

OK, but this is still the same as PAUSE_RELEASE+SUSPEND when it comes to
suspended_state, no?

I mean you need to move to RUNNING to be suspend, or stop to skip the
suspend.
>> Most drivers are doing the same thing for STOP and SUSPEND trigger,
>> however it is possible to do extra steps for SUSPEND to make sure that
>> the whole system is in a lowest power state.
> 
> Yes, that's the reason I proposed PAUSE_RESET.  Then the driver can
> handle the following stop or suspend operation differently.
> 
> That said, PAUSE_RESET is just another variant of PAUSE_RELEASE.
> But this shouldn't actually enable the audio output again, but prepare
> for the next SUSPEND or STOP trigger.

I'm not sure how this would be seen in ASoC level. Surely we need to
have flag per components that it is handling PAUSE_RESET as codecs would
like don't want to do it or they migh, hm.
But ASoC maintains another set of state for the dpcm:
SND_SOC_DPCM_STATE_* and it has this as well among other state-machine
things:
case SNDRV_PCM_TRIGGER_SUSPEND:
	if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
		goto next;

So, this is dpcm level, but one component can support PAUSE_RESET while
the other does not and the transitions are not too clear to me.

But the idea of resurrecting a paused stream with mute held is not bad.

I'm still uncertain how this actually works now on systems where RESUME
is supported and we suspend while a PCM is in paused...

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 14:52:48 +0200,
Péter Ujfalusi wrote:
> 
> 
> 
> On 02/04/2025 14:50, Takashi Iwai wrote:
> > On Wed, 02 Apr 2025 13:43:57 +0200,
> > Péter Ujfalusi wrote:
> >>
> >>
> >>
> >> On 02/04/2025 14:21, Takashi Iwai wrote:
> >>>>> On the second thought, yet another variant would be to introduce only
> >>>>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
> >>>>> PAUSED state as a step before stopping the stream.  This can be
> >>>>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
> >>>>> If the driver doesn't support this mode, the PCM core may fall back to
> >>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
> >>>>
> >>>> This looks identical to my proposed
> >>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
> >>>> explanatory. It's not clear what PAUSE_RESET should do (probably the
> >>>> stream should be kept inactive).
> >>>
> >>> There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
> >>> doesn't have to stop the stream by itself, but it's only a preparation
> >>> for stopping.  That is, a proper TRIGGER_STOP always follows after
> >>> TRIGGER_PAUSE_RESET.
> >>
> >> But what state that we move after we reset the PAUSE? is it RUNNING
> >> (iow, equals to PAUSE_RELEASE) or something else?
> > 
> > It'll be temporarily set to RUNNING.
> > 
> >> In case of suspend while the state is paused we will:
> >> TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?
> > 
> > For suspend, PAUSE_RESET, then SUSPEND.
> > For dropping, PAUSE_RESET, then STOP.
> 
> OK, but this is still the same as PAUSE_RELEASE+SUSPEND when it comes to
> suspended_state, no?
> I mean you need to move to RUNNING to be suspend, or stop to skip the
> suspend.

Yes, and this needs to be addressed.  e.g. save the suspended_state
beforehand.

> >> Most drivers are doing the same thing for STOP and SUSPEND trigger,
> >> however it is possible to do extra steps for SUSPEND to make sure that
> >> the whole system is in a lowest power state.
> > 
> > Yes, that's the reason I proposed PAUSE_RESET.  Then the driver can
> > handle the following stop or suspend operation differently.
> > 
> > That said, PAUSE_RESET is just another variant of PAUSE_RELEASE.
> > But this shouldn't actually enable the audio output again, but prepare
> > for the next SUSPEND or STOP trigger.
> 
> I'm not sure how this would be seen in ASoC level. Surely we need to
> have flag per components that it is handling PAUSE_RESET as codecs would
> like don't want to do it or they migh, hm.
> But ASoC maintains another set of state for the dpcm:
> SND_SOC_DPCM_STATE_* and it has this as well among other state-machine
> things:
> case SNDRV_PCM_TRIGGER_SUSPEND:
> 	if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
> 		goto next;
> 
> So, this is dpcm level, but one component can support PAUSE_RESET while
> the other does not and the transitions are not too clear to me.

It's a good question.  ASoC may have the own intermediate state
(SND_SOC_DPCM_STATE_PAUSED_RESET) or just use
SND_SOC_DPCM_STATE_START but leave all to BE trigger (by passing
SNDRV_PCM_TRIGGER_PAUSE_RESET).

> But the idea of resurrecting a paused stream with mute held is not bad.
>
> I'm still uncertain how this actually works now on systems where RESUME
> is supported and we suspend while a PCM is in paused...

Is it a general question about suspend+resume during pause, or the
behavior change after PAUSE_RESET?  PAUSE_RESET won't be applied to
the drivers that support RESUME.

BTW, I'm not pushing to sell PAUSE_RESET so much for now at all.
Currently rather in a brain storming phase.  Let's try to pick up the
more appropriate one.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 02/04/2025 16:23, Takashi Iwai wrote:
> On Wed, 02 Apr 2025 14:52:48 +0200,
> Péter Ujfalusi wrote:
>>
>>
>>
>> On 02/04/2025 14:50, Takashi Iwai wrote:
>>> On Wed, 02 Apr 2025 13:43:57 +0200,
>>> Péter Ujfalusi wrote:
>>>>
>>>>
>>>>
>>>> On 02/04/2025 14:21, Takashi Iwai wrote:
>>>>>>> On the second thought, yet another variant would be to introduce only
>>>>>>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
>>>>>>> PAUSED state as a step before stopping the stream.  This can be
>>>>>>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
>>>>>>> If the driver doesn't support this mode, the PCM core may fall back to
>>>>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
>>>>>>
>>>>>> This looks identical to my proposed
>>>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
>>>>>> explanatory. It's not clear what PAUSE_RESET should do (probably the
>>>>>> stream should be kept inactive).
>>>>>
>>>>> There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
>>>>> doesn't have to stop the stream by itself, but it's only a preparation
>>>>> for stopping.  That is, a proper TRIGGER_STOP always follows after
>>>>> TRIGGER_PAUSE_RESET.
>>>>
>>>> But what state that we move after we reset the PAUSE? is it RUNNING
>>>> (iow, equals to PAUSE_RELEASE) or something else?
>>>
>>> It'll be temporarily set to RUNNING.
>>>
>>>> In case of suspend while the state is paused we will:
>>>> TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?
>>>
>>> For suspend, PAUSE_RESET, then SUSPEND.
>>> For dropping, PAUSE_RESET, then STOP.
>>
>> OK, but this is still the same as PAUSE_RELEASE+SUSPEND when it comes to
>> suspended_state, no?
>> I mean you need to move to RUNNING to be suspend, or stop to skip the
>> suspend.
> 
> Yes, and this needs to be addressed.  e.g. save the suspended_state
> beforehand.
> 
>>>> Most drivers are doing the same thing for STOP and SUSPEND trigger,
>>>> however it is possible to do extra steps for SUSPEND to make sure that
>>>> the whole system is in a lowest power state.
>>>
>>> Yes, that's the reason I proposed PAUSE_RESET.  Then the driver can
>>> handle the following stop or suspend operation differently.
>>>
>>> That said, PAUSE_RESET is just another variant of PAUSE_RELEASE.
>>> But this shouldn't actually enable the audio output again, but prepare
>>> for the next SUSPEND or STOP trigger.
>>
>> I'm not sure how this would be seen in ASoC level. Surely we need to
>> have flag per components that it is handling PAUSE_RESET as codecs would
>> like don't want to do it or they migh, hm.
>> But ASoC maintains another set of state for the dpcm:
>> SND_SOC_DPCM_STATE_* and it has this as well among other state-machine
>> things:
>> case SNDRV_PCM_TRIGGER_SUSPEND:
>> 	if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
>> 		goto next;
>>
>> So, this is dpcm level, but one component can support PAUSE_RESET while
>> the other does not and the transitions are not too clear to me.
> 
> It's a good question.  ASoC may have the own intermediate state
> (SND_SOC_DPCM_STATE_PAUSED_RESET) or just use
> SND_SOC_DPCM_STATE_START but leave all to BE trigger (by passing
> SNDRV_PCM_TRIGGER_PAUSE_RESET).
> 
>> But the idea of resurrecting a paused stream with mute held is not bad.
>>
>> I'm still uncertain how this actually works now on systems where RESUME
>> is supported and we suspend while a PCM is in paused...
> 
> Is it a general question about suspend+resume during pause, or the
> behavior change after PAUSE_RESET?  PAUSE_RESET won't be applied to
> the drivers that support RESUME.
> 
> BTW, I'm not pushing to sell PAUSE_RESET so much for now at all.
> Currently rather in a brain storming phase.  Let's try to pick up the
> more appropriate one.

While I think the patch is a good start to handle the current situation
with a minimal change (and a possibility of dropping it when the more
elaborate and likely laborious one is available), this is how it was
before the patch in regards to suspend while paused:

RESUME is suported:
- Stream is PAUSED
- No SUSPEND trigger is sent (stream is not running), state changes to
SUSPENDED, suspended_state is PAUSED
- On resume, no RESUME trigger is sent (suspended state is not RUNNING
or DRAIN), but the state is changed to PAUSED.
- On PAUSE_RELEASE the trigger is sent (how it is handled in driver is
another question as we had suspended the system in between).

RESUME is not suported:
- Stream is PAUSED
- No SUSPEND trigger is sent (stream is not running), state changes to
SUSPENDED, suspended_state is PAUSED
- On resume, no RESUME trigger is sent (resume is not supported), the
state remains to SUSPENDED, suspended_state is PAUSED
On PAUSE_RELEASE the core will raturn error as the state is not PAUSED,
it is SUSPENDED and we go to xrun recovery
- driver received PAUSE_PUSH but never receives PAUSE_RELEASE

I think that the SUSPEND trigger (or a new similar trigger) should be
sent to a PAUSED stream regardless of RESUME support. Imho it is not a
correct assumption that a PAUSED stream is ready for system suspend, it
might not be, it might needs to be prepared for that.
If we use the same SUSPEND trigger than the drivers need to track their
paused state as well to know what to do - we rely on the enforced state
changes in core to do this implicitly atm.

If there is no RESUME support, then it is a bit more complicated as
after the system suspend the audio hardware's state does not matter,
whenever it was RUNNMING or PAUSED we need to go to xrun recovery to
restart, resume is not supported.

But if RESUME is not supported we do need to send the PAUSE_RELEASE to
balance the PUSH.
Should we set XRUN as suspended_state in case of RESUME is not
supported? Or some other state which indicates that whatever we had
before, we are not going to be able to pick up where we were?

The fact that we discuss this shows how much this corner case have been
excercised (we eneded upm disabling this in CI testing for now in SOF).

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Thu, 03 Apr 2025 12:13:39 +0200,
Péter Ujfalusi wrote:
> 
> 
> 
> On 02/04/2025 16:23, Takashi Iwai wrote:
> > On Wed, 02 Apr 2025 14:52:48 +0200,
> > Péter Ujfalusi wrote:
> >>
> >>
> >>
> >> On 02/04/2025 14:50, Takashi Iwai wrote:
> >>> On Wed, 02 Apr 2025 13:43:57 +0200,
> >>> Péter Ujfalusi wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 02/04/2025 14:21, Takashi Iwai wrote:
> >>>>>>> On the second thought, yet another variant would be to introduce only
> >>>>>>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
> >>>>>>> PAUSED state as a step before stopping the stream.  This can be
> >>>>>>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
> >>>>>>> If the driver doesn't support this mode, the PCM core may fall back to
> >>>>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
> >>>>>>
> >>>>>> This looks identical to my proposed
> >>>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
> >>>>>> explanatory. It's not clear what PAUSE_RESET should do (probably the
> >>>>>> stream should be kept inactive).
> >>>>>
> >>>>> There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
> >>>>> doesn't have to stop the stream by itself, but it's only a preparation
> >>>>> for stopping.  That is, a proper TRIGGER_STOP always follows after
> >>>>> TRIGGER_PAUSE_RESET.
> >>>>
> >>>> But what state that we move after we reset the PAUSE? is it RUNNING
> >>>> (iow, equals to PAUSE_RELEASE) or something else?
> >>>
> >>> It'll be temporarily set to RUNNING.
> >>>
> >>>> In case of suspend while the state is paused we will:
> >>>> TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?
> >>>
> >>> For suspend, PAUSE_RESET, then SUSPEND.
> >>> For dropping, PAUSE_RESET, then STOP.
> >>
> >> OK, but this is still the same as PAUSE_RELEASE+SUSPEND when it comes to
> >> suspended_state, no?
> >> I mean you need to move to RUNNING to be suspend, or stop to skip the
> >> suspend.
> > 
> > Yes, and this needs to be addressed.  e.g. save the suspended_state
> > beforehand.
> > 
> >>>> Most drivers are doing the same thing for STOP and SUSPEND trigger,
> >>>> however it is possible to do extra steps for SUSPEND to make sure that
> >>>> the whole system is in a lowest power state.
> >>>
> >>> Yes, that's the reason I proposed PAUSE_RESET.  Then the driver can
> >>> handle the following stop or suspend operation differently.
> >>>
> >>> That said, PAUSE_RESET is just another variant of PAUSE_RELEASE.
> >>> But this shouldn't actually enable the audio output again, but prepare
> >>> for the next SUSPEND or STOP trigger.
> >>
> >> I'm not sure how this would be seen in ASoC level. Surely we need to
> >> have flag per components that it is handling PAUSE_RESET as codecs would
> >> like don't want to do it or they migh, hm.
> >> But ASoC maintains another set of state for the dpcm:
> >> SND_SOC_DPCM_STATE_* and it has this as well among other state-machine
> >> things:
> >> case SNDRV_PCM_TRIGGER_SUSPEND:
> >> 	if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
> >> 		goto next;
> >>
> >> So, this is dpcm level, but one component can support PAUSE_RESET while
> >> the other does not and the transitions are not too clear to me.
> > 
> > It's a good question.  ASoC may have the own intermediate state
> > (SND_SOC_DPCM_STATE_PAUSED_RESET) or just use
> > SND_SOC_DPCM_STATE_START but leave all to BE trigger (by passing
> > SNDRV_PCM_TRIGGER_PAUSE_RESET).
> > 
> >> But the idea of resurrecting a paused stream with mute held is not bad.
> >>
> >> I'm still uncertain how this actually works now on systems where RESUME
> >> is supported and we suspend while a PCM is in paused...
> > 
> > Is it a general question about suspend+resume during pause, or the
> > behavior change after PAUSE_RESET?  PAUSE_RESET won't be applied to
> > the drivers that support RESUME.
> > 
> > BTW, I'm not pushing to sell PAUSE_RESET so much for now at all.
> > Currently rather in a brain storming phase.  Let's try to pick up the
> > more appropriate one.
> 
> While I think the patch is a good start to handle the current situation
> with a minimal change (and a possibility of dropping it when the more
> elaborate and likely laborious one is available), this is how it was
> before the patch in regards to suspend while paused:
> 
> RESUME is suported:
> - Stream is PAUSED
> - No SUSPEND trigger is sent (stream is not running), state changes to
> SUSPENDED, suspended_state is PAUSED
> - On resume, no RESUME trigger is sent (suspended state is not RUNNING
> or DRAIN), but the state is changed to PAUSED.
> - On PAUSE_RELEASE the trigger is sent (how it is handled in driver is
> another question as we had suspended the system in between).
> 
> RESUME is not suported:
> - Stream is PAUSED
> - No SUSPEND trigger is sent (stream is not running), state changes to
> SUSPENDED, suspended_state is PAUSED
> - On resume, no RESUME trigger is sent (resume is not supported), the
> state remains to SUSPENDED, suspended_state is PAUSED
> On PAUSE_RELEASE the core will raturn error as the state is not PAUSED,
> it is SUSPENDED and we go to xrun recovery
> - driver received PAUSE_PUSH but never receives PAUSE_RELEASE
> 
> I think that the SUSPEND trigger (or a new similar trigger) should be
> sent to a PAUSED stream regardless of RESUME support. Imho it is not a
> correct assumption that a PAUSED stream is ready for system suspend, it
> might not be, it might needs to be prepared for that.
> If we use the same SUSPEND trigger than the drivers need to track their
> paused state as well to know what to do - we rely on the enforced state
> changes in core to do this implicitly atm.
> 
> If there is no RESUME support, then it is a bit more complicated as
> after the system suspend the audio hardware's state does not matter,
> whenever it was RUNNMING or PAUSED we need to go to xrun recovery to
> restart, resume is not supported.
> 
> But if RESUME is not supported we do need to send the PAUSE_RELEASE to
> balance the PUSH.
> Should we set XRUN as suspended_state in case of RESUME is not
> supported? Or some other state which indicates that whatever we had
> before, we are not going to be able to pick up where we were?
> 
> The fact that we discuss this shows how much this corner case have been
> excercised (we eneded upm disabling this in CI testing for now in SOF).

I agree that your patch can be a good start, at least, it addresses
the existing issue with the minimal change.  There are still rough
edges and we'll need to address, but I believe the patch (or modified
/ fixed one) can be applied to 6.15 kernel, while keeping the
development for 6.16.

I did some quick work on it, and now implemented
SNDRV_PCM_PAUSE_RELEASE_STOP trigger.  I guess it's more or less
aligned with what Jaroslav suggested.

The patches are pushed out to topic/pcm-pause-rework branch of my
sound.git tree.  Please take a look.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 03. 04. 25 16:01, Takashi Iwai wrote:

> I agree that your patch can be a good start, at least, it addresses
> the existing issue with the minimal change.  There are still rough
> edges and we'll need to address, but I believe the patch (or modified
> / fixed one) can be applied to 6.15 kernel, while keeping the
> development for 6.16.

I did a quick research how is RESUME/SUSPEND implemented in current drivers 
and I found that it may be considered to enable SUSPEND trigger also in the 
paused state:

find sound/ -name "*.[ch]" -exec grep -A 2 -B 2 -H -E \
     "SNDRV_PCM_TRIGGER_(RESUME|SUSPEND)" {} \;

My comments - seems almost all drivers handles SUSPEND as STOP:

sound/pci/ymfpci/ymfpci_main.c
  - STOP/SUSPEND different, should not be a problem
sound/pci/nm256/nm256.c
sound/pci/intel8x0.c
  - only saves suspend flag additionaly to STOP
sound/soc
  - some drivers ignores suspend/resume (ignore_suspend flag)
sound/soc/pxa/pxa-ssp.c
  - should not be a problem (enable/disable hw parts)
sound/soc/sti/uniperif_player.c
  - only resume trigger is supported ? but no resume info is set
sound/soc/sof/intel/hda-dai.c
  - only suspend support, but resume is not supported
sound/xen/xen_snd_front_alsa.c
  - it's just rpc
sound/virtio/virtio_pcm_ops.c
  - only suspend support, no resume

It would be probably nice if more eyes verifies this, but the only suspicious 
place is the xen front end. And some drivers may be broken already (no changes 
for them).

I believe that this patch should be enough to resolve the issue instead the 
non-elegant pause release and may be applied also to 6.15:

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 4057f9f10aee..63a1b37cc098 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1694,8 +1694,17 @@ 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)
+                       return 0;
+               if (runtime->state != SNDRV_PCM_STATE_PAUSED)
+                       return 0;
+               /*
+                * When resume is not supported, SUSPEND should STOP stream.
+                * For futher operation, the stream must be fully restarted
+                * to leave the permanent SUSPEND state.
+                */
+       }
         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
         runtime->stop_operating = true;
         return 0; /* suspend unconditionally */

				Jaroslav

> I did some quick work on it, and now implemented
> SNDRV_PCM_PAUSE_RELEASE_STOP trigger.  I guess it's more or less
> aligned with what Jaroslav suggested.

Nice cleanups. The "ALSA: pcm: Save the proper suspended_state for 
non-resumable case, too" should be recoded. The state should be handled in 
action callbacks for all streams separetely.

Also, I would consider to call suspend/resume triggers (depending on a flag 
like can_pause_release_stop) when the stream is paused, too. Some drivers may 
want this scheme.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Thu, 03 Apr 2025 17:24:44 +0200,
Jaroslav Kysela wrote:
> 
> On 03. 04. 25 16:01, Takashi Iwai wrote:
> 
> > I agree that your patch can be a good start, at least, it addresses
> > the existing issue with the minimal change.  There are still rough
> > edges and we'll need to address, but I believe the patch (or modified
> > / fixed one) can be applied to 6.15 kernel, while keeping the
> > development for 6.16.
> 
> I did a quick research how is RESUME/SUSPEND implemented in current
> drivers and I found that it may be considered to enable SUSPEND
> trigger also in the paused state:
> 
> find sound/ -name "*.[ch]" -exec grep -A 2 -B 2 -H -E \
>     "SNDRV_PCM_TRIGGER_(RESUME|SUSPEND)" {} \;
> 
> My comments - seems almost all drivers handles SUSPEND as STOP:
> 
> sound/pci/ymfpci/ymfpci_main.c
>  - STOP/SUSPEND different, should not be a problem
> sound/pci/nm256/nm256.c
> sound/pci/intel8x0.c
>  - only saves suspend flag additionaly to STOP
> sound/soc
>  - some drivers ignores suspend/resume (ignore_suspend flag)
> sound/soc/pxa/pxa-ssp.c
>  - should not be a problem (enable/disable hw parts)
> sound/soc/sti/uniperif_player.c
>  - only resume trigger is supported ? but no resume info is set
> sound/soc/sof/intel/hda-dai.c
>  - only suspend support, but resume is not supported
> sound/xen/xen_snd_front_alsa.c
>  - it's just rpc
> sound/virtio/virtio_pcm_ops.c
>  - only suspend support, no resume
> 
> It would be probably nice if more eyes verifies this, but the only
> suspicious place is the xen front end. And some drivers may be broken
> already (no changes for them).

The xen driver provides SNDRV_PCM_INFO_PAUSE, but it doesn't handle
SNDRV_PCM_TRIGGER_PAUSE_* at all -- so the pause handling is broken
there, as it seems.

> I believe that this patch should be enough to resolve the issue
> instead the non-elegant pause release and may be applied also to 6.15:
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 4057f9f10aee..63a1b37cc098 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,17 @@ 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)
> +                       return 0;
> +               if (runtime->state != SNDRV_PCM_STATE_PAUSED)
> +                       return 0;
> +               /*
> +                * When resume is not supported, SUSPEND should STOP stream.
> +                * For futher operation, the stream must be fully restarted
> +                * to leave the permanent SUSPEND state.
> +                */
> +       }
>         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>         runtime->stop_operating = true;
>         return 0; /* suspend unconditionally */

I'm afraid that it's not that straightforward, unfortunately.
There are some stuff that require the pause release.

For example, pcm_dmaengine.c uses dmaengine_pause() as the suspend,
and if we apply this change, it'll end up with dmaengine_pause() calls
twice.  As far as I see, a similar pattern is seen in ASoC Intel catpt
driver (and maybe some others), too.

And, in drivers/soundwire/intel.c, there is already a workaround for
suspend-at-pause, and this might conflict.  We need to verify it.

Last but not least, the ASoC PCM core has its own DPCM state, and the
trigger-SUSPEND skips the operation unless the stream has been
running.  I believe that's the very starting point of the problem
Peter's patch tries to address.


> > I did some quick work on it, and now implemented
> > SNDRV_PCM_PAUSE_RELEASE_STOP trigger.  I guess it's more or less
> > aligned with what Jaroslav suggested.
> 
> Nice cleanups. The "ALSA: pcm: Save the proper suspended_state for
> non-resumable case, too" should be recoded. The state should be
> handled in action callbacks for all streams separetely.

OK, I'll rewrite it.  I wrote in the current way, as it'll be a bit
too complex otherwise, but let me try another shot.

> Also, I would consider to call suspend/resume triggers (depending on a
> flag like can_pause_release_stop) when the stream is paused, too. Some
> drivers may want this scheme.

Do you mean drivers that do share the same operation for
suspend/resume and pause?


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 03. 04. 25 18:09, Takashi Iwai wrote:

> Last but not least, the ASoC PCM core has its own DPCM state, and the
> trigger-SUSPEND skips the operation unless the stream has been
> running.  I believe that's the very starting point of the problem
> Peter's patch tries to address.

Unfortunately, after all discussions, I'm not convinced about Peter's patch. It smells like an workaround rather than a fix for forever. The problem is there for years so we should not concentrate on a quick fix.

>> Also, I would consider to call suspend/resume triggers (depending on a
>> flag like can_pause_release_stop) when the stream is paused, too. Some
>> drivers may want this scheme.
> 
> Do you mean drivers that do share the same operation for
> suspend/resume and pause?

Many drivers supporting pause may use it when we allow PAUSE -> SUSPEND and SUSPEND -> PAUSE transitions.

Something like:

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index ac8f3aef9205..6d6882973883 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -541,6 +541,8 @@ struct snd_pcm {
         bool internal; /* pcm is for internal use only */
         bool nonatomic; /* whole PCM operations are in non-atomic context */
         bool no_device_suspend; /* don't invoke device PM suspend */
+       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED -> SUSPENDED and
+                                               SUSPENDED -> PAUSED transitions */
  #if IS_ENABLED(CONFIG_SND_PCM_OSS)
         struct snd_pcm_oss oss;
  #endif
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 4057f9f10aee..3ec4ef7809ea 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1694,8 +1694,11 @@ 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->do_suspend_and_resume_in_pause ||
+                   runtime->state != SNDRV_PCM_STATE_PAUSED)
+                       return 0;
+       }
         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
         runtime->stop_operating = true;
         return 0; /* suspend unconditionally */
@@ -1798,8 +1801,11 @@ static int snd_pcm_do_resume(struct snd_pcm_substream *substream,
         /* DMA not running previously? */
         if (runtime->suspended_state != SNDRV_PCM_STATE_RUNNING &&
             (runtime->suspended_state != SNDRV_PCM_STATE_DRAINING ||
-            substream->stream != SNDRV_PCM_STREAM_PLAYBACK))
-               return 0;
+            substream->stream != SNDRV_PCM_STREAM_PLAYBACK)) {
+               if (!runtime->do_suspend_and_resume_in_pause ||
+                   runtime->suspend_state != SNDRV_PCM_STATE_PAUSED)
+                       return 0;
+       }
         return substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_RESUME);
  }

This will allow to address this problem correctly without any pause release just to put all parts to the running state and immediately stop the stream. Ideally, all drivers should be migrated to this state scheme and the condition may be removed at the end.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Thu, 03 Apr 2025 21:05:51 +0200,
Jaroslav Kysela wrote:
> 
> On 03. 04. 25 18:09, Takashi Iwai wrote:
> 
> > Last but not least, the ASoC PCM core has its own DPCM state, and the
> > trigger-SUSPEND skips the operation unless the stream has been
> > running.  I believe that's the very starting point of the problem
> > Peter's patch tries to address.
> 
> Unfortunately, after all discussions, I'm not convinced about Peter's patch. It smells like an workaround rather than a fix for forever. The problem is there for years so we should not concentrate on a quick fix.
> 
> >> Also, I would consider to call suspend/resume triggers (depending on a
> >> flag like can_pause_release_stop) when the stream is paused, too. Some
> >> drivers may want this scheme.
> > 
> > Do you mean drivers that do share the same operation for
> > suspend/resume and pause?
> 
> Many drivers supporting pause may use it when we allow PAUSE -> SUSPEND and SUSPEND -> PAUSE transitions.
> 
> Something like:
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index ac8f3aef9205..6d6882973883 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -541,6 +541,8 @@ struct snd_pcm {
>         bool internal; /* pcm is for internal use only */
>         bool nonatomic; /* whole PCM operations are in non-atomic context */
>         bool no_device_suspend; /* don't invoke device PM suspend */
> +       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED -> SUSPENDED and
> +                                               SUSPENDED -> PAUSED transitions */
>  #if IS_ENABLED(CONFIG_SND_PCM_OSS)
>         struct snd_pcm_oss oss;
>  #endif
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 4057f9f10aee..3ec4ef7809ea 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,11 @@ 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->do_suspend_and_resume_in_pause ||
> +                   runtime->state != SNDRV_PCM_STATE_PAUSED)
> +                       return 0;
> +       }
>         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>         runtime->stop_operating = true;
>         return 0; /* suspend unconditionally */
> @@ -1798,8 +1801,11 @@ static int snd_pcm_do_resume(struct snd_pcm_substream *substream,
>         /* DMA not running previously? */
>         if (runtime->suspended_state != SNDRV_PCM_STATE_RUNNING &&
>             (runtime->suspended_state != SNDRV_PCM_STATE_DRAINING ||
> -            substream->stream != SNDRV_PCM_STREAM_PLAYBACK))
> -               return 0;
> +            substream->stream != SNDRV_PCM_STREAM_PLAYBACK)) {
> +               if (!runtime->do_suspend_and_resume_in_pause ||
> +                   runtime->suspend_state != SNDRV_PCM_STATE_PAUSED)
> +                       return 0;
> +       }
>         return substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_RESUME);
>  }
> 
> This will allow to address this problem correctly without any pause release just to put all parts to the running state and immediately stop the stream. Ideally, all drivers should be migrated to this state scheme and the condition may be removed at the end.

OK, this should work, too.
If we deal with another corner case such as PAUSED -> STOP, a new flag
can be added, too. (Or make one flag indicating both meanings.)

Or, there can be a driver that wants to receive trigger SUSPEND even
for the non-running substreams, too.  We need to check the details
before moving forward...

And, if there can be multiple flags, I'd rather introduce a new field
to snd_pcm_hardware, e.g. extra_flags field to keep the internal bit
flags that aren't exposed to user-space.  Otherwise the setup can be
cumbersome for each driver.

Unfortunately, the existing flags in snd_pcm like nonatomic and
no_device_suspend can't be put there, though, as those are referred
for the closed substreams, too.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 04. 04. 25 12:44, Takashi Iwai wrote:
> On Thu, 03 Apr 2025 21:05:51 +0200,
> Jaroslav Kysela wrote:
>>
>> On 03. 04. 25 18:09, Takashi Iwai wrote:
>>
>>> Last but not least, the ASoC PCM core has its own DPCM state, and the
>>> trigger-SUSPEND skips the operation unless the stream has been
>>> running.  I believe that's the very starting point of the problem
>>> Peter's patch tries to address.
>>
>> Unfortunately, after all discussions, I'm not convinced about Peter's patch. It smells like an workaround rather than a fix for forever. The problem is there for years so we should not concentrate on a quick fix.
>>
>>>> Also, I would consider to call suspend/resume triggers (depending on a
>>>> flag like can_pause_release_stop) when the stream is paused, too. Some
>>>> drivers may want this scheme.
>>>
>>> Do you mean drivers that do share the same operation for
>>> suspend/resume and pause?
>>
>> Many drivers supporting pause may use it when we allow PAUSE -> SUSPEND and SUSPEND -> PAUSE transitions.
>>
>> Something like:
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index ac8f3aef9205..6d6882973883 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -541,6 +541,8 @@ struct snd_pcm {
>>          bool internal; /* pcm is for internal use only */
>>          bool nonatomic; /* whole PCM operations are in non-atomic context */
>>          bool no_device_suspend; /* don't invoke device PM suspend */
>> +       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED -> SUSPENDED and
>> +                                               SUSPENDED -> PAUSED transitions */
>>   #if IS_ENABLED(CONFIG_SND_PCM_OSS)
>>          struct snd_pcm_oss oss;
>>   #endif
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 4057f9f10aee..3ec4ef7809ea 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -1694,8 +1694,11 @@ 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->do_suspend_and_resume_in_pause ||
>> +                   runtime->state != SNDRV_PCM_STATE_PAUSED)
>> +                       return 0;
>> +       }
>>          substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>>          runtime->stop_operating = true;
>>          return 0; /* suspend unconditionally */
>> @@ -1798,8 +1801,11 @@ static int snd_pcm_do_resume(struct snd_pcm_substream *substream,
>>          /* DMA not running previously? */
>>          if (runtime->suspended_state != SNDRV_PCM_STATE_RUNNING &&
>>              (runtime->suspended_state != SNDRV_PCM_STATE_DRAINING ||
>> -            substream->stream != SNDRV_PCM_STREAM_PLAYBACK))
>> -               return 0;
>> +            substream->stream != SNDRV_PCM_STREAM_PLAYBACK)) {
>> +               if (!runtime->do_suspend_and_resume_in_pause ||
>> +                   runtime->suspend_state != SNDRV_PCM_STATE_PAUSED)
>> +                       return 0;
>> +       }
>>          return substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_RESUME);
>>   }
>>
>> This will allow to address this problem correctly without any pause release just to put all parts to the running state and immediately stop the stream. Ideally, all drivers should be migrated to this state scheme and the condition may be removed at the end.
> 
> OK, this should work, too.
> If we deal with another corner case such as PAUSED -> STOP, a new flag
> can be added, too. (Or make one flag indicating both meanings.)
> 
> Or, there can be a driver that wants to receive trigger SUSPEND even
> for the non-running substreams, too.  We need to check the details
> before moving forward...
> 
> And, if there can be multiple flags, I'd rather introduce a new field
> to snd_pcm_hardware, e.g. extra_flags field to keep the internal bit
> flags that aren't exposed to user-space.  Otherwise the setup can be
> cumbersome for each driver.

I think that one flag for new suspend/pause/resume trigger behavior should be 
enough. We may reuse SNDRV_PCM_INFO_DRAIN_TRIGGER flag  which is no longer 
used. It was probably implemented only in removed Intel SST drivers.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Fri, 04 Apr 2025 13:33:10 +0200,
Jaroslav Kysela wrote:
> 
> On 04. 04. 25 12:44, Takashi Iwai wrote:
> > On Thu, 03 Apr 2025 21:05:51 +0200,
> > Jaroslav Kysela wrote:
> >> 
> >> On 03. 04. 25 18:09, Takashi Iwai wrote:
> >> 
> >>> Last but not least, the ASoC PCM core has its own DPCM state, and the
> >>> trigger-SUSPEND skips the operation unless the stream has been
> >>> running.  I believe that's the very starting point of the problem
> >>> Peter's patch tries to address.
> >> 
> >> Unfortunately, after all discussions, I'm not convinced about Peter's patch. It smells like an workaround rather than a fix for forever. The problem is there for years so we should not concentrate on a quick fix.
> >> 
> >>>> Also, I would consider to call suspend/resume triggers (depending on a
> >>>> flag like can_pause_release_stop) when the stream is paused, too. Some
> >>>> drivers may want this scheme.
> >>> 
> >>> Do you mean drivers that do share the same operation for
> >>> suspend/resume and pause?
> >> 
> >> Many drivers supporting pause may use it when we allow PAUSE -> SUSPEND and SUSPEND -> PAUSE transitions.
> >> 
> >> Something like:
> >> 
> >> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> >> index ac8f3aef9205..6d6882973883 100644
> >> --- a/include/sound/pcm.h
> >> +++ b/include/sound/pcm.h
> >> @@ -541,6 +541,8 @@ struct snd_pcm {
> >>          bool internal; /* pcm is for internal use only */
> >>          bool nonatomic; /* whole PCM operations are in non-atomic context */
> >>          bool no_device_suspend; /* don't invoke device PM suspend */
> >> +       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED -> SUSPENDED and
> >> +                                               SUSPENDED -> PAUSED transitions */
> >>   #if IS_ENABLED(CONFIG_SND_PCM_OSS)
> >>          struct snd_pcm_oss oss;
> >>   #endif
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 4057f9f10aee..3ec4ef7809ea 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -1694,8 +1694,11 @@ 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->do_suspend_and_resume_in_pause ||
> >> +                   runtime->state != SNDRV_PCM_STATE_PAUSED)
> >> +                       return 0;
> >> +       }
> >>          substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
> >>          runtime->stop_operating = true;
> >>          return 0; /* suspend unconditionally */
> >> @@ -1798,8 +1801,11 @@ static int snd_pcm_do_resume(struct snd_pcm_substream *substream,
> >>          /* DMA not running previously? */
> >>          if (runtime->suspended_state != SNDRV_PCM_STATE_RUNNING &&
> >>              (runtime->suspended_state != SNDRV_PCM_STATE_DRAINING ||
> >> -            substream->stream != SNDRV_PCM_STREAM_PLAYBACK))
> >> -               return 0;
> >> +            substream->stream != SNDRV_PCM_STREAM_PLAYBACK)) {
> >> +               if (!runtime->do_suspend_and_resume_in_pause ||
> >> +                   runtime->suspend_state != SNDRV_PCM_STATE_PAUSED)
> >> +                       return 0;
> >> +       }
> >>          return substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_RESUME);
> >>   }
> >> 
> >> This will allow to address this problem correctly without any pause release just to put all parts to the running state and immediately stop the stream. Ideally, all drivers should be migrated to this state scheme and the condition may be removed at the end.
> > 
> > OK, this should work, too.
> > If we deal with another corner case such as PAUSED -> STOP, a new flag
> > can be added, too. (Or make one flag indicating both meanings.)
> > 
> > Or, there can be a driver that wants to receive trigger SUSPEND even
> > for the non-running substreams, too.  We need to check the details
> > before moving forward...
> > 
> > And, if there can be multiple flags, I'd rather introduce a new field
> > to snd_pcm_hardware, e.g. extra_flags field to keep the internal bit
> > flags that aren't exposed to user-space.  Otherwise the setup can be
> > cumbersome for each driver.
> 
> I think that one flag for new suspend/pause/resume trigger behavior
> should be enough. We may reuse SNDRV_PCM_INFO_DRAIN_TRIGGER flag
> which is no longer used. It was probably implemented only in removed
> Intel SST drivers.

OK, this is a subtle detail and can be discussed later.
I believe the biggest obstacle in this route is the handling in ASoC
DPCM.  AFAIUC, it has a refcount management of paused BEs, and this
transition can break it.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 03/04/2025 22:05, Jaroslav Kysela wrote:
> On 03. 04. 25 18:09, Takashi Iwai wrote:
> 
>> Last but not least, the ASoC PCM core has its own DPCM state, and the
>> trigger-SUSPEND skips the operation unless the stream has been
>> running.  I believe that's the very starting point of the problem
>> Peter's patch tries to address.
> 
> Unfortunately, after all discussions, I'm not convinced about Peter's
> patch. It smells like an workaround rather than a fix for forever. The
> problem is there for years so we should not concentrate on a quick fix.
> 
>>> Also, I would consider to call suspend/resume triggers (depending on a
>>> flag like can_pause_release_stop) when the stream is paused, too. Some
>>> drivers may want this scheme.
>>
>> Do you mean drivers that do share the same operation for
>> suspend/resume and pause?
> 
> Many drivers supporting pause may use it when we allow PAUSE -> SUSPEND
> and SUSPEND -> PAUSE transitions.
> 
> Something like:
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index ac8f3aef9205..6d6882973883 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -541,6 +541,8 @@ struct snd_pcm {
>         bool internal; /* pcm is for internal use only */
>         bool nonatomic; /* whole PCM operations are in non-atomic
> context */
>         bool no_device_suspend; /* don't invoke device PM suspend */
> +       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED ->
> SUSPENDED and
> +                                               SUSPENDED -> PAUSED
> transitions */

I think the issue is bit different than this.
What needs to be flagged is that the driver is not treating the PAUSE
trigger in the same way as it treats the STOP or SUSPEND, in other
worlds, the PAUSED state is not equal to STOPPED/SUSPENDED.

If the PAUSED state is equal to SUSPENDED state, then the trigger could
be skipped (as it is ignored before my patch unconditionally), but if
the two states are not equal then the SUSPEND must be sent, but the
patches from me and Takashi-san stil can be used as fallback in case the
RESUME is not supported as in that case the driver will certainly not
going to be able to resume to PAUSE_RESUME, so stopping a stream in that
case is appropriate.

Side note: I think we might also have broken case when we stop a PAUSED
stream if the driver's PAUSED state is not equal to STOPPED/SUSPENDED.
It is just that applications never do this, at least I ave never seen a
missing STOP trigger. aplay is doing a pause release before stop at least.


>  #if IS_ENABLED(CONFIG_SND_PCM_OSS)
>         struct snd_pcm_oss oss;
>  #endif
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 4057f9f10aee..3ec4ef7809ea 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,11 @@ 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->do_suspend_and_resume_in_pause ||
> +                   runtime->state != SNDRV_PCM_STATE_PAUSED)
> +                       return 0;
> +       }
>         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>         runtime->stop_operating = true;
>         return 0; /* suspend unconditionally */
> @@ -1798,8 +1801,11 @@ static int snd_pcm_do_resume(struct
> snd_pcm_substream *substream,
>         /* DMA not running previously? */
>         if (runtime->suspended_state != SNDRV_PCM_STATE_RUNNING &&
>             (runtime->suspended_state != SNDRV_PCM_STATE_DRAINING ||
> -            substream->stream != SNDRV_PCM_STREAM_PLAYBACK))
> -               return 0;
> +            substream->stream != SNDRV_PCM_STREAM_PLAYBACK)) {
> +               if (!runtime->do_suspend_and_resume_in_pause ||
> +                   runtime->suspend_state != SNDRV_PCM_STATE_PAUSED)
> +                       return 0;
> +       }
>         return substream->ops->trigger(substream,
> SNDRV_PCM_TRIGGER_RESUME);
>  }
> 
> This will allow to address this problem correctly without any pause
> release just to put all parts to the running state and immediately stop
> the stream. Ideally, all drivers should be migrated to this state scheme
> and the condition may be removed at the end.
> 
>                     Jaroslav
> 

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 08. 04. 25 9:03, Péter Ujfalusi wrote:
> 
> 
> On 03/04/2025 22:05, Jaroslav Kysela wrote:
>> On 03. 04. 25 18:09, Takashi Iwai wrote:
>>
>>> Last but not least, the ASoC PCM core has its own DPCM state, and the
>>> trigger-SUSPEND skips the operation unless the stream has been
>>> running.  I believe that's the very starting point of the problem
>>> Peter's patch tries to address.
>>
>> Unfortunately, after all discussions, I'm not convinced about Peter's
>> patch. It smells like an workaround rather than a fix for forever. The
>> problem is there for years so we should not concentrate on a quick fix.
>>
>>>> Also, I would consider to call suspend/resume triggers (depending on a
>>>> flag like can_pause_release_stop) when the stream is paused, too. Some
>>>> drivers may want this scheme.
>>>
>>> Do you mean drivers that do share the same operation for
>>> suspend/resume and pause?
>>
>> Many drivers supporting pause may use it when we allow PAUSE -> SUSPEND
>> and SUSPEND -> PAUSE transitions.
>>
>> Something like:
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index ac8f3aef9205..6d6882973883 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -541,6 +541,8 @@ struct snd_pcm {
>>          bool internal; /* pcm is for internal use only */
>>          bool nonatomic; /* whole PCM operations are in non-atomic
>> context */
>>          bool no_device_suspend; /* don't invoke device PM suspend */
>> +       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED ->
>> SUSPENDED and
>> +                                               SUSPENDED -> PAUSED
>> transitions */
> 
> I think the issue is bit different than this.
> What needs to be flagged is that the driver is not treating the PAUSE
> trigger in the same way as it treats the STOP or SUSPEND, in other
> worlds, the PAUSED state is not equal to STOPPED/SUSPENDED.
> 
> If the PAUSED state is equal to SUSPENDED state, then the trigger could
> be skipped (as it is ignored before my patch unconditionally), but if
> the two states are not equal then the SUSPEND must be sent, but the
> patches from me and Takashi-san stil can be used as fallback in case the
> RESUME is not supported as in that case the driver will certainly not
> going to be able to resume to PAUSE_RESUME, so stopping a stream in that
 > case is appropriate.

The question is if the fallback is required when we had this issue for years. 
Using only a new trigger - to not release pause but directly do stop from the 
pause state - is acceptable for me. It should be easy to fix affected drivers. 
But on other side, the allowing state transitions (and even multiple triggers 
for finer driver notifications) may have also benefits for drivers.

> Side note: I think we might also have broken case when we stop a PAUSED
> stream if the driver's PAUSED state is not equal to STOPPED/SUSPENDED.
> It is just that applications never do this, at least I ave never seen a
> missing STOP trigger. aplay is doing a pause release before stop at least.

Aplay implementation is just about simplicity. The pause is handled in one 
routine which releases pause at the end. When the file descriptor is closed,
snd_pcm_drop() is called and the pause is released there, too. Logically, it's 
not 100% correct thing. We know that we want to stop stream before the pause 
release is called.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 08/04/2025 11:51, Jaroslav Kysela wrote:
>>>          bool no_device_suspend; /* don't invoke device PM suspend */
>>> +       bool do_suspend_and_resume_in_pause; /* allow direct PAUSED ->
>>> SUSPENDED and
>>> +                                               SUSPENDED -> PAUSED
>>> transitions */
>>
>> I think the issue is bit different than this.
>> What needs to be flagged is that the driver is not treating the PAUSE
>> trigger in the same way as it treats the STOP or SUSPEND, in other
>> worlds, the PAUSED state is not equal to STOPPED/SUSPENDED.
>>
>> If the PAUSED state is equal to SUSPENDED state, then the trigger could
>> be skipped (as it is ignored before my patch unconditionally), but if
>> the two states are not equal then the SUSPEND must be sent, but the
>> patches from me and Takashi-san stil can be used as fallback in case the
>> RESUME is not supported as in that case the driver will certainly not
>> going to be able to resume to PAUSE_RESUME, so stopping a stream in that
>> case is appropriate.
> 
> The question is if the fallback is required when we had this issue for
> years.

This discussion is a proof that we might want.
We ended up disabling testing the suspend while paused case entirely for
quite some time and I could only get it working with my RFC attempt
before Takashi-san suggestion to do this in core level.

I agree that this is a rare corner case, more academic than not, users
will likely not going to face with as none of the audio servers use
pause so the chances that the system goes suspend while audio is paused
is close to zero.

> Using only a new trigger - to not release pause but directly do
> stop from the pause state - is acceptable for me. It should be easy to
> fix affected drivers. But on other side, the allowing state transitions
> (and even multiple triggers for finer driver notifications) may have
> also benefits for drivers.
> 
>> Side note: I think we might also have broken case when we stop a PAUSED
>> stream if the driver's PAUSED state is not equal to STOPPED/SUSPENDED.
>> It is just that applications never do this, at least I ave never seen a
>> missing STOP trigger. aplay is doing a pause release before stop at
>> least.
> 
> Aplay implementation is just about simplicity. The pause is handled in
> one routine which releases pause at the end. When the file descriptor is
> closed,
> snd_pcm_drop() is called and the pause is released there, too.
> Logically, it's not 100% correct thing. We know that we want to stop
> stream before the pause release is called.

The assumption is that PAUSED == STOPPED == SUSPENDED is not correct
either. This is not true for some drivers or devices.
I would go as far as to say that most drivers are mishandling the PAUSE
as stop, they likely cannot resume from the exact place where the pause
taken - they stop and if they have FIFO, they drop that as well...

> 
>                     Jaroslav
> 

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 03/04/2025 18:24, Jaroslav Kysela wrote:
> On 03. 04. 25 16:01, Takashi Iwai wrote:
> 
>> I agree that your patch can be a good start, at least, it addresses
>> the existing issue with the minimal change.  There are still rough
>> edges and we'll need to address, but I believe the patch (or modified
>> / fixed one) can be applied to 6.15 kernel, while keeping the
>> development for 6.16.
> 
> I did a quick research how is RESUME/SUSPEND implemented in current
> drivers and I found that it may be considered to enable SUSPEND trigger
> also in the paused state:
> 
> find sound/ -name "*.[ch]" -exec grep -A 2 -B 2 -H -E \
>     "SNDRV_PCM_TRIGGER_(RESUME|SUSPEND)" {} \;
> 
> My comments - seems almost all drivers handles SUSPEND as STOP:
> 
> sound/pci/ymfpci/ymfpci_main.c
>  - STOP/SUSPEND different, should not be a problem
> sound/pci/nm256/nm256.c
> sound/pci/intel8x0.c
>  - only saves suspend flag additionaly to STOP
> sound/soc
>  - some drivers ignores suspend/resume (ignore_suspend flag)
> sound/soc/pxa/pxa-ssp.c
>  - should not be a problem (enable/disable hw parts)
> sound/soc/sti/uniperif_player.c
>  - only resume trigger is supported ? but no resume info is set
> sound/soc/sof/intel/hda-dai.c
>  - only suspend support, but resume is not supported
> sound/xen/xen_snd_front_alsa.c
>  - it's just rpc
> sound/virtio/virtio_pcm_ops.c
>  - only suspend support, no resume
> 
> It would be probably nice if more eyes verifies this, but the only
> suspicious place is the xen front end. And some drivers may be broken
> already (no changes for them).
> 
> I believe that this patch should be enough to resolve the issue instead
> the non-elegant pause release and may be applied also to 6.15:
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 4057f9f10aee..63a1b37cc098 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,17 @@ 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)
> +                       return 0;
> +               if (runtime->state != SNDRV_PCM_STATE_PAUSED)
> +                       return 0;
> +               /*
> +                * When resume is not supported, SUSPEND should STOP
> stream.
> +                * For futher operation, the stream must be fully restarted
> +                * to leave the permanent SUSPEND state.
> +                */
> +       }
>         substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>         runtime->stop_operating = true;
>         return 0; /* suspend unconditionally */

I have tried similar thing and the original problem remains that we are
missing the PAUSE_RELEASE trigger for the PUSH and ASoC core stops the
SUSPEND trigger to reach the drivers.

> 
>                 Jaroslav
> 
>> I did some quick work on it, and now implemented
>> SNDRV_PCM_PAUSE_RELEASE_STOP trigger.  I guess it's more or less
>> aligned with what Jaroslav suggested.
> 
> Nice cleanups. The "ALSA: pcm: Save the proper suspended_state for non-
> resumable case, too" should be recoded. The state should be handled in
> action callbacks for all streams separetely.
> 
> Also, I would consider to call suspend/resume triggers (depending on a
> flag like can_pause_release_stop) when the stream is paused, too. Some
> drivers may want this scheme.
> 
>                     Jaroslav
> 

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 04. 04. 25 10:58, Péter Ujfalusi wrote:
> 
> 
> On 03/04/2025 18:24, Jaroslav Kysela wrote:
>> On 03. 04. 25 16:01, Takashi Iwai wrote:
>>
>>> I agree that your patch can be a good start, at least, it addresses
>>> the existing issue with the minimal change.  There are still rough
>>> edges and we'll need to address, but I believe the patch (or modified
>>> / fixed one) can be applied to 6.15 kernel, while keeping the
>>> development for 6.16.
>>
>> I did a quick research how is RESUME/SUSPEND implemented in current
>> drivers and I found that it may be considered to enable SUSPEND trigger
>> also in the paused state:
>>
>> find sound/ -name "*.[ch]" -exec grep -A 2 -B 2 -H -E \
>>      "SNDRV_PCM_TRIGGER_(RESUME|SUSPEND)" {} \;
>>
>> My comments - seems almost all drivers handles SUSPEND as STOP:
>>
>> sound/pci/ymfpci/ymfpci_main.c
>>   - STOP/SUSPEND different, should not be a problem
>> sound/pci/nm256/nm256.c
>> sound/pci/intel8x0.c
>>   - only saves suspend flag additionaly to STOP
>> sound/soc
>>   - some drivers ignores suspend/resume (ignore_suspend flag)
>> sound/soc/pxa/pxa-ssp.c
>>   - should not be a problem (enable/disable hw parts)
>> sound/soc/sti/uniperif_player.c
>>   - only resume trigger is supported ? but no resume info is set
>> sound/soc/sof/intel/hda-dai.c
>>   - only suspend support, but resume is not supported
>> sound/xen/xen_snd_front_alsa.c
>>   - it's just rpc
>> sound/virtio/virtio_pcm_ops.c
>>   - only suspend support, no resume
>>
>> It would be probably nice if more eyes verifies this, but the only
>> suspicious place is the xen front end. And some drivers may be broken
>> already (no changes for them).
>>
>> I believe that this patch should be enough to resolve the issue instead
>> the non-elegant pause release and may be applied also to 6.15:
>>
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 4057f9f10aee..63a1b37cc098 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -1694,8 +1694,17 @@ 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)
>> +                       return 0;
>> +               if (runtime->state != SNDRV_PCM_STATE_PAUSED)
>> +                       return 0;
>> +               /*
>> +                * When resume is not supported, SUSPEND should STOP
>> stream.
>> +                * For futher operation, the stream must be fully restarted
>> +                * to leave the permanent SUSPEND state.
>> +                */
>> +       }
>>          substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>>          runtime->stop_operating = true;
>>          return 0; /* suspend unconditionally */
> 
> I have tried similar thing and the original problem remains that we are
> missing the PAUSE_RELEASE trigger for the PUSH and ASoC core stops the
> SUSPEND trigger to reach the drivers.

All drivers must be adopted for this scheme including ASoC core, of course.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 03/04/2025 17:01, Takashi Iwai wrote:
>> The fact that we discuss this shows how much this corner case have been
>> excercised (we eneded upm disabling this in CI testing for now in SOF).
> 
> I agree that your patch can be a good start, at least, it addresses
> the existing issue with the minimal change.  There are still rough
> edges and we'll need to address, but I believe the patch (or modified
> / fixed one) can be applied to 6.15 kernel, while keeping the
> development for 6.16.
> 
> I did some quick work on it, and now implemented
> SNDRV_PCM_PAUSE_RELEASE_STOP trigger.  I guess it's more or less
> aligned with what Jaroslav suggested.
> 
> The patches are pushed out to topic/pcm-pause-rework branch of my
> sound.git tree.  Please take a look.

Sorry for the delay, I just got to test the patches from the
pcm-pause-rework and they seams to work OK.
There is only one difference: before suspend the drivers will receive
STOP instead of SUSPEND if the can_pause_release_stop flag is not set,
but thinking about this a bit more, it sounds the correct thing as the
stream is really stopped.

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Jaroslav Kysela]

On 02. 04. 25 13:50, Takashi Iwai wrote:
> On Wed, 02 Apr 2025 13:43:57 +0200,
> Péter Ujfalusi wrote:
>>
>>
>>
>> On 02/04/2025 14:21, Takashi Iwai wrote:
>>>>> On the second thought, yet another variant would be to introduce only
>>>>> one new trigger, SNDRV_PCM_TRIGGER_PAUSE_RESET, for resetting the
>>>>> PAUSED state as a step before stopping the stream.  This can be
>>>>> applied to all cases for snd_pcm_drop() and snd_pcm_suspend(), too.
>>>>> If the driver doesn't support this mode, the PCM core may fall back to
>>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
>>>>
>>>> This looks identical to my proposed
>>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP which is more
>>>> explanatory. It's not clear what PAUSE_RESET should do (probably the
>>>> stream should be kept inactive).
>>>
>>> There is a slight difference.  My proposal, *_TRIGGER_PAUSE_RESET,
>>> doesn't have to stop the stream by itself, but it's only a preparation
>>> for stopping.  That is, a proper TRIGGER_STOP always follows after
>>> TRIGGER_PAUSE_RESET.
>>
>> But what state that we move after we reset the PAUSE? is it RUNNING
>> (iow, equals to PAUSE_RELEASE) or something else?
> 
> It'll be temporarily set to RUNNING.

I don't think that there's requirement to change state at this point. It would 
be better to keep PAUSED state and add this state to the running state checks. 
Something like:

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index ac8f3aef9205..166ef0376293 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -708,6 +708,12 @@ static inline int snd_pcm_running(struct 
snd_pcm_substream *substream)
                  substream->stream == SNDRV_PCM_STREAM_PLAYBACK));
  }

+static inline int snd_pcm_active(struct snd_pcm_substream *substream)
+{
+       return snd_pcm_running(substream) ||
+               substream->runtime->state == SNDRV_PCM_STATE_PAUSED;
+}
+
  /**
   * __snd_pcm_set_state - Change the current PCM state
   * @runtime: PCM runtime to set

But...

>> In case of suspend while the state is paused we will:
>> TRIGGER_PAUSE_RESET followed by STOP or SUSPEND?
> 
> For suspend, PAUSE_RESET, then SUSPEND.
> For dropping, PAUSE_RESET, then STOP.

I'm not convinced if the functionality/information split is a good idea in 
this case. The preparation (code) for suspend and stop may be different. With 
separate triggers, drivers know exactly about the requested behavior (and we 
know it at trigger call time already).

Perhaps, more universal triggers like SNDRV_PCM_TRIGGER_STOP_PREPARE and
SNDRV_PCM_TRIGGER_SUSPEND_PREPARE may be invoked (always) before STOP/PREPARE 
triggers  Drivers should handle also the pause state there on it's own to keep 
the DMA transfers in idle for those cases.

						Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Péter Ujfalusi]

On 02/04/2025 11:52, Jaroslav Kysela wrote:
>>>> Actually, this patch follows the same pattern, too.  It calls
>>>> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
>>>> to the suspend action immediately that sets to SUSPENDED.
>>>
>>> The previous state (suspended_state) will be confusing from the
>>> application with the proposed patch, because there will be RUNNING
>>> state instead PAUSED. This previous state is exported via API.
>>
>> No, it won't happen.  The condition of the new behavior is only when
>> SNDRV_PCM_INFO_RESUME isn't set -- that is, no resume is possible,
>> hence no state recovery happens after resume.  From the application
>> POV, it won't change.
> 
> The suspended_state can be obtained in snd_pcm_status64(). With Peter's
> patch, there will be RUNNING instead PAUSED, don't?

Yes, that is true, but it does not matter.
if the SNDRV_PCM_INFO_RESUME is not set, on resume nothing is going to
be done, the state remains SUSPENDED and the suspended_substate also
retained, so:

The stream was in RUNNING, after suspend:
state = SUSPENDED, suspended_state = RUNNING
After resume:
state = SUSPENDED, suspended_state = RUNNING
The the stream goes to xrun and got restarted.

The stream was in RUNNING, after suspend:
Without my patch / with my patch:
state = SUSPENDED, suspended_state = PAUSED / RUNNING
After resume:
state = SUSPENDED, suspended_state = RUNNING / RUNNING

The stream remains "not running" as resume is not supported and
application knows that it had left the stream paused.

When it tries to PAUSE_RELEASE the stream, in both case the core will
look at the state, which is SUSPENDED and thus the pause release fails
(it is not in paused state), so in both cases we go under a restart
(suspended_state is reset, ignored).

>>>> That's another possibility, yes.  Though, IMO, it makes the
>>>> pause-handling a bit more cumbersome.  With Peter's proposal, the
>>>> pause state change is always paired, so the driver doesn't consider
>>>> about that too much; that's the biggest merit.
>>>
>>> It depends, if we agree that releasing pause is a extra thing to do
>>> when we know that this step is just to minimize state changes for
>>> drivers.
>>>
>>> For my view, the drivers may also receive those triggers:
>>>
>>> SNDRV_PCM_TRIGGER_PAUSE_RELEASE_AND_STOP
>>> SNDRV_PCM_TRIGGER_SUSPEND_IN_PAUSE
>>> SNDRV_PCM_TRIGGER_RELEASE_IN_PAUSE
>>>
>>> This will allow to handle all states properly without any side effects
>>> (like short unwanted DMA transfers).
>>>
>>> The drivers should probably activate those triggers conditionally to
>>> not break current state sequences.
>>
>> Hmm, that looks too complex, IMO.
>>
>> Another slightly more straightforward fix would be just to allow the
>> direct state transition from PAUSED to any state.  Then the remaining
>> piece is all about driver implementations.  And, this can be
>> conditionally operated, e.g. only when some extra flag is set.  When
>> no flag is set, we keep applying PAUSE_RELEASE before the transition
>> like now, including Peter's patch, that can work generically (but not
>> always ideally).
> 
> It looks also feasible. My proposal just allows the straight state
> identification from the trigger value in the drivers.

We could also add a flag that the driver can say that 'I know what I'm
doing' and in that case send just simple SUSPEND and trust the driver
that it really knows what it is doing.
However if the driver does not support RESUME, then things get a bit
complicated. I'm not entirely sure what drivers do when they support
RESUME and they went to suspend with paused state...

But, this is not that simple in ASoC, as there we can have at least four
drivers involved (machine, platform, cpu dai, codec), all of them has to
support this new fine graded state dependent trigger, right?

-- 
Péter

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 11:28:55 +0200,
Péter Ujfalusi wrote:
> 
> 
> On 02/04/2025 11:52, Jaroslav Kysela wrote:
> >>>> Actually, this patch follows the same pattern, too.  It calls
> >>>> snd_pcm_pause(false) to set the state to RUNNING again, then proceed
> >>>> to the suspend action immediately that sets to SUSPENDED.
> >>>
> >>> The previous state (suspended_state) will be confusing from the
> >>> application with the proposed patch, because there will be RUNNING
> >>> state instead PAUSED. This previous state is exported via API.
> >>
> >> No, it won't happen.  The condition of the new behavior is only when
> >> SNDRV_PCM_INFO_RESUME isn't set -- that is, no resume is possible,
> >> hence no state recovery happens after resume.  From the application
> >> POV, it won't change.
> > 
> > The suspended_state can be obtained in snd_pcm_status64(). With Peter's
> > patch, there will be RUNNING instead PAUSED, don't?
> 
> Yes, that is true, but it does not matter.
> if the SNDRV_PCM_INFO_RESUME is not set, on resume nothing is going to
> be done, the state remains SUSPENDED and the suspended_substate also
> retained, so:
> 
> The stream was in RUNNING, after suspend:
> state = SUSPENDED, suspended_state = RUNNING
> After resume:
> state = SUSPENDED, suspended_state = RUNNING
> The the stream goes to xrun and got restarted.
> 
> The stream was in RUNNING, after suspend:
> Without my patch / with my patch:
> state = SUSPENDED, suspended_state = PAUSED / RUNNING
> After resume:
> state = SUSPENDED, suspended_state = RUNNING / RUNNING
> 
> The stream remains "not running" as resume is not supported and
> application knows that it had left the stream paused.
> 
> When it tries to PAUSE_RELEASE the stream, in both case the core will
> look at the state, which is SUSPENDED and thus the pause release fails
> (it is not in paused state), so in both cases we go under a restart
> (suspended_state is reset, ignored).

Well, the suspended_state itself is set no matter whether the resume
is supported or not.  The difference is that it's evaluated in the
driver only by the suspended state, but it's free for user-space to
read this out from the output of snd_pcm_status ioctl.
How seriously it needs to be a different question, but there is a
possibility of breakage.

Though, looking through the code, the suspended_state isn't updated at
all except for the suspend, and it means this can keep a bogus state,
too.  We should fix the code to clear the suspended_state at init
phase.  It's another corner case.


thanks,

Takashi

Re: [PATCH] ALSA: pcm: Release paused streams before suspend if resume is not supported

[Takashi Iwai]

On Wed, 02 Apr 2025 13:27:09 +0200,
Takashi Iwai wrote:
> 
> ... The difference is that it's evaluated in the
> driver only by the suspended state,

Here I meant "only by the resume operation".

> but it's free for user-space to
> read this out from the output of snd_pcm_status ioctl.
> How seriously it needs to be a different question, but there is a
> possibility of breakage.


Takashi