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: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