[alsa-devel] [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai]

On Wed, 18 May 2016 07:48:15 +0200,
Shengjiu Wang wrote:
> 
> Hi Takashi
> 
>   After adding your patch, I find another regression issue.
> 
>   The alsa-lib may stop at 
>   
>   snd_pcm_write_areas()
> 		snd_pcm_wait_nocheck()
> 
>   with suspend and resume test.
> 
>   The reason is that:
> 
>   In the beginning of playback, before the snd_pcm_dmix_start() is 
> called, the system enter suspend. After resume, snd_pcm_direct_resume()
> update the dmix->state, and dmix->state is 3 (RUNNING, because
> the dmix->spcm is in RUNNING from snd_pcm_dmix_open()). 
> 
>   So in snd_pcm_write_areas() the state is RUNNING, then
> snd_pcm_start() will never be called, after a while,
> alsa-lib will stop at the snd_pcm_wait_nocheck() for the kernel
> will not wake up the timer.

A good point.  Actually the culprit is that we declare dmix as if it's
supporting the resume properly.  Even if the resume works in the
slave, dmix itself can't guarantee the proper resume.  So, we should
rather drop the whole resume stuff from dmix & co.

Below is the patch against to the current git tree.  Give it a try.


thanks,

Takashi

---
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] pcm: Remove resume support from dmix & co

PCM dmix and other plugins inherit the resume behavior from the slave
PCM.  However, the resume on dmix can't work reliably even if the
slave PCM may do resume.  The running state of each dmix stream is
individual and may be PREPARED or RUN_PENDING while the slave PCM is
already in RUNNING.  And, when the slave PCM is resumed, the whole
samples that have been already mapped are also played back, even if
the corresponding dmix stream is still in SUSPENDED.  Such
inconsistencies can't be avoided as long as we manage each stream
individually.

That said, dmix & co can't provide the proper resume support "by
design".  For aligning with it, we should drop the whole resume code
and clear the PCM SND_PCM_INFO_RESUME flag.

Reported-by: Shengjiu Wang <shengjiu.wang at nxp.com>
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 src/pcm/pcm_direct.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index ac082f1a73b2..53c49929cb1f 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -837,27 +837,7 @@ int snd_pcm_direct_prepare(snd_pcm_t *pcm)
 
 int snd_pcm_direct_resume(snd_pcm_t *pcm)
 {
-	snd_pcm_direct_t *dmix = pcm->private_data;
-	int err;
-	
-	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
-	/* resume only when the slave PCM is still in suspended state */
-	if (snd_pcm_state(dmix->spcm) != SND_PCM_STATE_SUSPENDED) {
-		err = 0;
-		goto out;
-	}
-
-	err = snd_pcm_resume(dmix->spcm);
-	if (err == -ENOSYS) {
-		/* FIXME: error handling? */
-		snd_pcm_prepare(dmix->spcm);
-		snd_pcm_start(dmix->spcm);
-		err = 0;
-	}
- out:
-	dmix->state = snd_pcm_state(dmix->spcm);
-	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
-	return err;
+	return -ENOSYS;
 }
 
 #define COPY_SLAVE(field) (dmix->shmptr->s.field = spcm->field)
@@ -865,7 +845,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
 /* copy the slave setting */
 static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 {
-	spcm->info &= ~SND_PCM_INFO_PAUSE;
+	spcm->info &= ~(SND_PCM_INFO_PAUSE | SND_PCM_INFO_RESUME);
 
 	COPY_SLAVE(access);
 	COPY_SLAVE(format);
-- 
2.8.2