Re: [PATCH] pcm: Don't store the state for SND_PCM_STATE_SUSPENDED

[Takashi Iwai <tiwai@suse.de>]

On Tue, 24 May 2016 12:18:18 +0200,
Takashi Iwai wrote:
> 
> On Tue, 24 May 2016 12:12:49 +0200,
> Shengjiu Wang wrote:
> > 
> > Hi
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, May 20, 2016 10:32 PM
> > > To: Shengjiu Wang
> > > Cc: perex@perex.cz; alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] pcm: Don't store the state for
> > > SND_PCM_STATE_SUSPENDED
> > > 
> > > On Fri, 20 May 2016 12:46:37 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > On Fri, 20 May 2016 11:41:25 +0200,
> > > > Shengjiu Wang wrote:
> > > > >
> > > > > Hi Takashi
> > > > >
> > > > >    I tested your patch, after suspend and resume, the playback is
> > > stopped.
> > > > > It is caused by the DMA. DMA is not started after resume.
> > > > >
> > > > > With your patch, DMA is not terminated but then is re-started. The
> > > driver don't
> > > > > support this behavior.
> > > >
> > > > If so, it's simply a driver bug.  Blame the kernel driver instead.
> > > 
> > > Which driver did you see the problem?  We should fix it.
> > 
> > But my thought is when suspended, the dmaengine_pause() is called, then
> > dmaengine_resume() should be called in resume(). If there is no resume()
> > Just call the prepare() and start(), it seems not reasonable. What do
> > you think?
> 
> There are several ways to fix the problem, but the point is that, from
> the API POV, the direct state change from SUSPENDED to PREPARED is
> valid.  So, the kernel driver has to support such a state change, no
> matter how.
> 
> An easier way would be to add a check and some trigger in PCM core
> side.  OTOH, this would affect effectively all drivers, thus we'd need
> a wider test coverage, too.
> 
> Judging from your comment, the broken driver is ASoC one, right?

Thinking of this again, I'm inclined to have a workaround for such
buggy drivers.  In the end, alsa-lib should work for older kernels,
too.

Does the patch below work on your device?

Maybe better to clear the buffer beforehand for avoiding the
unnecessary noise.  But it can be done later.


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] pcm: dmix: resume workaround for buggy driver

The previous commit removed the whole handling of resume in dmix, but
this seems causing another regression; some buggy drivers assume that
the device-resume needs to be triggered before transitioning to
PREPARED state.  As an ugly workaround, in this patch, when the slave
PCM supports resume, snd_pcm_direct_resume() does resume of the slave
PCM but immediately drop the stream after that.  In that way, the
device is brought to the sane active state, then the apps can prepare
and restart the stream properly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm_direct.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 53c49929cb1f..169758d18a29 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -837,6 +837,21 @@ 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;
+
+	snd_pcm_direct_semaphore_down(dmix, DIRECT_IPC_SEM_CLIENT);
+	/* some buggy drivers require the device resumed before prepared;
+	 * when a device has RESUME flag and is in SUSPENDED state, resume
+	 * here but immediately drop to bring it to a sane active state.
+	 */
+	if ((dmix->spcm->info & SND_PCM_INFO_RESUME) &&
+	    snd_pcm_state(dmix->spcm) == SND_PCM_STATE_SUSPENDED) {
+		snd_pcm_resume(dmix->spcm);
+		snd_pcm_drop(dmix->spcm);
+		snd_pcm_direct_timer_stop(dmix);
+		snd_pcm_direct_clear_timer_queue(dmix);
+	}
+	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
 	return -ENOSYS;
 }
 
@@ -845,7 +860,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 | SND_PCM_INFO_RESUME);
+	spcm->info &= ~SND_PCM_INFO_PAUSE;
 
 	COPY_SLAVE(access);
 	COPY_SLAVE(format);
@@ -874,6 +889,8 @@ static void save_slave_setting(snd_pcm_direct_t *dmix, snd_pcm_t *spcm)
 	COPY_SLAVE(buffer_time);
 	COPY_SLAVE(sample_bits);
 	COPY_SLAVE(frame_bits);
+
+	dmix->shmptr->s.info &= ~SND_PCM_INFO_RESUME;
 }
 
 #undef COPY_SLAVE
-- 
2.8.3