[alsa-devel] [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities
Currently each platform making use the the generic dmaengine PCM driver still needs to provide a custom snd_pcm_hardware struct which specifies the capabilities of the DMA controller, e.g. the maximum period size that can be supported. This patch adds code which uses the newly introduced dma_get_slave_caps() API to query this information from the dmaengine driver. The new code path will only be taken if the 'pcm_hardware' field of the snd_dmaengine_pcm_config struct is NULL.
The patch also introduces a new 'fifo_size' field to the snd_dmaengine_dai_dma_data struct which is used to initialize the snd_pcm_hardware 'fifo_size' field and needs to be set by the DAI driver.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- For review only for now. The dma_get_slave_caps() API has not been merged yet, the patch adding support for it can be found here: http://permalink.gmane.org/gmane.linux.kernel/1524880 --- include/sound/dmaengine_pcm.h | 2 ++ sound/soc/soc-generic-dmaengine-pcm.c | 46 +++++++++++++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h index f11c35c..83b2c3e 100644 --- a/include/sound/dmaengine_pcm.h +++ b/include/sound/dmaengine_pcm.h @@ -61,6 +61,7 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream) * @slave_id: Slave requester id for the DMA channel. * @filter_data: Custom DMA channel filter data, this will usually be used when * requesting the DMA channel. + * @fifo_size: FIFO size of the DAI controller in bytes */ struct snd_dmaengine_dai_dma_data { dma_addr_t addr; @@ -68,6 +69,7 @@ struct snd_dmaengine_dai_dma_data { u32 maxburst; unsigned int slave_id; void *filter_data; + unsigned int fifo_size; };
void snd_dmaengine_pcm_set_config_from_dai_data( diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index e29ec3c..98044a7 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -92,6 +92,43 @@ static int dmaengine_pcm_hw_params(struct snd_pcm_substream *substream, return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params)); }
+static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform); + struct dma_chan *chan = pcm->chan[substream->stream]; + struct snd_dmaengine_dai_dma_data *dma_data; + struct snd_pcm_hardware hw; + struct dma_slave_caps dma_caps; + int ret; + + dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); + + ret = dma_get_slave_caps(chan, &dma_caps); + if (ret) + return ret; + + memset(&hw, 0, sizeof(hw)); + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | + SNDRV_PCM_INFO_INTERLEAVED; + hw.periods_min = 2; + hw.periods_max = dma_caps.max_sg_nr; + hw.period_bytes_min = 16; + hw.period_bytes_max = dma_caps.max_sg_len; + hw.buffer_bytes_max = SIZE_MAX; + hw.fifo_size = dma_data->fifo_size; + + if (hw.periods_max == 0) + hw.periods_max = UINT_MAX; + if (hw.period_bytes_max == 0) + hw.period_bytes_max = SIZE_MAX; + + if (dma_caps.cmd_pause) + hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME; + + return snd_soc_set_runtime_hwparams(substream, &hw); +} + static int dmaengine_pcm_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; @@ -99,8 +136,13 @@ static int dmaengine_pcm_open(struct snd_pcm_substream *substream) struct dma_chan *chan = pcm->chan[substream->stream]; int ret;
- ret = snd_soc_set_runtime_hwparams(substream, - pcm->config->pcm_hardware); + if (pcm->config->pcm_hardware) { + ret = snd_soc_set_runtime_hwparams(substream, + pcm->config->pcm_hardware); + } else { + ret = dmaengine_pcm_set_runtime_hwparams(substream); + } + if (ret) return ret;
This patch adds some default settings for the generic dmaengine PCM driver for the case that no config has been supplied. The following defaults are used: * Use snd_dmaengine_pcm_prepare_slave_config for preparing the DMA slave config. * 256kB for the prealloc buffer size. This value has been chosen based on 'feels about right' and is not backed up by any scientific facts. We may need to come up with something smarter in the future but it should work fine for now. * Query the DMA driver for its maximum period size, etc.
With this infrastructure in place we can finally write DAI drivers which are independent of the DMA controller they are connected to. This is e.g. useful if the DAI IP core is reused across different SoCs, but the SoCs uses different DMA controllers.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- sound/soc/soc-generic-dmaengine-pcm.c | 52 +++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 98044a7..947365de 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -75,12 +75,19 @@ static int dmaengine_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform); struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream); + int (*prepare_slave_config)(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct dma_slave_config *slave_config); struct dma_slave_config slave_config; int ret;
- if (pcm->config->prepare_slave_config) { - ret = pcm->config->prepare_slave_config(substream, params, - &slave_config); + if (!pcm->config) + prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config; + else + prepare_slave_config = pcm->config->prepare_slave_config; + + if (prepare_slave_config) { + ret = prepare_slave_config(substream, params, &slave_config); if (ret) return ret;
@@ -136,7 +143,7 @@ static int dmaengine_pcm_open(struct snd_pcm_substream *substream) struct dma_chan *chan = pcm->chan[substream->stream]; int ret;
- if (pcm->config->pcm_hardware) { + if (pcm->config && pcm->config->pcm_hardware) { ret = snd_soc_set_runtime_hwparams(substream, pcm->config->pcm_hardware); } else { @@ -179,14 +186,39 @@ static struct dma_chan *dmaengine_pcm_compat_request_channel( snd_soc_dai_get_dma_data(rtd->cpu_dai, substream)); }
+static size_t dmaengine_pcm_get_max_buffer_size(struct dma_chan *chan) +{ + struct dma_slave_caps dma_caps; + int ret; + + ret = dma_get_slave_caps(chan, &dma_caps); + if (ret) + return 0; + + if (dma_caps.max_sg_nr == 0 || dma_caps.max_sg_len == 0) + return SIZE_MAX; + + if (SIZE_MAX / dma_caps.max_sg_nr > dma_caps.max_sg_len) + return SIZE_MAX; + + return dma_caps.max_sg_nr * dma_caps.max_sg_len; +} + static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) { struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform); const struct snd_dmaengine_pcm_config *config = pcm->config; struct snd_pcm_substream *substream; + size_t prealloc_buffer_size; + size_t max_buffer_size; unsigned int i; int ret;
+ if (config && config->prealloc_buffer_size) + prealloc_buffer_size = config->prealloc_buffer_size; + else + prealloc_buffer_size = 256 * 1024; + for (i = SNDRV_PCM_STREAM_PLAYBACK; i <= SNDRV_PCM_STREAM_CAPTURE; i++) { substream = rtd->pcm->streams[i].substream; if (!substream) @@ -204,11 +236,19 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd) goto err_free; }
+ if (config && config->pcm_hardware) + max_buffer_size = config->pcm_hardware->buffer_bytes_max; + else + max_buffer_size = dmaengine_pcm_get_max_buffer_size(pcm->chan[i]); + + if (max_buffer_size == 0) + max_buffer_size = prealloc_buffer_size; + ret = snd_pcm_lib_preallocate_pages(substream, SNDRV_DMA_TYPE_DEV, dmaengine_dma_dev(pcm, substream), - config->prealloc_buffer_size, - config->pcm_hardware->buffer_bytes_max); + prealloc_buffer_size, + max_buffer_size); if (ret) goto err_free; }
On Mon, Jul 15, 2013 at 06:42:10PM +0200, Lars-Peter Clausen wrote:
- hw.period_bytes_min = 16;
- hw.period_bytes_max = dma_caps.max_sg_len;
We can't read the minimum period size from dmaengine? Seems like something might have a restriction other than 16 bytes here.
On Mon, Jul 15, 2013 at 06:26:43PM +0100, Mark Brown wrote:
On Mon, Jul 15, 2013 at 06:42:10PM +0200, Lars-Peter Clausen wrote:
- hw.period_bytes_min = 16;
- hw.period_bytes_max = dma_caps.max_sg_len;
We can't read the minimum period size from dmaengine? Seems like something might have a restriction other than 16 bytes here.
max would be how many list items the engine supports so queried therotical min would be 1 byte, not sure how 16 bytes is assumed above but my guess is that if engine is able to push min burst lengths which would be 1 and give you interrupt. But seriously, that would not be practical. So this should be a realistic limit which system can cope with.
~Vinod
On Mon, Jul 15, 2013 at 10:27:21PM +0530, Vinod Koul wrote:
On Mon, Jul 15, 2013 at 06:26:43PM +0100, Mark Brown wrote:
On Mon, Jul 15, 2013 at 06:42:10PM +0200, Lars-Peter Clausen wrote:
- hw.period_bytes_min = 16;
- hw.period_bytes_max = dma_caps.max_sg_len;
We can't read the minimum period size from dmaengine? Seems like something might have a restriction other than 16 bytes here.
max would be how many list items the engine supports so queried therotical min would be 1 byte, not sure how 16 bytes is assumed above but my guess is that if engine is able to push min burst lengths which would be 1 and give you interrupt. But seriously, that would not be practical. So this should be a realistic limit which system can cope with.
Right, we probably want to set an artificial floor here but it still seems like we should be checking that the device actually supports this. If the hardware can only support 64 bytes then the above code won't work properly.
On 07/15/2013 07:57 PM, Mark Brown wrote:
On Mon, Jul 15, 2013 at 10:27:21PM +0530, Vinod Koul wrote:
On Mon, Jul 15, 2013 at 06:26:43PM +0100, Mark Brown wrote:
On Mon, Jul 15, 2013 at 06:42:10PM +0200, Lars-Peter Clausen wrote:
- hw.period_bytes_min = 16;
- hw.period_bytes_max = dma_caps.max_sg_len;
We can't read the minimum period size from dmaengine? Seems like something might have a restriction other than 16 bytes here.
max would be how many list items the engine supports so queried therotical min would be 1 byte, not sure how 16 bytes is assumed above but my guess is that if engine is able to push min burst lengths which would be 1 and give you interrupt. But seriously, that would not be practical. So this should be a realistic limit which system can cope with.
Right, we probably want to set an artificial floor here but it still seems like we should be checking that the device actually supports this. If the hardware can only support 64 bytes then the above code won't work properly.
It shouldn't be to hard to extend the dma_caps API with a min_sg_len. But is this something you've actually seen in existing hardware for that the driver would make use of the dmaengine PCM framework? If it is more of theoretical nature we can still easily add it later if it becomes necessary.
That said it is not uncommon that the segment size needs to be a multiple of the burst size. Adding support for that is still on my TODO list but will require some changes to some of the existing users, since implementing this will be a lot easier if all users use the snd_dmaengine_dai_dma_data struct for their DAI DMA data.
- Lars
On Mon, Jul 15, 2013 at 08:20:28PM +0200, Lars-Peter Clausen wrote:
On 07/15/2013 07:57 PM, Mark Brown wrote:
On Mon, Jul 15, 2013 at 10:27:21PM +0530, Vinod Koul wrote:
Right, we probably want to set an artificial floor here but it still seems like we should be checking that the device actually supports this. If the hardware can only support 64 bytes then the above code won't work properly.
It shouldn't be to hard to extend the dma_caps API with a min_sg_len. But is this something you've actually seen in existing hardware for that the driver would make use of the dmaengine PCM framework? If it is more of theoretical nature we can still easily add it later if it becomes necessary.
I'm not aware of anything but equally well I made zero effort to look and note that quite a few existing drivers appear to have minimum values quite a bit above 16 though I doubt they are all actual restrictions.
That said it is not uncommon that the segment size needs to be a multiple of the burst size. Adding support for that is still on my TODO list but will require some changes to some of the existing users, since implementing this will be a lot easier if all users use the snd_dmaengine_dai_dma_data struct for their DAI DMA data.
Yes, burst sizes are one source of restriction I've seen - probably the main one.
On 07/15/2013 09:51 PM, Mark Brown wrote:
On Mon, Jul 15, 2013 at 08:20:28PM +0200, Lars-Peter Clausen wrote:
On 07/15/2013 07:57 PM, Mark Brown wrote:
On Mon, Jul 15, 2013 at 10:27:21PM +0530, Vinod Koul wrote:
Right, we probably want to set an artificial floor here but it still seems like we should be checking that the device actually supports this. If the hardware can only support 64 bytes then the above code won't work properly.
It shouldn't be to hard to extend the dma_caps API with a min_sg_len. But is this something you've actually seen in existing hardware for that the driver would make use of the dmaengine PCM framework? If it is more of theoretical nature we can still easily add it later if it becomes necessary.
I'm not aware of anything but equally well I made zero effort to look and note that quite a few existing drivers appear to have minimum values quite a bit above 16 though I doubt they are all actual restrictions.
I would assume that most of them don't express hardware limitations but rather are sensible lower limits which allow operation without over-/underruns. But that's something that doesn't necessarily depend on the DMA controller, but rather on the system as a whole, e.g. on a slower machine you'd typically set the limit higher so the CPU has a better chance to keep up. So this isn't something you'd want to set in the DMA controller driver. But I'm not sure if there is a good way to calculate a sensible minimum buffer size based on the whole system's constraints.
- Lars
On Tue, Jul 16, 2013 at 11:07:07AM +0200, Lars-Peter Clausen wrote:
I would assume that most of them don't express hardware limitations but rather are sensible lower limits which allow operation without over-/underruns. But that's something that doesn't necessarily depend on the
Yes, indeed. Or just cut'n'pasted from some other driver without much thought.
DMA controller, but rather on the system as a whole, e.g. on a slower machine you'd typically set the limit higher so the CPU has a better chance to keep up. So this isn't something you'd want to set in the DMA controller driver. But I'm not sure if there is a good way to calculate a sensible minimum buffer size based on the whole system's constraints.
Not really. It's going to depend on userspace as well, and things like SMIs on systems with those.
participants (3)
-
Lars-Peter Clausen -
Mark Brown -
Vinod Koul