Re: [RFC 00/10] ASoC: Introduce dmaengine pcm helper functions

Re: [RFC 00/10] ASoC: Introduce dmaengine pcm helper functions

[Shawn Guo]

Nice consolidation ...

On Tue, Feb 21, 2012 at 05:47:54PM +0100, Lars-Peter Clausen wrote:
> This patch series introduces a new set of helper functions for dmaengine based
> PCM drivers. Currently we have 6 different dmaengine based PCM drivers which are
> all more or less similar in structure, but all have also individual requirements
> which can't easily be generalized. So that's why this series does not introduce
> a generic dmaengine PCM driver but rather a set of helper functions which can be
> used to implement the common bits between such dmaengine based PCM drivers.
> 
> The helper functions only provide infrastructure code for dmaengines which
> provide thed evice_prep_dma_cyclic. While it is possible to add support for
> dmaengine, which only provide device_prep_slave_sg, it is in my opinion a better
> idea to either implement the missing functionality in the dmaengine driver or
> provide a emulation layer for cyclic transfer within the dmaengine framework
> itself. This has the advantage that it is also available outside of ALSA.
> 
> This series converts the three PCM drivers which already use the circular
> dmaengine prepare function. Since I don't have access to hardware using the
> converted drivers the patches have only been compile tested. But the helper
> functions introduced in this series have been runtime tested with a platform
> which is not upstream yet.
> 
> The first three patches are just cleanup patches which remove unused functions
> and struct fields and can probably applied without waiting for the rest of the
> series. The next two patches restructure the iMX and MXS drivers to request
> their DMA channel in the PCM open callback instead of the hwparams callback.
> This is done to make their structure more similar to what it will be when using
> the helper functions and make the actual patch converting them to the new
> dmaengine PCM helper functions less intrusive. The next patch introduces the
> helper functions and the last three patches respectively convert the imx, mxs
> and ep39xx dmaengine based PCM drivers to use the new set of helper functions.
> 
> The set of helper functions consists of a open, a close, a trigger and a pointer
> callback and also a function to convert hwparams to a dma_slave_config. The
> trigger and pointer callbacks can usually be used directly for the pcm_ops
> callbacks. The open callback wants to be called from a driver specific open
> function which may do some driver specific setup tasks and provide the helper
> open callback with a dma filter function. If the driver specific open functions
> allocates resources they need to be freed in a driver specific close function
> which has to call the helper close callback. If not the helper close callback
> can be assigned directly to the pcm_ops struct.
> 
> If a driver needs to keep additional driver specific data around it can be done
> by using snd_dmaengine_pcm_{get,set}_data. Normally a driver should not require
> to keep additional data around, but all of the converted drivers need this,
> because of this horrible abusive pattern where the dma channels private field
> gets assigned configuration data in the filter callback. We should hopefully be
> able to get rid of this pattern with Guennadi Liakhovetski's patches[1], which
> add a slave configuration parameter to dma_request_channel, and with it the need
> for storing extra private data.
> 
> While the DMA helper functions only use generic ALSA functions and nothing ASoC
> specific I've placed the, under ASoC for now since the only users are ASoC
> driver.
> 
> - Lars
> 
> [1] https://lkml.org/lkml/2012/2/1/227
> 
> Lars-Peter Clausen (10):
>   ASoC: imx-pcm: Remove empty prepare callback
>   ASoC: imx-pcm: Remove unused fields from imx_pcm_runtime_data struct
>   ASoC: mxs-pcm: Remove unused fields from struct mxs_pcm_runtime_data
>   ASoC: imx-ssi: Set dma data early
>   ASoC: imx-pcm: Request DMA channel early
>   ASoC: mxs-pxm: Request DMA channel early

s/mxs-pxm/mxs-pcm

>   ASoC: Add dmaengine PCM helper functions
>   ASoC: imx-dma: Use dmaengine PCM helper functions

s/imx-dma/imx-pcm

