[PATCH v2 0/4] ASoC: davinci: edma dmaengine PCM and mcasp preparation

[PATCH v2 0/4] ASoC: davinci: edma dmaengine PCM and mcasp preparation

[Peter Ujfalusi]

Hi,

This is a cutdown version of the previous [1] series containing only the ASoC
part of the edma dmaengine conversion, but without the actual switch to this new
edma-pcm platform for the McASP since the edma arch and dmaengine core changes
are needed for it to be usable.

Changes since v1:
- arch and dmaengine patches has been dropped from the ASoC series
- Patches has been reordered so only one patch is going to be needed to switch
  AM335x/AM447x to use the edma-pcm platform driver
- all comments from Lars-Peter Clausen has been addressed for the edma-pcm driver
- no changes to Kconfig/Makefile - do not yet build the edma-pcm driver. It will
  be enabled and integrated when we can do the real switch.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/074269.html

Regards,
Peter
---
Peter Ujfalusi (4):
  ASoC: davinci: Add edma dmaengine platform driver
  ASoC: davinci-mcasp: Provide correct filter_data for dmaengine for
    non-DT boot
  ASoC: davinci-mcasp: Constraint on the period and buffer size based on
    FIFO usage
  ASoC: davinci-mcasp: Assign the dma_data earlier in dai_probe callback

 sound/soc/davinci/davinci-mcasp.c | 59 +++++++++++++++++++++++++++++++++------
 sound/soc/davinci/edma-pcm.c      | 57 +++++++++++++++++++++++++++++++++++++
 sound/soc/davinci/edma-pcm.h      | 25 +++++++++++++++++
 3 files changed, 132 insertions(+), 9 deletions(-)
 create mode 100644 sound/soc/davinci/edma-pcm.c
 create mode 100644 sound/soc/davinci/edma-pcm.h

-- 
1.9.0

[PATCH v2 1/4] ASoC: davinci: Add edma dmaengine platform driver

[Peter Ujfalusi]

Platform driver glue for SoC using eDMA3 to use dmaengine PCM.
The maximum number of periods need to be limited to 19 since the
edma dmaengine driver limits the paRAM slot use for audio at
in cyclic mode.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/edma-pcm.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/davinci/edma-pcm.h | 25 +++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 sound/soc/davinci/edma-pcm.c
 create mode 100644 sound/soc/davinci/edma-pcm.h

diff --git a/sound/soc/davinci/edma-pcm.c b/sound/soc/davinci/edma-pcm.c
new file mode 100644
index 000000000000..d38afb1c61ae
--- /dev/null
+++ b/sound/soc/davinci/edma-pcm.c
@@ -0,0 +1,57 @@
+/*
+ * edma-pcm.c - eDMA PCM driver using dmaengine for AM3xxx, AM4xxx
+ *
+ * Copyright (C) 2014 Texas Instruments, Inc.
+ *
+ * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
+ *
+ * Based on: sound/soc/tegra/tegra_pcm.c
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/dmaengine_pcm.h>
+#include <linux/edma.h>
+
+static const struct snd_pcm_hardware edma_pcm_hardware = {
+	.info			= SNDRV_PCM_INFO_MMAP |
+				  SNDRV_PCM_INFO_MMAP_VALID |
+				  SNDRV_PCM_INFO_BATCH |
+				  SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME |
+				  SNDRV_PCM_INFO_INTERLEAVED,
+	.buffer_bytes_max	= 128 * 1024,
+	.period_bytes_min	= 32,
+	.period_bytes_max	= 64 * 1024,
+	.periods_min		= 2,
+	.periods_max		= 19, /* Limit by edma dmaengine driver */
+};
+
+static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config = {
+	.pcm_hardware = &edma_pcm_hardware,
+	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+	.compat_filter_fn = edma_filter_fn,
+	.prealloc_buffer_size = 128 * 1024,
+};
+
+int edma_pcm_platform_register(struct device *dev)
+{
+	return devm_snd_dmaengine_pcm_register(dev, &edma_dmaengine_pcm_config,
+					SND_DMAENGINE_PCM_FLAG_COMPAT);
+}
+EXPORT_SYMBOL_GPL(edma_pcm_platform_register);
+
+MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
+MODULE_DESCRIPTION("eDMA PCM ASoC platform driver");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/davinci/edma-pcm.h b/sound/soc/davinci/edma-pcm.h
new file mode 100644
index 000000000000..894c378c0f74
--- /dev/null
+++ b/sound/soc/davinci/edma-pcm.h
@@ -0,0 +1,25 @@
+/*
+ * edma-pcm.h - eDMA PCM driver using dmaengine for AM3xxx, AM4xxx
+ *
+ * Copyright (C) 2014 Texas Instruments, Inc.
+ *
+ * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
+ *
+ * Based on: sound/soc/tegra/tegra_pcm.h
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef __EDMA_PCM_H__
+#define __EDMA_PCM_H__
+
+int edma_pcm_platform_register(struct device *dev);
+
+#endif /* __EDMA_PCM_H__ */
-- 
1.9.0

