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

[Shengjiu Wang]

Hi

> 
> 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 at suse.de]
> > > > Sent: Friday, May 20, 2016 10:32 PM
> > > > To: Shengjiu Wang
> > > > Cc: perex at perex.cz; alsa-devel at 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.
> 

I test this patch, there will be error after resume.

aplay: pcm_write:1940: write error: Input/output error


The reason is that the snd_pcm_state(dmix->spcm) is SETUP, the
snd_pcm_direct_prepare() won't do snd_pcm_prepare().

Best regards
Wang shengjiu
> 
> Takashi
> 
> ---
> From: Takashi Iwai <tiwai at 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 at 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