>   ASoC: mxs-pcm: Use dmaengine PCM helper functions
>   ASoC: ep93xx-pcm: Use dmaengine PCM helper functions
> 
>  include/sound/dmaengine_pcm.h   |   49 +++++++
>  sound/soc/Kconfig               |    3 +
>  sound/soc/Makefile              |    3 +
>  sound/soc/ep93xx/Kconfig        |    1 +
>  sound/soc/ep93xx/ep93xx-pcm.c   |  148 +++-----------------
>  sound/soc/imx/Kconfig           |    1 +
>  sound/soc/imx/imx-pcm-dma-mx2.c |  216 ++++-------------------------
>  sound/soc/imx/imx-ssi.c         |   28 +++-
>  sound/soc/mxs/Kconfig           |    1 +
>  sound/soc/mxs/mxs-pcm.c         |  157 ++++------------------
>  sound/soc/mxs/mxs-pcm.h         |   16 --
>  sound/soc/soc-dmaengine-pcm.c   |  287 +++++++++++++++++++++++++++++++++++++++
>  12 files changed, 443 insertions(+), 467 deletions(-)
>  create mode 100644 include/sound/dmaengine_pcm.h
>  create mode 100644 sound/soc/soc-dmaengine-pcm.c
> 
I tested it on both MXS (i.MX28) and IMX (i.MX51), it works fine, so

Tested-by: Shawn Guo <shawn.guo@linaro.org>

However, when I play the series on today's linux-next, I found a patch
from Russell (Cc-ed) below doing something overlapping with the work
here.

commit 4c815a835e9b0231028e9cc303406b381bfff7a2
Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Wed Jan 18 13:01:26 2012 +0000

    ALSA: ASoC: add generic slave-only DMA engine support

    Add a generic DMA engine platform driver to ASoC.  This allows a range
    of DMA engines to be used to transfer data to a CPUs audio interface
    without having to rewrite this code many times.

    This driver only supports DMA_SLAVE channels, not DMA_CYCLIC channels.
    DMA_CYCLIC channels could be added at a later date.

    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

I did not notice this patch on any list yet, so am not sure about
Russell's plan about it.  But it seems this series does not reach
alsa-devel list either. (forgot?)

-- 
Regards,
Shawn

Re: [RFC 00/10] ASoC: Introduce dmaengine pcm helper functions

[Dong Aisheng]

On Wed, Feb 22, 2012 at 04:19:10PM +0800, Shawn Guo wrote:
> Nice consolidation ...
> 
> On Tue, Feb 21, 2012 at 05:47:54PM +0100, Lars-Peter Clausen wrote:
> > This patch series introduces a new set of helper functions for dmaengine based
> > PCM drivers. Currently we have 6 different dmaengine based PCM drivers which are
> > all more or less similar in structure, but all have also individual requirements
Correct.

> > which can't easily be generalized. So that's why this series does not introduce
> > a generic dmaengine PCM driver but rather a set of helper functions which can be
> > used to implement the common bits between such dmaengine based PCM drivers.
> > 
I once had the same thinking about it...

> I did not notice this patch on any list yet, so am not sure about
> Russell's plan about it.  But it seems this series does not reach
> alsa-devel list either. (forgot?)
> 
I'm also interested in this patch series.
However, i can not see it in the alsa-dev list....


Regards
Dong Aisheng

Re: [RFC 00/10] ASoC: Introduce dmaengine pcm helper functions

[Russell King - ARM Linux]

On Wed, Feb 22, 2012 at 04:19:10PM +0800, Shawn Guo wrote:
> Nice consolidation ...
> 
> On Tue, Feb 21, 2012 at 05:47:54PM +0100, Lars-Peter Clausen wrote:
> > This patch series introduces a new set of helper functions for dmaengine based
> > PCM drivers. Currently we have 6 different dmaengine based PCM drivers which are
> > all more or less similar in structure, but all have also individual requirements
> > which can't easily be generalized.

