| Message ID | 20250401133652.11617-1-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: pcm: Release paused streams before suspend if resume is not supported | expand |
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
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
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
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
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
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.
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
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
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
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
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
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?
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
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
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
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.
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
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
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...
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
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
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).
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
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
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
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
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 >
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
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
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); }
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(+)