[RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Lars-Peter Clausen]

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;
 
-- 
1.8.0

[RFC 2/2] ASoC: dmaengine-pcm: Provide default config

[Lars-Peter Clausen]

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;
 	}
-- 
1.8.0

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

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.

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

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Vinod Koul]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

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

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

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Mark Brown]

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

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.

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

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

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Lars-Peter Clausen]

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

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Mark Brown]

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

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.

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

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

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Lars-Peter Clausen]

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

Re: [RFC 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Mark Brown]

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

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.

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

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