Re: [PATCH v2 1/4] ASoC: davinci: Add edma dmaengine platform driver

[Lars-Peter Clausen]

On 03/14/2014 03:42 PM, Peter Ujfalusi wrote:
> Platform driver glue for SoC using eDMA3 to use dmaengine PCM.
> The maximum number of periods need to be limited to 19 since the
> edma dmaengine driver limits the paRAM slot use for audio at
> in cyclic mode.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Looks good.

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>   sound/soc/davinci/edma-pcm.c | 57 ++++++++++++++++++++++++++++++++++++++++++++
>   sound/soc/davinci/edma-pcm.h | 25 +++++++++++++++++++
>   2 files changed, 82 insertions(+)
>   create mode 100644 sound/soc/davinci/edma-pcm.c
>   create mode 100644 sound/soc/davinci/edma-pcm.h
>
> diff --git a/sound/soc/davinci/edma-pcm.c b/sound/soc/davinci/edma-pcm.c
> new file mode 100644
> index 000000000000..d38afb1c61ae
> --- /dev/null
> +++ b/sound/soc/davinci/edma-pcm.c
> @@ -0,0 +1,57 @@
> +/*
> + * edma-pcm.c - eDMA PCM driver using dmaengine for AM3xxx, AM4xxx
> + *
> + * Copyright (C) 2014 Texas Instruments, Inc.
> + *
> + * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
> + *
> + * Based on: sound/soc/tegra/tegra_pcm.c
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/dmaengine_pcm.h>
> +#include <linux/edma.h>
> +
> +static const struct snd_pcm_hardware edma_pcm_hardware = {
> +	.info			= SNDRV_PCM_INFO_MMAP |
> +				  SNDRV_PCM_INFO_MMAP_VALID |
> +				  SNDRV_PCM_INFO_BATCH |
> +				  SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME |
> +				  SNDRV_PCM_INFO_INTERLEAVED,
> +	.buffer_bytes_max	= 128 * 1024,
> +	.period_bytes_min	= 32,
> +	.period_bytes_max	= 64 * 1024,
> +	.periods_min		= 2,
> +	.periods_max		= 19, /* Limit by edma dmaengine driver */
> +};
> +
> +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config = {
> +	.pcm_hardware = &edma_pcm_hardware,
> +	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
> +	.compat_filter_fn = edma_filter_fn,
> +	.prealloc_buffer_size = 128 * 1024,
> +};
> +
> +int edma_pcm_platform_register(struct device *dev)
> +{
> +	return devm_snd_dmaengine_pcm_register(dev, &edma_dmaengine_pcm_config,
> +					SND_DMAENGINE_PCM_FLAG_COMPAT);
> +}
> +EXPORT_SYMBOL_GPL(edma_pcm_platform_register);
> +
> +MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
> +MODULE_DESCRIPTION("eDMA PCM ASoC platform driver");
> +MODULE_LICENSE("GPL");
> diff --git a/sound/soc/davinci/edma-pcm.h b/sound/soc/davinci/edma-pcm.h
> new file mode 100644
> index 000000000000..894c378c0f74
> --- /dev/null
> +++ b/sound/soc/davinci/edma-pcm.h
> @@ -0,0 +1,25 @@
> +/*
> + * edma-pcm.h - eDMA PCM driver using dmaengine for AM3xxx, AM4xxx
> + *
> + * Copyright (C) 2014 Texas Instruments, Inc.
> + *
> + * Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
> + *
> + * Based on: sound/soc/tegra/tegra_pcm.h
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef __EDMA_PCM_H__
> +#define __EDMA_PCM_H__
> +
> +int edma_pcm_platform_register(struct device *dev);
> +
> +#endif /* __EDMA_PCM_H__ */
>

