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

[Shengjiu Wang]

Hi

> 
> On Tue, 31 May 2016 11:27:39 +0200,
> Shengjiu Wang wrote:
> >
> > 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().
> 
> OK, one fix would be to allow SETUP in snd_pcm_direct_prepare().  I'll
> prepare it later.  Meanwhile, the prepare of the slave should be done
> immediately at resume, so it's good to call in
> snd_pcm_direct_resume().
> 
> Below is the revised version.  Give it a try.

I test this patch, it is ok. But I have some questions.

1. Why do you add snd_pcm_drop()? It seems only adding snd_pcm_resume(spcm)
can fix this issue also.
2. Does the snd_pcm_drop cause some several period data be dropped?
3. The return values -ENOSYS, always cause error print "Failed. Restarting
stream." in aplay. Can it be fixed?

Best regards
Wang shengjiu
> 
> 
> thanks,
> 
> Takashi
> 
> ---
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH v2] 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 | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c
> index 53c49929cb1f..343fd3c6da3c 100644
> --- a/src/pcm/pcm_direct.c
> +++ b/src/pcm/pcm_direct.c
> @@ -837,6 +837,27 @@ 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_t *spcm = dmix->spcm;
> +
> +	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 ((spcm->info & SND_PCM_INFO_RESUME) &&
> +	    snd_pcm_state(spcm) == SND_PCM_STATE_SUSPENDED) {
> +		snd_pcm_resume(spcm);
> +		snd_pcm_drop(spcm);
> +		snd_pcm_direct_timer_stop(dmix);
> +		snd_pcm_direct_clear_timer_queue(dmix);
> +		snd_pcm_areas_silence(snd_pcm_mmap_areas(spcm), 0,
> +				      spcm->channels, spcm->buffer_size,
> +				      spcm->format);
> +		snd_pcm_prepare(spcm);
> +		snd_pcm_start(spcm);
> +	}
> +	snd_pcm_direct_semaphore_up(dmix, DIRECT_IPC_SEM_CLIENT);
>  	return -ENOSYS;
>  }
> 
> @@ -845,7 +866,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 +895,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