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

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?)

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

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.)

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.

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.
participants (4)
-
Dong Aisheng
-
Mark Brown
-
Russell King - ARM Linux
-
Shawn Guo