Re: [PATCH v2 1/4] ASoC: davinci: Add edma dmaengine platform driver

[Mark Brown]

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

On Fri, Mar 14, 2014 at 04:42:45PM +0200, Peter Ujfalusi wrote:
> Platform driver glue for SoC using eDMA3 to use dmaengine PCM.
> The maximum number of periods need to be limited to 19 since the
> edma dmaengine driver limits the paRAM slot use for audio at
> in cyclic mode.

Applied, thanks.

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

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

[PATCH v2 2/4] ASoC: davinci-mcasp: Provide correct filter_data for dmaengine for non-DT boot

[Peter Ujfalusi]

When we boot with non-DT mode the damengine will need the channel number and
a filter function in order to get the channel.
The filter_data is filled in the DAI driver while the filter_function will
be provided by the edma-pcm driver.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index b0ae0677f023..a01ae97c90aa 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -1026,6 +1026,7 @@ nodata:
 static int davinci_mcasp_probe(struct platform_device *pdev)
 {
 	struct davinci_pcm_dma_params *dma_params;
+	struct snd_dmaengine_dai_dma_data *dma_data;
 	struct resource *mem, *ioarea, *res, *dat;
 	struct davinci_mcasp_pdata *pdata;
 	struct davinci_mcasp *mcasp;
@@ -1095,6 +1096,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 		mcasp->dat_port = true;
 
 	dma_params = &mcasp->dma_params[SNDRV_PCM_STREAM_PLAYBACK];
+	dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
 	dma_params->asp_chan_q = pdata->asp_chan_q;
 	dma_params->ram_chan_q = pdata->ram_chan_q;
 	dma_params->sram_pool = pdata->sram_pool;
@@ -1105,7 +1107,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 		dma_params->dma_addr = mem->start + pdata->tx_dma_offset;
 
 	/* Unconditional dmaengine stuff */
-	mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr = dma_params->dma_addr;
+	dma_data->addr = dma_params->dma_addr;
 
 	res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
 	if (res)
@@ -1113,7 +1115,14 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 	else
 		dma_params->channel = pdata->tx_dma_channel;
 
+	/* dmaengine filter data for DT and non-DT boot */
+	if (pdev->dev.of_node)
+		dma_data->filter_data = "tx";
+	else
+		dma_data->filter_data = &dma_params->channel;
+
 	dma_params = &mcasp->dma_params[SNDRV_PCM_STREAM_CAPTURE];
+	dma_data = &mcasp->dma_data[SNDRV_PCM_STREAM_CAPTURE];
 	dma_params->asp_chan_q = pdata->asp_chan_q;
 	dma_params->ram_chan_q = pdata->ram_chan_q;
 	dma_params->sram_pool = pdata->sram_pool;
@@ -1124,7 +1133,7 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 		dma_params->dma_addr = mem->start + pdata->rx_dma_offset;
 
 	/* Unconditional dmaengine stuff */
-	mcasp->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr = dma_params->dma_addr;
+	dma_data->addr = dma_params->dma_addr;
 
 	if (mcasp->version < MCASP_VERSION_3) {
 		mcasp->fifo_base = DAVINCI_MCASP_V2_AFIFO_BASE;
@@ -1140,9 +1149,11 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 	else
 		dma_params->channel = pdata->rx_dma_channel;
 
-	/* Unconditional dmaengine stuff */
-	mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data = "tx";
-	mcasp->dma_data[SNDRV_PCM_STREAM_CAPTURE].filter_data = "rx";
+	/* dmaengine filter data for DT and non-DT boot */
+	if (pdev->dev.of_node)
+		dma_data->filter_data = "rx";
+	else
+		dma_data->filter_data = &dma_params->channel;
 
 	dev_set_drvdata(&pdev->dev, mcasp);
 
-- 
1.9.0

Re: [PATCH v2 2/4] ASoC: davinci-mcasp: Provide correct filter_data for dmaengine for non-DT boot

[Mark Brown]

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

On Fri, Mar 14, 2014 at 04:42:46PM +0200, Peter Ujfalusi wrote:
> When we boot with non-DT mode the damengine will need the channel number and
> a filter function in order to get the channel.
> The filter_data is filled in the DAI driver while the filter_function will
> be provided by the edma-pcm driver.

Applied, thanks.

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

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

[PATCH v2 3/4] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Peter Ujfalusi]

We need to place constraint on the period and buffer size if the read
or write AFIFO is enabled and it is configured for more than one word
otherwise the DMA will fail in buffer configuration where the sizes
are not aligned with the requested FIFO configuration.

Some application (like mplayer) needs the constraint placed on the
buffer size as well. If only period size is constrained they might
fail to figure out the allowed buffer configuration.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index a01ae97c90aa..3ca6e8c4568a 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -720,6 +720,7 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
 				 struct snd_soc_dai *dai)
 {
 	struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
+	int afifo_numevt;
 
 	if (mcasp->version == MCASP_VERSION_4)
 		snd_soc_dai_set_dma_data(dai, substream,
@@ -727,6 +728,21 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
 	else
 		snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params);
 
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		afifo_numevt = mcasp->txnumevt;
+	else
+		afifo_numevt = mcasp->rxnumevt;
+
+	if (afifo_numevt > 1) {
+		snd_pcm_hw_constraint_step(substream->runtime, 0,
+					   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
+					   afifo_numevt);
+
+		snd_pcm_hw_constraint_step(substream->runtime, 0,
+					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
+					   afifo_numevt);
+	}
+
 	return 0;
 }
 
