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

[Takashi Iwai <tiwai@suse.de>]

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