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

[Takashi Iwai <tiwai@suse.de>]

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