-- 
1.9.0

Re: [PATCH v2 3/4] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Lars-Peter Clausen]

On 03/14/2014 03:42 PM, Peter Ujfalusi wrote:
> We need to place constraint on the period and buffer size if the read
> or write AFIFO is enabled and it is configured for more than one word
> otherwise the DMA will fail in buffer configuration where the sizes
> are not aligned with the requested FIFO configuration.
>
> Some application (like mplayer) needs the constraint placed on the
> buffer size as well. If only period size is constrained they might
> fail to figure out the allowed buffer configuration.

Hm... the sound like there is still a bug somewhere. We constrain the buffer 
size to be a multiple of the period size. If the period size is constraint 
to be a multiple of a constant then the buffer size should automatically be 
constrained to be a multiple of constant * period count. And just 
constraining the buffer size to be a multiple of the burst size still allows 
buffer sizes that are not a multiple of burst size * period count.

>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>   sound/soc/davinci/davinci-mcasp.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
> index a01ae97c90aa..3ca6e8c4568a 100644
> --- a/sound/soc/davinci/davinci-mcasp.c
> +++ b/sound/soc/davinci/davinci-mcasp.c
> @@ -720,6 +720,7 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
>   				 struct snd_soc_dai *dai)
>   {
>   	struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
> +	int afifo_numevt;
>
>   	if (mcasp->version == MCASP_VERSION_4)
>   		snd_soc_dai_set_dma_data(dai, substream,
> @@ -727,6 +728,21 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
>   	else
>   		snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params);
>
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		afifo_numevt = mcasp->txnumevt;
> +	else
> +		afifo_numevt = mcasp->rxnumevt;

Shouldn't this use the same calculation that's used to set 
dma_data->maxburst? Also we should add this to the dmaengine PCM driver, 
since there will most likely be more DMA controllers with this restriction, 
doesn't necessarily need to be done in this patch series though.

> +
> +	if (afifo_numevt > 1) {
> +		snd_pcm_hw_constraint_step(substream->runtime, 0,
> +					   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
> +					   afifo_numevt);
> +
> +		snd_pcm_hw_constraint_step(substream->runtime, 0,
> +					   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
> +					   afifo_numevt);
> +	}
> +
>   	return 0;
>   }
>
>

