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

[Takashi Iwai <tiwai@suse.de>]

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