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

[Takashi Iwai <tiwai@suse.de>]

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