I have developed a standard DMA engine ASoC driver tested on SA11x0 for
playback only (that's because SA11x0 requires playback DMA to be active
for capture to work, and ASoC doesn't support that.)

It uses the standard scatterlist DMA submission stuff, but could be
expanded to use the cyclic stuff if available.

> I did not notice this patch on any list yet, so am not sure about
> Russell's plan about it.  But it seems this series does not reach
> alsa-devel list either. (forgot?)

I've not yet posted it, mainly because it's there by accident, along with
the rest of the SA11x0 Assabet stuff (it's part of my testing for the
SA11x0 DMA engine code.)

Re: [RFC 00/10] ASoC: Introduce dmaengine pcm helper functions

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1647 bytes --]

On Wed, Feb 22, 2012 at 09:03:07AM +0000, Russell King - ARM Linux wrote:

> I have developed a standard DMA engine ASoC driver tested on SA11x0 for
> playback only (that's because SA11x0 requires playback DMA to be active
> for capture to work, and ASoC doesn't support that.)

Eew, that does sound like a rather spectacular hardware fail.  What's
the root of the restriction?  Since I've never heard of any other
hardware with a similar requirement and would be surprised to see any
I'd be comfortable with a driver specific bodge to keep playback
running whenever there's a capture.

> I've not yet posted it, mainly because it's there by accident, along with
> the rest of the SA11x0 Assabet stuff (it's part of my testing for the
> SA11x0 DMA engine code.)

Can you guys take a look at each other's code and see what the overlap
is please?  Looking through it seems like the final result probably
wants to be a merge of both.

It'd also be nice to get as much as we can of the sa11x0 support merged,
glancing at the code there's quite a few updates I'd like to see like
more use of devm_ and moving over to using snd_soc_register_card() (you
shouldn't be registering any devices at all in your machine driver,
though I've no idea what's going on with L3, perhaps you need to
register something for that).

For the suspend/resume stuff you can make any assumptions you like about
the hardware setup in the machine driver suspend and resume callbacks
since all the code is specific to a particular system.  You can also
assume that the hardware is not in use over suspend and resume, the core
should quiesce the hardware before it suspends it.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC 00/10] ASoC: Introduce dmaengine pcm helper functions

[Russell King - ARM Linux]

On Wed, Feb 22, 2012 at 10:50:04AM +0000, Mark Brown wrote:
> On Wed, Feb 22, 2012 at 09:03:07AM +0000, Russell King - ARM Linux wrote:
> 
> > I have developed a standard DMA engine ASoC driver tested on SA11x0 for
> > playback only (that's because SA11x0 requires playback DMA to be active
> > for capture to work, and ASoC doesn't support that.)
> 
> Eew, that does sound like a rather spectacular hardware fail.  What's
> the root of the restriction?  Since I've never heard of any other
> hardware with a similar requirement and would be surprised to see any
> I'd be comfortable with a driver specific bodge to keep playback
> running whenever there's a capture.

It comes from the fact that the SSP interface only clocks when there's
DMA _to_ the transmit side.  It's a SSP (Microwire/SPI etc) interface
which has been adapted through a FPGA to drive audio codecs.  As such,
every SA11x0 based platform does this.

> > I've not yet posted it, mainly because it's there by accident, along with
> > the rest of the SA11x0 Assabet stuff (it's part of my testing for the
> > SA11x0 DMA engine code.)
> 
> Can you guys take a look at each other's code and see what the overlap
> is please?  Looking through it seems like the final result probably
> wants to be a merge of both.
> 
> It'd also be nice to get as much as we can of the sa11x0 support merged,
> glancing at the code there's quite a few updates I'd like to see like
> more use of devm_ and moving over to using snd_soc_register_card() (you
> shouldn't be registering any devices at all in your machine driver,
> though I've no idea what's going on with L3, perhaps you need to
> register something for that).

L3 is a total abortion - read the comments in the assabet code about it.
The L3 clock and data pins share the I2C bus pins on the assabet.  This
is something that my original L3 driver supported but that's long since
bitten the dust.  Moreover, the ALSA L3 stuff doesn't allow it to be used
with the SA1111 companion device, because that bus isn't bitbanged.  Again,
that's something my original L3 code supported.

Getting that sorted properly is going to be not nice on assabet because it
requires a lock to be shared between the L3 and I2C code.  I don't think
it's worth doing for just one platform.