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

[Jaroslav Kysela <perex@perex.cz>]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.