Re: [alsa-devel] [PATCH 2/4] ASoC: mmp: add audio dma support
On Fri, 2012-05-25 at 15:11 +0800, Zhangfei Gao wrote:
mmp-pcm handle audio dma based on dmaengine Support mmp and pxa910
Looks like this is *not* using soc-dmaengine library, why?
Signed-off-by: Zhangfei Gao zhangfei.gao@marvell.com Signed-off-by: Leo Yan leoy@marvell.com Signed-off-by: Qiao Zhou zhouqiao@marvell.com
include/linux/platform_data/mmp_audio.h | 22 ++ sound/soc/pxa/Kconfig | 8 + sound/soc/pxa/Makefile | 2 + sound/soc/pxa/mmp-pcm.c | 448 +++++++++++++++++++++++++++++++ 4 files changed, 480 insertions(+), 0 deletions(-) create mode 100644 include/linux/platform_data/mmp_audio.h create mode 100644 sound/soc/pxa/mmp-pcm.c
diff --git a/include/linux/platform_data/mmp_audio.h b/include/linux/platform_data/mmp_audio.h new file mode 100644 index 0000000..0f25d16 --- /dev/null +++ b/include/linux/platform_data/mmp_audio.h @@ -0,0 +1,22 @@ +/*
- MMP Platform AUDIO Management
- Copyright (c) 2011 Marvell Semiconductors Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- */
+#ifndef MMP_AUDIO_H +#define MMP_AUDIO_H
+struct mmp_audio_platdata {
- u32 period_max_capture;
- u32 buffer_max_capture;
- u32 period_max_playback;
- u32 buffer_max_playback;
+};
+#endif /* MMP_AUDIO_H */ diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig index a0f7d3c..a516068 100644 --- a/sound/soc/pxa/Kconfig +++ b/sound/soc/pxa/Kconfig @@ -8,6 +8,14 @@ config SND_PXA2XX_SOC the PXA2xx AC97, I2S or SSP interface. You will also need to select the audio interfaces to support below.
+config SND_MMP_SOC
- bool "Soc Audio for Marvell MMP chips"
- depends on ARCH_MMP
- select SND_ARM
- help
Say Y if you want to add support for codecs attached tothe MMP SSPA interface.config SND_PXA2XX_AC97 tristate select SND_AC97_CODEC diff --git a/sound/soc/pxa/Makefile b/sound/soc/pxa/Makefile index af35762..f913e9b 100644 --- a/sound/soc/pxa/Makefile +++ b/sound/soc/pxa/Makefile @@ -3,11 +3,13 @@ snd-soc-pxa2xx-objs := pxa2xx-pcm.o snd-soc-pxa2xx-ac97-objs := pxa2xx-ac97.o snd-soc-pxa2xx-i2s-objs := pxa2xx-i2s.o snd-soc-pxa-ssp-objs := pxa-ssp.o +snd-soc-mmp-objs := mmp-pcm.o
obj-$(CONFIG_SND_PXA2XX_SOC) += snd-soc-pxa2xx.o obj-$(CONFIG_SND_PXA2XX_SOC_AC97) += snd-soc-pxa2xx-ac97.o obj-$(CONFIG_SND_PXA2XX_SOC_I2S) += snd-soc-pxa2xx-i2s.o obj-$(CONFIG_SND_PXA_SOC_SSP) += snd-soc-pxa-ssp.o +obj-$(CONFIG_SND_MMP_SOC) += snd-soc-mmp.o
# PXA Machine Support snd-soc-corgi-objs := corgi.o diff --git a/sound/soc/pxa/mmp-pcm.c b/sound/soc/pxa/mmp-pcm.c new file mode 100644 index 0000000..abafbb9 --- /dev/null +++ b/sound/soc/pxa/mmp-pcm.c @@ -0,0 +1,448 @@ +/*
- linux/sound/soc/pxa/mmp-pcm.c
- Copyright (C) 2011 Marvell International Ltd.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- */
+#include <linux/module.h> +#include <linux/init.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/dma-mapping.h> +#include <linux/dmaengine.h> +#include <linux/platform_data/mmp_dma.h> +#include <linux/platform_data/mmp_audio.h> +#include <sound/pxa2xx-lib.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <mach/sram.h>
+struct mmp_runtime_data {
- int ssp_id;
- u32 period_size;
- u32 pointer;
- struct resource *dma_res;
- struct mmp_tdma_data tdma_data;
- struct gen_pool *gpool;
- struct snd_pcm_substream *substream;
- struct dma_chan *dma_chan;
- struct dma_async_tx_descriptor *desc;
+};
+#define MMP_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
SNDRV_PCM_INFO_MMAP_VALID | \SNDRV_PCM_INFO_INTERLEAVED | \SNDRV_PCM_INFO_PAUSE | \SNDRV_PCM_INFO_RESUME)+#define MMP_PCM_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
SNDRV_PCM_FMTBIT_S24_LE | \SNDRV_PCM_FMTBIT_S32_LE)+static struct snd_pcm_hardware mmp_pcm_hardware[] = {
- {
.info = MMP_PCM_INFO,.formats = MMP_PCM_FORMATS,.period_bytes_min = 1024,.period_bytes_max = 2048,.periods_min = 2,.periods_max = 32,.buffer_bytes_max = 4096,.fifo_size = 32,- },
- {
.info = MMP_PCM_INFO,.formats = MMP_PCM_FORMATS,.period_bytes_min = 1024,.period_bytes_max = 2048,.periods_min = 2,.periods_max = 32,.buffer_bytes_max = 4096,.fifo_size = 32,- },
+};
+static void mmp_pcm_adma_irq(void *data) +{
- struct snd_pcm_substream *substream = data;
- struct mmp_runtime_data *prtd = substream->runtime->private_data;
- size_t dma_bytes = substream->runtime->dma_bytes;
- prtd->pointer = (prtd->pointer + prtd->period_size) % dma_bytes;
- snd_pcm_period_elapsed(substream);
- return;
+}
+static bool filter(struct dma_chan *chan, void *param) +{
- struct mmp_runtime_data *prtd = param;
- bool found = false;
- char *devname;
- devname = kasprintf(GFP_KERNEL, "%s.%d", prtd->dma_res->name,
prtd->ssp_id);- if ((strcmp(dev_name(chan->device->dev), devname) == 0) &&
(chan->chan_id == prtd->dma_res->start)) {chan->private = &prtd->tdma_data;found = true;- }
- kfree(devname);
- return found;
+}
+static int mmp_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct mmp_runtime_data *prtd = runtime->private_data;
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct pxa2xx_pcm_dma_params *dma_params;
- size_t totsize = params_buffer_bytes(params);
- size_t period = params_period_bytes(params);
- int ret;
- struct dma_slave_config slave_config;
- dma_cap_mask_t mask;
- dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
- if (!dma_params)
return 0;- switch (params_format(params)) {
- case SNDRV_PCM_FORMAT_S8:
prtd->tdma_data.bus_size = 8;break;- case SNDRV_PCM_FORMAT_S16_LE:
prtd->tdma_data.bus_size = 16;break;- case SNDRV_PCM_FORMAT_S24_LE:
prtd->tdma_data.bus_size = 24;break;- case SNDRV_PCM_FORMAT_S32_LE:
prtd->tdma_data.bus_size = 32;break;- default:
return -EINVAL;- }
- prtd->tdma_data.pack_mod = true;
- dma_cap_zero(mask);
- dma_cap_set(DMA_CYCLIC, mask);
- prtd->dma_chan = dma_request_channel(mask, filter, prtd);
- if (!prtd->dma_chan)
return -EINVAL;- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
slave_config.direction = DMA_TO_DEVICE;slave_config.dst_addr = dma_params->dev_addr;slave_config.dst_maxburst = 4;- } else {
slave_config.direction = DMA_FROM_DEVICE;slave_config.src_addr = dma_params->dev_addr;slave_config.src_maxburst = 4;- }
- ret = dmaengine_slave_config(prtd->dma_chan, &slave_config);
- if (ret)
return ret;- snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
- prtd->period_size = period;
- runtime->dma_bytes = totsize;
- prtd->desc = prtd->dma_chan->device->device_prep_dma_cyclic(
prtd->dma_chan, runtime->dma_addr, totsize, period,substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?DMA_TO_DEVICE : DMA_FROM_DEVICE, NULL);- if (!prtd->desc) {
dev_err(&prtd->dma_chan->dev->device, "cannot prepare slave dma\n");return -EINVAL;- }
- prtd->desc->callback = mmp_pcm_adma_irq;
- prtd->desc->callback_param = substream;
- dmaengine_submit(prtd->desc);
- return 0;
+}
+static int mmp_pcm_hw_free(struct snd_pcm_substream *substream) +{
- struct mmp_runtime_data *prtd = substream->runtime->private_data;
- if (prtd->dma_chan) {
dma_release_channel(prtd->dma_chan);prtd->dma_chan = NULL;- }
- return 0;
+}
+static int mmp_pcm_trigger(struct snd_pcm_substream *substream, int cmd) +{
- struct mmp_runtime_data *prtd = substream->runtime->private_data;
- int ret = 0;
- switch (cmd) {
- case SNDRV_PCM_TRIGGER_START:
prtd->pointer = 0;dma_async_issue_pending(prtd->dma_chan);break;- case SNDRV_PCM_TRIGGER_STOP:
- case SNDRV_PCM_TRIGGER_SUSPEND:
- case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
dmaengine_terminate_all(prtd->dma_chan);break;- case SNDRV_PCM_TRIGGER_RESUME:
dma_async_issue_pending(prtd->dma_chan);break;- case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
dma_async_issue_pending(prtd->dma_chan);break;- default:
ret = -EINVAL;break;- }
- return ret;
+}
+static snd_pcm_uframes_t mmp_pcm_pointer(struct snd_pcm_substream *substream) +{
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct mmp_runtime_data *prtd = runtime->private_data;
- snd_pcm_uframes_t x;
- x = bytes_to_frames(runtime, prtd->pointer);
- if (x == runtime->buffer_size)
x = 0;- return x;
+}
+static int mmp_pcm_open(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct platform_device *pdev = to_platform_device(rtd->platform->dev);
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- struct mmp_runtime_data *prtd;
- struct resource *r;
- int ret;
- r = platform_get_resource(pdev, IORESOURCE_DMA, substream->stream);
- if (!r)
return -EBUSY;- snd_soc_set_runtime_hwparams(substream,
&mmp_pcm_hardware[substream->stream]);- /*
* For mysterious reasons (and despite what the manual says)* playback samples are lost if the DMA count is not a multiple* of the DMA burst size. Let's add a rule to enforce that.*/- ret = snd_pcm_hw_constraint_step(runtime, 0,
SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32);- if (ret)
goto out;- ret = snd_pcm_hw_constraint_step(runtime, 0,
SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32);- if (ret)
goto out;- ret = snd_pcm_hw_constraint_integer(runtime,
SNDRV_PCM_HW_PARAM_PERIODS);- if (ret < 0)
goto out;- prtd = devm_kzalloc(&pdev->dev,
sizeof(struct mmp_runtime_data), GFP_KERNEL);- if (prtd == NULL) {
ret = -ENOMEM;goto out;- }
- prtd->substream = substream;
- runtime->private_data = prtd;
- prtd->dma_res = r;
- prtd->ssp_id = cpu_dai->id;
- return 0;
+out:
- return ret;
+}
+static int mmp_pcm_close(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct platform_device *pdev = to_platform_device(rtd->platform->dev);
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct mmp_runtime_data *prtd = runtime->private_data;
- devm_kfree(&pdev->dev, prtd);
- runtime->private_data = NULL;
- return 0;
+}
+static int mmp_pcm_mmap(struct snd_pcm_substream *substream,
struct vm_area_struct *vma)+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- unsigned long off = vma->vm_pgoff;
- vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
- return remap_pfn_range(vma, vma->vm_start,
__phys_to_pfn(runtime->dma_addr) + off,vma->vm_end - vma->vm_start, vma->vm_page_prot);+}
+struct snd_pcm_ops mmp_pcm_ops = {
- .open = mmp_pcm_open,
- .close = mmp_pcm_close,
- .ioctl = snd_pcm_lib_ioctl,
- .hw_params = mmp_pcm_hw_params,
- .hw_free = mmp_pcm_hw_free,
- .trigger = mmp_pcm_trigger,
- .pointer = mmp_pcm_pointer,
- .mmap = mmp_pcm_mmap,
+};
+static int mmp_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream) +{
- struct snd_pcm_substream *substream = pcm->streams[stream].substream;
- struct snd_dma_buffer *buf = &substream->dma_buffer;
- size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
- struct gen_pool *gpool;
- buf->dev.type = SNDRV_DMA_TYPE_DEV;
- buf->dev.dev = pcm->card->dev;
- buf->private_data = NULL;
- gpool = sram_get_gpool("asram");
- if (!gpool)
return -ENOMEM;- buf->area = (unsigned char *)gen_pool_alloc(gpool, size);
- if (!buf->area)
return -ENOMEM;- buf->addr = gen_pool_virt_to_phys(gpool, (unsigned long)buf->area);
- buf->bytes = size;
- return 0;
+}
+static void mmp_pcm_free_dma_buffers(struct snd_pcm *pcm) +{
- struct snd_pcm_substream *substream;
- struct snd_dma_buffer *buf;
- int stream;
- struct gen_pool *gpool;
- gpool = sram_get_gpool("asram");
- if (!gpool)
return;- for (stream = 0; stream < 2; stream++) {
size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;substream = pcm->streams[stream].substream;if (!substream)continue;buf = &substream->dma_buffer;if (!buf->area)continue;gen_pool_free(gpool, (unsigned long)buf->area, size);buf->area = NULL;- }
- return;
+}
+static u64 mmp_pcm_dmamask = DMA_BIT_MASK(64);
+int mmp_pcm_new(struct snd_soc_pcm_runtime *rtd) +{
- struct snd_card *card = rtd->card->snd_card;
- struct snd_pcm *pcm = rtd->pcm;
- int ret = 0;
- if (!card->dev->dma_mask)
card->dev->dma_mask = &mmp_pcm_dmamask;- if (!card->dev->coherent_dma_mask)
card->dev->coherent_dma_mask = DMA_BIT_MASK(64);- if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) {
ret = mmp_pcm_preallocate_dma_buffer(pcm,SNDRV_PCM_STREAM_PLAYBACK);if (ret)goto out;- }
- if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
ret = mmp_pcm_preallocate_dma_buffer(pcm,SNDRV_PCM_STREAM_CAPTURE);if (ret)goto out;- }
- out:
- return ret;
+}
+struct snd_soc_platform_driver mmp_soc_platform = {
- .ops = &mmp_pcm_ops,
- .pcm_new = mmp_pcm_new,
- .pcm_free = mmp_pcm_free_dma_buffers,
+};
+static __devinit int mmp_pcm_probe(struct platform_device *pdev) +{
- struct mmp_audio_platdata *pdata = pdev->dev.platform_data;
- if (pdata) {
mmp_pcm_hardware[SNDRV_PCM_STREAM_PLAYBACK].buffer_bytes_max =pdata->buffer_max_playback;mmp_pcm_hardware[SNDRV_PCM_STREAM_PLAYBACK].period_bytes_max =pdata->period_max_playback;mmp_pcm_hardware[SNDRV_PCM_STREAM_CAPTURE].buffer_bytes_max =pdata->buffer_max_capture;mmp_pcm_hardware[SNDRV_PCM_STREAM_CAPTURE].period_bytes_max =pdata->period_max_capture;- }
- return snd_soc_register_platform(&pdev->dev, &mmp_soc_platform);
+}
+static int __devexit mmp_pcm_remove(struct platform_device *pdev) +{
- snd_soc_unregister_platform(&pdev->dev);
- return 0;
+}
+static struct platform_driver mmp_pcm_driver = {
- .driver = {
.name = "mmp-pcm-audio",.owner = THIS_MODULE,- },
- .probe = mmp_pcm_probe,
- .remove = __devexit_p(mmp_pcm_remove),
+};
+module_platform_driver(mmp_pcm_driver);
+MODULE_AUTHOR("Leo Yan leoy@marvell.com"); +MODULE_DESCRIPTION("MMP Soc Audio DMA module"); +MODULE_LICENSE("GPL");
On Fri, May 25, 2012 at 01:23:36PM +0530, Vinod Koul wrote:
On Fri, 2012-05-25 at 15:11 +0800, Zhangfei Gao wrote:
mmp-pcm handle audio dma based on dmaengine Support mmp and pxa910
Looks like this is *not* using soc-dmaengine library, why?
Note also...
- prtd->dma_chan = dma_request_channel(mask, filter, prtd);
- if (!prtd->dma_chan)
return -EINVAL;
This should be done at probe time, so we know the struct device, so that...
+static int mmp_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream) +{
- struct snd_pcm_substream *substream = pcm->streams[stream].substream;
- struct snd_dma_buffer *buf = &substream->dma_buffer;
- size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
- struct gen_pool *gpool;
- buf->dev.type = SNDRV_DMA_TYPE_DEV;
- buf->dev.dev = pcm->card->dev;
... this uses the right device, and...
+static u64 mmp_pcm_dmamask = DMA_BIT_MASK(64);
+int mmp_pcm_new(struct snd_soc_pcm_runtime *rtd) +{
- struct snd_card *card = rtd->card->snd_card;
- struct snd_pcm *pcm = rtd->pcm;
- int ret = 0;
- if (!card->dev->dma_mask)
card->dev->dma_mask = &mmp_pcm_dmamask;- if (!card->dev->coherent_dma_mask)
card->dev->coherent_dma_mask = DMA_BIT_MASK(64);
... we don't need crap like this.
Because then we'll be allocating buffers against the _right_ struct device which is the DMA engine struct device. -- To unsubscribe from this list: send the line "unsubscribe alsa-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Russell and Vinod.
On Fri, May 25, 2012 at 4:05 PM, Russell King - ARM Linux < linux@arm.linux.org.uk> wrote:
On Fri, May 25, 2012 at 01:23:36PM +0530, Vinod Koul wrote:
On Fri, 2012-05-25 at 15:11 +0800, Zhangfei Gao wrote:
mmp-pcm handle audio dma based on dmaengine Support mmp and pxa910
Looks like this is *not* using soc-dmaengine library, why?
Note also...
Will look into soc-dmaengine.
- prtd->dma_chan = dma_request_channel(mask, filter, prtd);
- if (!prtd->dma_chan)
return -EINVAL;This should be done at probe time, so we know the struct device, so that...
Do you mean at open time, like snd_dmaengine_pcm_open. The channel resource is limited and better get dynamically. As a result the pcm_new and preallocate already called before.
+static int mmp_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int
stream)
+{
- struct snd_pcm_substream *substream =
pcm->streams[stream].substream;
- struct snd_dma_buffer *buf = &substream->dma_buffer;
- size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
- struct gen_pool *gpool;
- buf->dev.type = SNDRV_DMA_TYPE_DEV;
- buf->dev.dev = pcm->card->dev;
... this uses the right device, and...
+static u64 mmp_pcm_dmamask = DMA_BIT_MASK(64);
+int mmp_pcm_new(struct snd_soc_pcm_runtime *rtd) +{
- struct snd_card *card = rtd->card->snd_card;
- struct snd_pcm *pcm = rtd->pcm;
- int ret = 0;
- if (!card->dev->dma_mask)
card->dev->dma_mask = &mmp_pcm_dmamask;- if (!card->dev->coherent_dma_mask)
card->dev->coherent_dma_mask = DMA_BIT_MASK(64);... we don't need crap like this.
Because then we'll be allocating buffers against the _right_ struct device which is the DMA engine struct device. -- To unsubscribe from this list: send the line "unsubscribe alsa-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, May 25, 2012 at 04:47:20PM +0800, zhangfei gao wrote:
Do you mean at open time, like snd_dmaengine_pcm_open. The channel resource is limited and better get dynamically. As a result the pcm_new and preallocate already called before.
This is where dealing with slave DMA channels in a virtualized setup becomes a far better solution than trying to assign a particular physical channel at request time.
What we may wish to think about is having a way for slave drivers to assert to DMA engine the priority of a channel, which they can change dynamically according to what they're doing. Eg, an ALSA driver would leave the channel low priority while it's not expecting to be used, but as soon as we see the prepare call, set it to high priority.
The DMA engine driver could use that to decide to assign a physical channel to the virtual channel, so that DMA can start as soon as possible even with other activity on the DMA engine.
However, I've yet to see any setup where the number of physical DMA channels available exceeds the number of actual _simultaneous_ users. Even with the five channels on SA11x0 shared between 12? peripherals, with my DMA engine driver I've only seen one or two physical channels being used simultaneously.
On Fri, May 25, 2012 at 5:42 PM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, May 25, 2012 at 04:47:20PM +0800, zhangfei gao wrote:
Do you mean at open time, like snd_dmaengine_pcm_open. The channel resource is limited and better get dynamically. As a result the pcm_new and preallocate already called before.
This is where dealing with slave DMA channels in a virtualized setup becomes a far better solution than trying to assign a particular physical channel at request time.
What we may wish to think about is having a way for slave drivers to assert to DMA engine the priority of a channel, which they can change dynamically according to what they're doing. Eg, an ALSA driver would leave the channel low priority while it's not expecting to be used, but as soon as we see the prepare call, set it to high priority.
The DMA engine driver could use that to decide to assign a physical channel to the virtual channel, so that DMA can start as soon as possible even with other activity on the DMA engine.
However, I've yet to see any setup where the number of physical DMA channels available exceeds the number of actual _simultaneous_ users. Even with the five channels on SA11x0 shared between 12? peripherals, with my DMA engine driver I've only seen one or two physical channels being used simultaneously.
When debugging with putting snd_dmaengine_pcm_open to probe or pcm_new, there is issue. snd_dmaengine_pcm_open -> snd_pcm_hw_constraint_integer(substream->runtime, SNDRV_PCM_HW_PARAM_PERIODS); The runtime is only exist at runtime. So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has to be put in open instead of probe.
Thanks
On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
When debugging with putting snd_dmaengine_pcm_open to probe or pcm_new, there is issue. snd_dmaengine_pcm_open -> snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS); The runtime is only exist at runtime. So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has to be put in open instead of probe.
I think you got it wrong, the snd_dmaengine_pcm_open needs to be called from your CPU pcm_open. See other examples how this is used in other drivers.
On Tue, May 29, 2012 at 10:48:54AM +0530, Vinod Koul wrote:
On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
When debugging with putting snd_dmaengine_pcm_open to probe or pcm_new, there is issue. snd_dmaengine_pcm_open -> snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS); The runtime is only exist at runtime. So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has to be put in open instead of probe.
I think you got it wrong, the snd_dmaengine_pcm_open needs to be called from your CPU pcm_open. See other examples how this is used in other drivers.
But, as I pointed out earlier, there's the issue of using the wrong struct device to allocate memory for the DMA engine. That's something that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
On Tue, May 29, 2012 at 3:33 PM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Tue, May 29, 2012 at 10:48:54AM +0530, Vinod Koul wrote:
On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
When debugging with putting snd_dmaengine_pcm_open to probe or pcm_new, there is issue. snd_dmaengine_pcm_open -> snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS); The runtime is only exist at runtime. So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has to be put in open instead of probe.
I think you got it wrong, the snd_dmaengine_pcm_open needs to be called from your CPU pcm_open. See other examples how this is used in other drivers.
But, as I pointed out earlier, there's the issue of using the wrong struct device to allocate memory for the DMA engine. That's something that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
Could you help clarify what's the struct device should be used? Any example, not find soc-dmaengine.c. When debug, rtd->card->snd_card->dev is same as substream->pcm->card->dev, which we using now.
Thanks a lot.
On Tue, May 29, 2012 at 03:57:18PM +0800, zhangfei gao wrote:
On Tue, May 29, 2012 at 3:33 PM, Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Tue, May 29, 2012 at 10:48:54AM +0530, Vinod Koul wrote:
On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
When debugging with putting snd_dmaengine_pcm_open to probe or pcm_new, there is issue. snd_dmaengine_pcm_open -> snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS); The runtime is only exist at runtime. So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has to be put in open instead of probe.
I think you got it wrong, the snd_dmaengine_pcm_open needs to be called from your CPU pcm_open. See other examples how this is used in other drivers.
But, as I pointed out earlier, there's the issue of using the wrong struct device to allocate memory for the DMA engine. That's something that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
Could you help clarify what's the struct device should be used? Any example, not find soc-dmaengine.c. When debug, rtd->card->snd_card->dev is same as substream->pcm->card->dev, which we using now.
When your intention is to DMA to/from some memory, that memory _should_ be mapped via the DMA API (or, in the case of dma_alloc_coherent, allocated) _using_ the device associated with the agent performing the DMA.
This is so that the DMA API can know whether there's an IOMMU that has to be programmed for the DMA agent to be able to see the memory, or whether there's restrictions on the range of RAM which is visible to the DMA agent. -- To unsubscribe from this list: send the line "unsubscribe alsa-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2012 at 08:33:34AM +0100, Russell King - ARM Linux wrote:
But, as I pointed out earlier, there's the issue of using the wrong struct device to allocate memory for the DMA engine. That's something that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
What is the issue with the current code - it *looks* like you want to use the component device for the DMA as the struct device with dmaengine but I don't understand the issue that's created if we don't (and why things appear to be working for people as they are).
On Tue, May 29, 2012 at 10:02:15AM +0100, Mark Brown wrote:
On Tue, May 29, 2012 at 08:33:34AM +0100, Russell King - ARM Linux wrote:
But, as I pointed out earlier, there's the issue of using the wrong struct device to allocate memory for the DMA engine. That's something that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
What is the issue with the current code - it *looks* like you want to use the component device for the DMA as the struct device with dmaengine but I don't understand the issue that's created if we don't (and why things appear to be working for people as they are).
Look. It's very very very very simple.
What does the DMA API take? A struct device. What struct device? Some random struct device for something in the system, or what? No, it takes the struct device for the _device_ in the system which is _performing_ the DMA.
If your DMA is offloaded onto anther device, as is the case with DMA engine, the _correct_ struct device to use with the DMA API is the DMA engine device structure. Because, if there's any restrictions on how that memory is accessed - as I said in my other email - for instance, whether it be that the DMA engine has an IOMMU in the way, or maybe has a restricted view of memory - then your local struct device is the wrong one to be using, because it will _not_ have that information to convey into the DMA API.
Therefore, allocating or mapping DMA memory against one struct device, and then passing it to an entirely different device in the system to perform DMA on is _WRONG_.
It _may_ work as long as there are no restrictions or IOMMUs in the way, because you'll happen to setup your local struct device the same as the DMA engine struct device. That's not always going to be the case.
If you persist in not wanting to care about this, then I'm afraid I'll ignore soc-dmaengine-pcm.c entirely as to me its totally bolloxed code. What I have as my own version (now with cyclic DMA support) is IMHO a far superior and more correct implementation.
On 05/29/2012 11:21 AM, Russell King - ARM Linux wrote:
[...] What I have as my own version (now with cyclic DMA support) is IMHO a far superior and more correct implementation.
Any chance you could post your code?
Thanks, - Lars
On Tue, May 29, 2012 at 10:21:29AM +0100, Russell King - ARM Linux wrote:
On Tue, May 29, 2012 at 10:02:15AM +0100, Mark Brown wrote:
What is the issue with the current code - it *looks* like you want to use the component device for the DMA as the struct device with dmaengine but I don't understand the issue that's created if we don't (and why things appear to be working for people as they are).
Look. It's very very very very simple.
What does the DMA API take? A struct device. What struct device? Some random struct device for something in the system, or what? No, it takes the struct device for the _device_ in the system which is _performing_ the DMA.
I'm assuming that you mean the client rather than the DMA controller itself (which we must already have found)? That makes sense,
If you persist in not wanting to care about this, then I'm afraid I'll ignore soc-dmaengine-pcm.c entirely as to me its totally bolloxed code. What I have as my own version (now with cyclic DMA support) is IMHO a far superior and more correct implementation.
I'm sorry, I'm not sure what you mean when you say "if you persist"? The mail I replied to was the first time I'd seen mention of this issue at all. I have to confess I haven't read every dmaengine thread in detail, they're quite large and when they talk about generic issues they seem to be focusing pretty much entirely on going over the issues with the completion callbacks.
It would be enormously helpful if you could submit some code here (you did once post a link to it but I don't recall it getting sent to the list), probably as a replacement for the existing code if you don't want to fix the existing code. Even though API changes are going to be required hopefully it should be relatively straightforward to convert the existing users over to the new API, everything should at least be following a common pattern so once one device is converted the rest should be simple. This seems much more likely to get us somewhere than the current situation.
Currently we seem to have nobody actually working on this code in mainline, as far as I can tell you and Vinod are the only people with any active interest at a framework level (with Vinod's mostly being at the dmaengine rather than ASoC level; I don't have any hardware with any sort of dmaengine support) and because you have this out of tree implementation you're working on you're mostly just offering feedback in these driver review threads where apparently chunks of it are getting missed. Nobody else seems to be showing much inclination to get involved so it seems unlikely to collide with other work.
On Tue, May 29, 2012 at 02:14:11PM +0100, Mark Brown wrote:
On Tue, May 29, 2012 at 10:21:29AM +0100, Russell King - ARM Linux wrote:
On Tue, May 29, 2012 at 10:02:15AM +0100, Mark Brown wrote:
What is the issue with the current code - it *looks* like you want to use the component device for the DMA as the struct device with dmaengine but I don't understand the issue that's created if we don't (and why things appear to be working for people as they are).
Look. It's very very very very simple.
What does the DMA API take? A struct device. What struct device? Some random struct device for something in the system, or what? No, it takes the struct device for the _device_ in the system which is _performing_ the DMA.
I'm assuming that you mean the client rather than the DMA controller itself (which we must already have found)? That makes sense,
No, I mean the DMA controller *itself*.
DMA engines _can_ (and do - quite common on ARM platforms) have this setup:
CPU | +-Bus switch matrix-+ (RAM bus) | | RAM------------+--DMA controller---+ | | |(rq+ack) |(IO bus) | | | +-----------Peripheral 1 +--------- | ------------' +-----------Peripheral 2 ...
So, the _peripherals_ themselves have no direct access to RAM at all. These are totally and utterly incapable of any kind of DMA on their own. They can only be _given_ data by some other agent in the system, that being in the above case the CPU or the DMA controller.
When the DMA controller is setup, and asked to transfer data to a peripheral device, it first starts by originating an access to RAM via the RAM bus, and it loads the data into it's FIFO. It then performs accesses on the IO bus to _write_ the data into the peripheral just as a CPU would under PIO.
So, to setup the _peripheral_ struct device with some nebulous "oh this can access the whole of system memory and is DMA capable, let's allocate DMA memory against it" is totally the wrong approach. The peripheral device can not know what the details are for some random DMA controller elsewhere in the system.
Indeed, there are systems out there where the same peripheral can be driven by more than one DMA controller. What if each DMA controller has different properties?
So, the struct device in the system representing the device which is accessing memory is _the_ _DMA_ _controller_.
Why does this matter? Consider this case:
CPU | +-----------Bus switch matrix-+ | | RAM--+--IOMMU-----DMA controller---+ | | |(rq+ack) |(IO bus) | | | +-----------Peripheral 1 +--------- | ------------' +-----------Peripheral 2 ...
Now, for the DMA controller to access RAM, the IOMMU needs to be setup to create mappings between the address space that the DMA controller can see, and the RAM. If there are no mappings setup, the DMA controller can't see the RAM _at_ _all_ and DMA is not possible. The IOMMU is managed via the DMA API on many kernel architectures, x86 included.
If you use the peripheral struct device with the DMA API, how does the DMA API know that it must setup some unknown DMA controller's IOMMU?
And why should the DMA API code even care about such complexities?
We don't do this on USB. When a USB device wants to perform DMA, we map its memory using the _host_ _controller_ struct device, not the USB peripheral device's struct device. It's exactly the same principle. The struct device corresponding with whatever device is _actually_ accessing memory must be the struct device passed into the DMA API.
Currently we seem to have nobody actually working on this code in mainline, as far as I can tell you and Vinod are the only people with any active interest at a framework level (with Vinod's mostly being at the dmaengine rather than ASoC level; I don't have any hardware with any sort of dmaengine support) and because you have this out of tree implementation you're working on you're mostly just offering feedback in these driver review threads where apparently chunks of it are getting missed. Nobody else seems to be showing much inclination to get involved so it seems unlikely to collide with other work.
That's partly because you took in soc-dmaengine.c, after you ridiculed my version - mainly because it didn't support cyclic DMA at the time. And so I assumed that you'd made up your mind, and just plain weren't interested. You have been very resistive towards discussing issues coming up in this area, and in other areas I've raised with ASoC such as the struct device lifetime issues - even when I've created patches for you.
Moreover, because I don't see any hope for the SA11x0 PCM support going in (it certainly won't support capture mode due to the need to run the playback side to receive capture samples) I'm really not pushing it; it's more my testbed for testing out my DMA engine work than serious ASoC work.
So please excuse me if I'm not in a rush to post it to the list. In the mean time, it's viewable via:
http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-arm.git;a=commitdiff;h=76...
On Tue, May 29, 2012 at 02:46:32PM +0100, Russell King - ARM Linux wrote:
On Tue, May 29, 2012 at 02:14:11PM +0100, Mark Brown wrote:
I'm assuming that you mean the client rather than the DMA controller itself (which we must already have found)? That makes sense,
No, I mean the DMA controller *itself*.
I'm quite famililar with the hardware, the issue here is about knowing what the device is being used for. In this case due to context being lost in the quotes I hadn't realised exactly what the bug you were identifying in the current code actually was, I had thought there was a DMA channel already at this point but the lack of one is in fact the problem.
You did identify this briefly on the original submission though as an issue for sa11x0 (which you were higlighting is very oddball and indicated you didn't really want in mainline) rather than a widespread issue and the discussion (well, single followup from Lars-Peter) was that this could be done incrementally which did seem resonable. After that nobody did anything.
That's partly because you took in soc-dmaengine.c, after you ridiculed my version - mainly because it didn't support cyclic DMA at the time.
I don't recall ever ridiculing your version, that's an extremely strong term. Can you please be more specific? The nearest I can find to that is Lars-Peter saying "I had a look at your 'generic' dmaengine PCM driver patch" (which is rather dismissive). I did say that the his idea of moving the cyclic emulation down into dmaengine seemed sensible but I did also agree that perhaps it was sensible to keep it further up the stack if depending on other factors and I'm struggling to see this as ridicule, and I did also express some displeasure at the SA11x0 hardware design but that's definitely a separate thing.
In terms of the decision to apply the library code it's simply a case of the fact that it was the only thing submitted, the only issue anyone had with it was the allocation issue which seemed fixable incrementally, and it was posted with all the relevant mainline platforms converted over. This seems like a clear win, providing a noticable improvement on the previous situation.
Having the library doesn't stop anyone creating a generic DMA driver on top of it or otherwise fixing or improving on the merged code and it does provide useful code sharing to the platforms.
And so I assumed that you'd made up your mind, and just plain weren't interested.
I think you're reading something into things that just isn't there. You seem to see this as an either/or thing but really as with everything else in the kernel the current code is just a useful point at which to start doing incremental development in tree.
As I said at the time I think the final result should be a merge of the two sets of code, and the addition of cyclic DMA support to your code is one example of this happening though sadly out of mainline. Looking at the current state of things the changes I'd hope to see happen if anyone's willing to work on it are something like (in no real order):
- Move the channel acquisition earlier and use that device to allocate buffers. - Merge the non-cyclic support into mainline. - Add the generic driver into mainline in some form or other (or otherwise factor even more code out into the generic code). - Provide the ability for DAIs to register an ASoC DMA driver for themselves from data (like the dmaengine based DT systems want to do). - Give platforms more control over the parameters for the generic driver than they currently do (there's some stuff still #defined in the file).
Possibly split up differently somehow but broadly speaking that's where I'd like to see us going. Hopefully some or all of this will happen before I have a dmaengine based platform running mainline, but if not I guess I'll do some stuff myself.
In short there's clearly room for improvement in what's in mainline, let's fix it with code.
You have been very resistive towards discussing issuescoming up in this area, and in other areas I've raised with ASoC such as the struct device lifetime issues - even when I've created patches for you.
The one case I can recall not applying a patch you've sent was those device lifetime things where my point was that unless there's an issue being seen in actual systems that really isn't the sort of thing to be fixing in -rc (which was what you were pushing strongly for), especially when the patch only addresses a subset of the issue. I have to say I am rather conservative about what I'll apply during -rc, especially to core code and around areas of code like that which are poorly tested minefields.
Besides, the fix should be done at AC'97 bus level anyway since the non-ASoC AC'97 code follows exactly the same pattern - if it's worth fixing it's worth fixing for non-ASoC AC'97 systems too.
Otherwise for things other than patches you have to be aware that, in common with most other maintainers, I've got a limited amount of time to work on things and a whole bunch of different priorities both within the kernel and in the rest of my life so if you're asking for something that's hard or time consuming it might not happen immediately. There are a bunch of other people who work full time on ASoC things with a similar mandate to me you can ask as well.
Moreover, because I don't see any hope for the SA11x0 PCM support going in (it certainly won't support capture mode due to the need to run the playback side to receive capture samples) I'm really not pushing it; it's more my testbed for testing out my DMA engine work than serious ASoC work.
I really don't recall anything fundamental that would block it at least for a playback only driver. There were some issues with L3 but you explained what they were and otherwise it just seemed to be updating to current APIs and what looked like fairly straightforward code motion stuff.
Doing capture might be more tricky but if the bodge is well enough hidden in the driver or sufficiently non-invasive to the core it shouldn't be that big a deal.
participants (5)
-
Lars-Peter Clausen -
Mark Brown -
Russell King - ARM Linux -
Vinod Koul -
zhangfei gao