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

[Takashi Iwai]

On Tue, 31 May 2016 09:30:49 +0200,
Shengjiu Wang wrote:
> 
> Hi
> 
> > 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?
> > 
> No, the driver is in dma folder, path is /drivers/dma/. We use the
> /drivers/dma/imx-sdma.c But the driver in community is old. We have
> updated the dma driver to support virtual-dma, just like the
> drivers/dma/omap-dma.c.

Then it's even less interesting to us.  The stuff should be upstreamed
at first :)

> The c->desc should be released in terminate_all() function, if it not
> Released, the issue_pending() will not go to start dma.
> 
> I can't find a good way to fix this issue in dma driver. 

Well it's a clearly driver bug.  Per API definition, there is no
guarantee that RESUME is performed always after SUSPEND, but it can be
DROP.  We may add some coverage in the alsa-lib like my patch, but it
doesn't guarantee that all user-space behave in that way.  So,
hardening the kernel behavior is inevitable.

In anyway, still waiting for your test result of my patch.


thanks,

Takashi