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

[Takashi Iwai <tiwai@suse.de>]

On Fri, 04 Apr 2025 13:33:10 +0200,
Jaroslav Kysela wrote:
> 
> On 04. 04. 25 12:44, Takashi Iwai wrote:
> > 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.
> 
> I think that one flag for new suspend/pause/resume trigger behavior
> should be enough. We may reuse SNDRV_PCM_INFO_DRAIN_TRIGGER flag
> which is no longer used. It was probably implemented only in removed
> Intel SST drivers.

OK, this is a subtle detail and can be discussed later.
I believe the biggest obstacle in this route is the handling in ASoC
DPCM.  AFAIUC, it has a refcount management of paused BEs, and this
transition can break it.


thanks,

Takashi