Re: [PATCH v2 3/4] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Peter Ujfalusi]

On 03/16/2014 01:18 PM, Lars-Peter Clausen wrote:
> On 03/14/2014 03:42 PM, Peter Ujfalusi wrote:
>> We need to place constraint on the period and buffer size if the read
>> or write AFIFO is enabled and it is configured for more than one word
>> otherwise the DMA will fail in buffer configuration where the sizes
>> are not aligned with the requested FIFO configuration.
>>
>> Some application (like mplayer) needs the constraint placed on the
>> buffer size as well. If only period size is constrained they might
>> fail to figure out the allowed buffer configuration.
> 
> Hm... the sound like there is still a bug somewhere. We constrain the buffer
> size to be a multiple of the period size. If the period size is constraint to
> be a multiple of a constant then the buffer size should automatically be
> constrained to be a multiple of constant * period count. And just constraining
> the buffer size to be a multiple of the burst size still allows buffer sizes
> that are not a multiple of burst size * period count.

Yeah, aplay, PA is fine. mplayer however does things in a different manner. I
use mplayer mainly to test PA (with -ao pulse) but sometimes I use it through
ALSA and this is where I noticed the issue.

If I constrain only period_size mplayer will fail to set hw_params. If I only
constrain the buffer_size than we can have period_size which does not allign
with the FIFO settings.

I also tried to have snd_pcm_hw_rule_add() and using the snd_interval_step() -
which need to be exported but mplayer fails with that also. It fails even if I
add the rule for period and buffer size.

Also the constraint as it is currently is not optimal since it does not take
into account the number of channels as you pointed out.

>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>   sound/soc/davinci/davinci-mcasp.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/sound/soc/davinci/davinci-mcasp.c
>> b/sound/soc/davinci/davinci-mcasp.c
>> index a01ae97c90aa..3ca6e8c4568a 100644
>> --- a/sound/soc/davinci/davinci-mcasp.c
>> +++ b/sound/soc/davinci/davinci-mcasp.c
>> @@ -720,6 +720,7 @@ static int davinci_mcasp_startup(struct
>> snd_pcm_substream *substream,
>>                    struct snd_soc_dai *dai)
>>   {
>>       struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
>> +    int afifo_numevt;
>>
>>       if (mcasp->version == MCASP_VERSION_4)
>>           snd_soc_dai_set_dma_data(dai, substream,
>> @@ -727,6 +728,21 @@ static int davinci_mcasp_startup(struct
>> snd_pcm_substream *substream,
>>       else
>>           snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params);
>>
>> +    if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +        afifo_numevt = mcasp->txnumevt;
>> +    else
>> +        afifo_numevt = mcasp->rxnumevt;
> 
> Shouldn't this use the same calculation that's used to set dma_data->maxburst?
> Also we should add this to the dmaengine PCM driver, since there will most
> likely be more DMA controllers with this restriction, doesn't necessarily need
> to be done in this patch series though.

At startup time we do not know the number of channels going to be used. I'm
planning to improve this constraint handling in a coming series.
Using the afifo_numevt as step is not optimal since in case of stereo stream
the step should be (afifo_numevt / 2). afifo_numevt is the sample count and
not the frame count.

> 
>> +
>> +    if (afifo_numevt > 1) {
>> +        snd_pcm_hw_constraint_step(substream->runtime, 0,
>> +                       SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
>> +                       afifo_numevt);
>> +
>> +        snd_pcm_hw_constraint_step(substream->runtime, 0,
>> +                       SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
>> +                       afifo_numevt);
>> +    }
>> +
>>       return 0;
>>   }
>>
>>
> 


-- 
Péter

Re: [PATCH v2 3/4] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Mark Brown]

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

