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

[PATCH v2 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>

---
Changes since v1:
	* Use dma_get_max_seg_size() to get the maximum size of one period
	* Assume that a DMA engine driver is able to support an infinite number of
	  periods
	* Increase min_period_bytes from 16 to 256.
---
 include/sound/dmaengine_pcm.h         |  2 ++
 sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++++++++++++++++++-------
 2 files changed, 47 insertions(+), 10 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..c39e19e 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -36,6 +36,15 @@ static struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
 	return container_of(p, struct dmaengine_pcm, platform);
 }
 
+static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
+	struct snd_pcm_substream *substream)
+{
+	if (!pcm->chan[substream->stream])
+		return NULL;
+
+	return pcm->chan[substream->stream]->device->dev;
+}
+
 /**
  * snd_dmaengine_pcm_prepare_slave_config() - Generic prepare_slave_config callback
  * @substream: PCM substream
@@ -92,28 +101,54 @@ 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_open(struct snd_pcm_substream *substream)
+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 device *dma_dev = dmaengine_dma_dev(pcm, substream);
 	struct dma_chan *chan = pcm->chan[substream->stream];
+	struct snd_dmaengine_dai_dma_data *dma_data;
+	struct dma_slave_caps dma_caps;
+	struct snd_pcm_hardware hw;
 	int ret;
 
-	ret = snd_soc_set_runtime_hwparams(substream,
+	if (pcm->config->pcm_hardware)
+		return snd_soc_set_runtime_hwparams(substream,
 				pcm->config->pcm_hardware);
-	if (ret)
-		return ret;
 
-	return snd_dmaengine_pcm_open(substream, chan);
+	dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
+
+	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 = UINT_MAX;
+	hw.period_bytes_min = 256;
+	hw.period_bytes_max = dma_get_max_seg_size(dma_dev);
+	hw.buffer_bytes_max = SIZE_MAX;
+	hw.fifo_size = dma_data->fifo_size;
+
+	ret = dma_get_slave_caps(chan, &dma_caps);
+	if (ret == 0) {
+		if (dma_caps.cmd_pause)
+			hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
+	}
+
+	return snd_soc_set_runtime_hwparams(substream, &hw);
 }
 
-static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
-	struct snd_pcm_substream *substream)
+static int dmaengine_pcm_open(struct snd_pcm_substream *substream)
 {
-	if (!pcm->chan[substream->stream])
-		return NULL;
+	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];
+	int ret;
 
-	return pcm->chan[substream->stream]->device->dev;
+	ret = dmaengine_pcm_set_runtime_hwparams(substream);
+	if (ret)
+		return ret;
+
+	return snd_dmaengine_pcm_open(substream, chan);
 }
 
 static void dmaengine_pcm_free(struct snd_pcm *pcm)
-- 
1.8.0

[PATCH v2 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.
	* 512kB 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.

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 | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index c39e19e..99f9495 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -84,12 +84,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;
 
@@ -112,7 +119,7 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
 	struct snd_pcm_hardware hw;
 	int ret;
 
-	if (pcm->config->pcm_hardware)
+	if (pcm->config && pcm->config->pcm_hardware)
 		return snd_soc_set_runtime_hwparams(substream,
 				pcm->config->pcm_hardware);
 
@@ -177,9 +184,20 @@ 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;
+		max_buffer_size = config->pcm_hardware->buffer_bytes_max;
+	} else {
+		prealloc_buffer_size = 512 * 1024;
+		max_buffer_size = SIZE_MAX;
+	}
+
+
 	for (i = SNDRV_PCM_STREAM_PLAYBACK; i <= SNDRV_PCM_STREAM_CAPTURE; i++) {
 		substream = rtd->pcm->streams[i].substream;
 		if (!substream)
@@ -200,8 +218,8 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd)
 		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: [PATCH v2 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Vinod Koul]

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

On Tue, Oct 08, 2013 at 04:49:23PM +0100, Mark Brown wrote:
> On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
> 
> > supported. This patch adds code which uses the newly introduced
> > dma_get_slave_caps() API to query this information from the dmaengine driver.
> 
> How newly introduced (ie, is it in Linus' tree)?
It was merged in last merge window, so you dont have any dependency...

--
~Vinod


-- 

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

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

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

[Vinod Koul]

On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
> 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>
> 
> ---
> Changes since v1:
> 	* Use dma_get_max_seg_size() to get the maximum size of one period
> 	* Assume that a DMA engine driver is able to support an infinite number of
> 	  periods
> 	* Increase min_period_bytes from 16 to 256.
> ---
>  include/sound/dmaengine_pcm.h         |  2 ++
>  sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++++++++++++++++++-------
>  2 files changed, 47 insertions(+), 10 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
is this supposed to be DMA FIFO or I2S FIFO? 

And second question for Takakshi or Mark. I see that this value ends up in
usermode, what is the intented usage of this?

>   */
>  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..c39e19e 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -36,6 +36,15 @@ static struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
>  	return container_of(p, struct dmaengine_pcm, platform);
>  }
>  
> +static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
> +	struct snd_pcm_substream *substream)
> +{
> +	if (!pcm->chan[substream->stream])
> +		return NULL;
> +
> +	return pcm->chan[substream->stream]->device->dev;
> +}
> +
>  /**
>   * snd_dmaengine_pcm_prepare_slave_config() - Generic prepare_slave_config callback
>   * @substream: PCM substream
> @@ -92,28 +101,54 @@ 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_open(struct snd_pcm_substream *substream)
> +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 device *dma_dev = dmaengine_dma_dev(pcm, substream);
>  	struct dma_chan *chan = pcm->chan[substream->stream];
> +	struct snd_dmaengine_dai_dma_data *dma_data;
> +	struct dma_slave_caps dma_caps;
> +	struct snd_pcm_hardware hw;
>  	int ret;
>  
> -	ret = snd_soc_set_runtime_hwparams(substream,
> +	if (pcm->config->pcm_hardware)
> +		return snd_soc_set_runtime_hwparams(substream,
>  				pcm->config->pcm_hardware);
> -	if (ret)
> -		return ret;
>  
> -	return snd_dmaengine_pcm_open(substream, chan);
> +	dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
> +
> +	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 = UINT_MAX;
> +	hw.period_bytes_min = 256;
> +	hw.period_bytes_max = dma_get_max_seg_size(dma_dev);
I think you should check if the driver has set this, if not goto some default

-- 
~Vinod

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

[Vinod Koul]

On Tue, Oct 08, 2013 at 06:13:56PM +0200, Lars-Peter Clausen wrote:
> On 10/08/2013 05:06 PM, Vinod Koul wrote:
> >On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
> >>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>
> >>
> >>---
> >>Changes since v1:
> >>	* Use dma_get_max_seg_size() to get the maximum size of one period
> >>	* Assume that a DMA engine driver is able to support an infinite number of
> >>	  periods
> >>	* Increase min_period_bytes from 16 to 256.
> >>---
> >>  include/sound/dmaengine_pcm.h         |  2 ++
> >>  sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++++++++++++++++++-------
> >>  2 files changed, 47 insertions(+), 10 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
> >is this supposed to be DMA FIFO or I2S FIFO?
> 
> This field is the internal FIFO of the audio core.
> 
> >
> >And second question for Takakshi or Mark. I see that this value ends up in
> >usermode, what is the intented usage of this?
> 
> I think it is supposed to help to calculate the audio delay, but I
> don't think it is actually that widely used at the moment. And yes,
> in order to make this more accurate we should probably add support
> for incorporating also the fifo size of the DMA channel.
But then why dont add to delay field, why report to usermode

> [...]
> >>+	hw.periods_min = 2;
> >>+	hw.periods_max = UINT_MAX;
> >>+	hw.period_bytes_min = 256;
> >>+	hw.period_bytes_max = dma_get_max_seg_size(dma_dev);
> >I think you should check if the driver has set this, if not goto some default
> >
> 
> I don't think we are supposed to directly peak at the dma_parms
> field. If the DMA driver does not work with the default, and it did
> not set up a proper value, it is a bug in the DMA driver.
Well i didnt mean to peek but to check the value returned by
dma_get_max_seg_size(). Also, while buggy driver get fixed we need this :)

-- 
~Vinod

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

[Mark Brown]

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

On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:

> supported. This patch adds code which uses the newly introduced
> dma_get_slave_caps() API to query this information from the dmaengine driver.

How newly introduced (ie, is it in Linus' tree)?

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

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

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

[Lars-Peter Clausen]

On 10/08/2013 05:06 PM, Vinod Koul wrote:
> On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
>> 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>
>>
>> ---
>> Changes since v1:
>> 	* Use dma_get_max_seg_size() to get the maximum size of one period
>> 	* Assume that a DMA engine driver is able to support an infinite number of
>> 	  periods
>> 	* Increase min_period_bytes from 16 to 256.
>> ---
>>   include/sound/dmaengine_pcm.h         |  2 ++
>>   sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++++++++++++++++++-------
>>   2 files changed, 47 insertions(+), 10 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
> is this supposed to be DMA FIFO or I2S FIFO?

This field is the internal FIFO of the audio core.

>
> And second question for Takakshi or Mark. I see that this value ends up in
> usermode, what is the intented usage of this?

I think it is supposed to help to calculate the audio delay, but I don't think 
it is actually that widely used at the moment. And yes, in order to make this 
more accurate we should probably add support for incorporating also the fifo 
size of the DMA channel.

[...]
>> +	hw.periods_min = 2;
>> +	hw.periods_max = UINT_MAX;
>> +	hw.period_bytes_min = 256;
>> +	hw.period_bytes_max = dma_get_max_seg_size(dma_dev);
> I think you should check if the driver has set this, if not goto some default
>

I don't think we are supposed to directly peak at the dma_parms field. If the 
DMA driver does not work with the default, and it did not set up a proper 
value, it is a bug in the DMA driver.

- Lars

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

[Lars-Peter Clausen]

On 10/08/2013 05:30 PM, Vinod Koul wrote:
> On Tue, Oct 08, 2013 at 06:13:56PM +0200, Lars-Peter Clausen wrote:
>> On 10/08/2013 05:06 PM, Vinod Koul wrote:
>>> On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
>>>> 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>
>>>>
>>>> ---
>>>> Changes since v1:
>>>> 	* Use dma_get_max_seg_size() to get the maximum size of one period
>>>> 	* Assume that a DMA engine driver is able to support an infinite number of
>>>> 	  periods
>>>> 	* Increase min_period_bytes from 16 to 256.
>>>> ---
>>>>   include/sound/dmaengine_pcm.h         |  2 ++
>>>>   sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++++++++++++++++++-------
>>>>   2 files changed, 47 insertions(+), 10 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
>>> is this supposed to be DMA FIFO or I2S FIFO?
>>
>> This field is the internal FIFO of the audio core.
>>
>>>
>>> And second question for Takakshi or Mark. I see that this value ends up in
>>> usermode, what is the intented usage of this?
>>
>> I think it is supposed to help to calculate the audio delay, but I
>> don't think it is actually that widely used at the moment. And yes,
>> in order to make this more accurate we should probably add support
>> for incorporating also the fifo size of the DMA channel.
> But then why dont add to delay field, why report to usermode

I don't now :)

>
>> [...]
>>>> +	hw.periods_min = 2;
>>>> +	hw.periods_max = UINT_MAX;
>>>> +	hw.period_bytes_min = 256;
>>>> +	hw.period_bytes_max = dma_get_max_seg_size(dma_dev);
>>> I think you should check if the driver has set this, if not goto some default
>>>
>>
>> I don't think we are supposed to directly peak at the dma_parms
>> field. If the DMA driver does not work with the default, and it did
>> not set up a proper value, it is a bug in the DMA driver.
> Well i didnt mean to peek but to check the value returned by
> dma_get_max_seg_size(). Also, while buggy driver get fixed we need this :)

The default value, if the DMA driver did not setup something explicitly, is 
65535, which is a legitimate value. So we won't be able to tell if this is a 
wrong default or a correct default. But the DAI driver has to explicitly enable 
this new functionality (by not setting any static hw_params). So we hopefully 
won't hit this path for DMA drivers which do not initialize their segment size 
correctly.

- Lars

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

[Mark Brown]

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

On Tue, Oct 08, 2013 at 09:00:24PM +0530, Vinod Koul wrote:
> On Tue, Oct 08, 2013 at 06:13:56PM +0200, Lars-Peter Clausen wrote:

> > >And second question for Takakshi or Mark. I see that this value ends up in
> > >usermode, what is the intented usage of this?

> > I think it is supposed to help to calculate the audio delay, but I
> > don't think it is actually that widely used at the moment. And yes,
> > in order to make this more accurate we should probably add support
> > for incorporating also the fifo size of the DMA channel.

> But then why dont add to delay field, why report to usermode

It's only really useful if the buffer is large enough to start causing
an issue with A/V sync and similar - it's there to help account for
delays introduced after things have gone from DMA.  Most hardware has
relatively small buffers so it's not a big deal.  I don't know why the
two are expressed differently, though the fact that one is in units of
time and the other is in units of data suggests it's just about making
it easier to express.

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

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

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

[Mark Brown]

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

On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
> 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

There was some discussion on this but it didn't seem terribly
conclusive.  Are people happy with these?

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

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

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

[Lars-Peter Clausen]

On 10/16/2013 08:10 PM, Mark Brown wrote:
> On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
>> 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
> 
> There was some discussion on this but it didn't seem terribly
> conclusive.  Are people happy with these?
> 

Was about to ask the same. I'm still happy with it.

- Lars

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

[Mark Brown]

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

On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
> 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

Applied both, thanks.  It seems like if there's issues with the FIFO
stuff we can always clean it up later and this is definitely a step
forward in terms of usability for the API.

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

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

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

[Vinod Koul]

On Wed, Oct 16, 2013 at 08:28:30PM +0200, Lars-Peter Clausen wrote:
> On 10/16/2013 08:10 PM, Mark Brown wrote:
> > On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
> >> 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
> > 
> > There was some discussion on this but it didn't seem terribly
> > conclusive.  Are people happy with these?
> > 
> 
> Was about to ask the same. I'm still happy with it.
Me too, so
Acked-by: Vinod Koul <vinod.koul@intel.com>

--
~Vinod