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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 11 May 2016 04:28:41 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, May 10, 2016 4:22 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 Tue, 10 May 2016 09:45:46 +0200,
> > Shengjiu Wang wrote:
> > >
> > > The resume function don't update the dmix->state, if store SUSPENDED
> > > state in snd_pcm_dmix_state, the write function after resume will
> > > return error -ESTRPIPE, because the snd_pcm_write_areas() will check
> > > the state of the pcm device.
> > > This patch remove the store SND_PCM_STATE_SUSPENDED state operation
> > > for dmix,dshare,dsnoop.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@freescale.com>
> > 
> > What's the exact problem you're seeing on surface?  Could illustrate
> > how the bug is triggered and how to reproduce easily?  It'll make
> > easier to understand what you're trying to fix.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> The aplay endlessly print " Suspended. Trying resume. Done." if suspend
> and resume in the middle of playback. Which is caused by this patch.
> 
> commit 8985742d91dbdd74b2f605374207473393454fff
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Fri Oct 30 17:13:50 2015 +0100
> 
>     pcm: dmix: Handle slave PCM xrun and unexpected states properly 
> 
> This patch store the SUSPENDED state to dmix->state, but after resume
> the dmix->state still is SUSPENDED, next write function will check the
> state, if state is SUSPENDED, it will return -ESTRPIPE, then the aplay
> will print another " Suspended. Trying resume. Done."  Then repeat this
> behavior again and again.

Thanks, this is exactly what I wanted to see in the changelog!
It's rather a regression, and it should be clearly mentioned.

Now about your fix: the problem is not about setting the correct
state at suspending.  It is suspended, so setting the right state
there is the correct behavior.  However, the counterpart, the resume,
is the culprit of this bug.  It misses the restore of the shadow
state.

Does the patch below work instead?


Takashi

---
diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
index 14de734d98eb..e28738b0de96 100644
--- a/src/pcm/pcm_direct.c
+++ b/src/pcm/pcm_direct.c
@@ -848,6 +848,7 @@ int snd_pcm_direct_resume(snd_pcm_t *pcm)
 		snd_pcm_start(dmix->spcm);
 		err = 0;
 	}
+	dmix->state = snd_pcm_state(dmix->spcm);
 	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
 	return err;
 }