On Mon, Mar 17, 2014 at 03:28:49PM +0200, Peter Ujfalusi wrote:
> On 03/16/2014 01:18 PM, Lars-Peter Clausen wrote:

> > Hm... the sound like there is still a bug somewhere. We constrain the buffer
> > size to be a multiple of the period size. If the period size is constraint to
> > be a multiple of a constant then the buffer size should automatically be
> > constrained to be a multiple of constant * period count. And just constraining
> > the buffer size to be a multiple of the burst size still allows buffer sizes
> > that are not a multiple of burst size * period count.

> Yeah, aplay, PA is fine. mplayer however does things in a different manner. I
> use mplayer mainly to test PA (with -ao pulse) but sometimes I use it through
> ALSA and this is where I noticed the issue.

> If I constrain only period_size mplayer will fail to set hw_params. If I only
> constrain the buffer_size than we can have period_size which does not allign
> with the FIFO settings.

> I also tried to have snd_pcm_hw_rule_add() and using the snd_interval_step() -
> which need to be exported but mplayer fails with that also. It fails even if I
> add the rule for period and buffer size.

> Also the constraint as it is currently is not optimal since it does not take
> into account the number of channels as you pointed out.

This is all sounding like the thing that needs to be looked at here is
mplayer so we understand what's going wrong with regard to the buffer
sizes.  It's sounding like if we should be doing it this is a general
thing which we should be constraining presumably it'd apply to all
drivers, not just this one, and so shouldn't be being fixed in the
driver but it's not obvious to me why the period constraint isn't
sufficient.

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

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

Re: [PATCH v2 3/4] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Peter Ujfalusi]

On 03/17/2014 06:52 PM, Mark Brown wrote:
> This is all sounding like the thing that needs to be looked at here is
> mplayer so we understand what's going wrong with regard to the buffer
> sizes.

The error which was printed by mplayer was the snd_pcm_hw_params() failure.
Prior to this call it calls number of snd_pcm_hw_params_set_* including:
snd_pcm_hw_params_set_buffer_time_near()
snd_pcm_hw_params_set_periods_near()

all without error.

So I recompiled alsa-lib with debug and compiled mplayer on the board as well.
I only kept the constraint for the period size from this patch (no constraint
on buffer size).

The compiled and original mplayer binary is working fine, no errors :o
Then I recompiled alsa-lib without debug (as it was before): same thing, moth
mplayer binary works fine and it picks correct buffer configuration.

I'll resend the last two patch and place the constraint only to the period size.

> It's sounding like if we should be doing it this is a general
> thing which we should be constraining presumably it'd apply to all
> drivers, not just this one, and so shouldn't be being fixed in the
> driver but it's not obvious to me why the period constraint isn't
> sufficient.

It can only be done if we have fixed FIFO for the dai. Right now this is kind
of true for McASP. In HW we actually have 64 32bit word FIFO and currently you
can set the desired FIFO depth via DT/pdata. I'm not really happy about this
since it creates this constraint on the buffer allocation when the SoC is
using eDMA (when sDMA is in used with McASP we do not have such issue).
McBSP also have FIFO on OMAPs, but there I do not lock the FIFO depth, it is
adaptive based on the period/buffer size.

