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

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

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
> 

-- 
Péter