If we have more device with fixed FIFO depth then it make sense to handle the
constraint in the core.

However in case of McASP it is still not that straight forward. The DMA burst
size depends on the number of channels, number of used serializers and the
AFIFO's depth configuration. At the end the period size need to be constraint
to be steps of DMA burst size for eDMA.

Not sure if this is applicable for other SoCs.

-- 
Péter

Re: [PATCH v2 3/4] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Mark Brown]

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

On Tue, Mar 18, 2014 at 02:35:48PM +0200, Peter Ujfalusi wrote:
> On 03/17/2014 06:52 PM, Mark Brown wrote:

> > It's sounding like if we should be doing it this is a general
> > thing which we should be constraining presumably it'd apply to all
> > drivers, not just this one, and so shouldn't be being fixed in the
> > driver but it's not obvious to me why the period constraint isn't
> > sufficient.

> It can only be done if we have fixed FIFO for the dai. Right now this is kind
> of true for McASP. In HW we actually have 64 32bit word FIFO and currently you
> can set the desired FIFO depth via DT/pdata. I'm not really happy about this

Sorry, what I meant was more the bit where you were setting the
constraint on both the buffer and the period than the constraint itself
- that seemed somehow redundant.

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

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

[PATCH v2 4/4] ASoC: davinci-mcasp: Assign the dma_data earlier in dai_probe callback

[Peter Ujfalusi]

Set up the playback_dma_data/capture_dma_data for the dai at probe
time since the generic dmaengine PCM stack needs to have access to
this information early.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 3ca6e8c4568a..a846288371bf 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -722,12 +722,6 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
 	struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
 	int afifo_numevt;
 
-	if (mcasp->version == MCASP_VERSION_4)
-		snd_soc_dai_set_dma_data(dai, substream,
-					&mcasp->dma_data[substream->stream]);
-	else
-		snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params);
-
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		afifo_numevt = mcasp->txnumevt;
 	else
@@ -755,6 +749,25 @@ static const struct snd_soc_dai_ops davinci_mcasp_dai_ops = {
 	.set_sysclk	= davinci_mcasp_set_sysclk,
 };
 
+static int davinci_mcasp_dai_probe(struct snd_soc_dai *dai)
+{
+	struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);
+
+	if (mcasp->version == MCASP_VERSION_4) {
+		/* Using dmaengine PCM */
+		dai->playback_dma_data =
+				&mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
+		dai->capture_dma_data =
+				&mcasp->dma_data[SNDRV_PCM_STREAM_CAPTURE];
+	} else {
+		/* Using davinci-pcm */
+		dai->playback_dma_data = mcasp->dma_params;
+		dai->capture_dma_data = mcasp->dma_params;
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int davinci_mcasp_suspend(struct snd_soc_dai *dai)
 {
@@ -808,6 +821,7 @@ static int davinci_mcasp_resume(struct snd_soc_dai *dai)
 static struct snd_soc_dai_driver davinci_mcasp_dai[] = {
 	{
 		.name		= "davinci-mcasp.0",
+		.probe		= davinci_mcasp_dai_probe,
 		.suspend	= davinci_mcasp_suspend,
 		.resume		= davinci_mcasp_resume,
 		.playback	= {
-- 
1.9.0

Re: [PATCH v2 4/4] ASoC: davinci-mcasp: Assign the dma_data earlier in dai_probe callback

[Mark Brown]

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

On Fri, Mar 14, 2014 at 04:42:48PM +0200, Peter Ujfalusi wrote:
> Set up the playback_dma_data/capture_dma_data for the dai at probe
> time since the generic dmaengine PCM stack needs to have access to
> this information early.

This is OK but depends on patch 3.

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

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