[PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Peter Ujfalusi]

Hi,

Changes since v2 (in patch 3):
- Replace BIT(bits / 8) with (1 << (bits / 8)) as pointed out by Takashi
- Use switch (addr_widths) to filter non valid physical widths, like -EINVAL or
  widths not dividable by 8.
  The same check could be done with:
  if (bits > 0 && bits <=64 && !(bits % 8) && (addr_widths & (1 << (bits / 8))))
  But I think the switch() implementation is more readable.

Changes since v1:
- the hw.formats mask preparation moved to dmaengine_pcm_set_runtime_hwparams()
- we prepare the mask for all supported sample formats based on the physical
  width of the sample and on the slave_caps information.

I have noticed while switching the daVinci audio to dmaengine_pcm that the _3LE
and _3BE support is not working correctly via dmaengine_pcm:
In case of _3LE/_3BE formats the samples are stored in 3 consecutive bytes
without padding it to 4 bytes. This means that the DMA needs to be able to
support 3 bytes word length in order to read/write the samples from memory
correctly. Originally the code treated 24 bits physical length samples as
they were 32 bits which leads to corruption when playing or recording audio.

To fix the support for 3 bytes physical samples the dma driver also need to have
support for such data arrangement. eDMA does have support for it in HW and the
legacy davinci-pcm platform driver w/o dmaengine was able to support such
formats.

First step is to add DMA_SLAVE_BUSWIDTH_3_BYTES to dma_slave_buswidth for
engines and users to select 3 bytes as bus width.

Followed by:
In dmaengine_pcm_set_runtime_hwparams() we should check the slave_caps of the
dma if it supports the give sample physical width of the sample. Based on this
information we initialize the hw.formats: masking out all non supported formats
based on the physical width.
In case the dma driver does not implement the slave_caps the default assumption
is that it supports 1, 2 and 4 bytes widths.

Regards,
Peter
---
Peter Ujfalusi (5):
  dma: Support for 3 bytes word size
  ALSA: pcm_dmaengine: Use the available wrapper to get physical width
  ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical
    sample sizes
  ALSA: pcm_dmaengine: Correct support for 24bits physical sample widths
  dma: edma: Declare DMA_SLAVE_BUSWIDTH_3_BYTES as supported buswidth

 drivers/dma/edma.c                    |  1 +
 include/linux/dmaengine.h             |  1 +
 sound/core/pcm_dmaengine.c            |  4 +++-
 sound/soc/soc-generic-dmaengine-pcm.c | 37 ++++++++++++++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.0.0

[PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Peter Ujfalusi]

Hi,

Changes since v2 (in patch 3):
- Replace BIT(bits / 8) with (1 << (bits / 8)) as pointed out by Takashi
- Use switch (addr_widths) to filter non valid physical widths, like -EINVAL or
  widths not dividable by 8.
  The same check could be done with:
  if (bits > 0 && bits <=64 && !(bits % 8) && (addr_widths & (1 << (bits / 8))))
  But I think the switch() implementation is more readable.

Changes since v1:
- the hw.formats mask preparation moved to dmaengine_pcm_set_runtime_hwparams()
- we prepare the mask for all supported sample formats based on the physical
  width of the sample and on the slave_caps information.

I have noticed while switching the daVinci audio to dmaengine_pcm that the _3LE
and _3BE support is not working correctly via dmaengine_pcm:
In case of _3LE/_3BE formats the samples are stored in 3 consecutive bytes
without padding it to 4 bytes. This means that the DMA needs to be able to
support 3 bytes word length in order to read/write the samples from memory
correctly. Originally the code treated 24 bits physical length samples as
they were 32 bits which leads to corruption when playing or recording audio.

To fix the support for 3 bytes physical samples the dma driver also need to have
support for such data arrangement. eDMA does have support for it in HW and the
legacy davinci-pcm platform driver w/o dmaengine was able to support such
formats.

First step is to add DMA_SLAVE_BUSWIDTH_3_BYTES to dma_slave_buswidth for
engines and users to select 3 bytes as bus width.

Followed by:
In dmaengine_pcm_set_runtime_hwparams() we should check the slave_caps of the
dma if it supports the give sample physical width of the sample. Based on this
information we initialize the hw.formats: masking out all non supported formats
based on the physical width.
In case the dma driver does not implement the slave_caps the default assumption
is that it supports 1, 2 and 4 bytes widths.

Regards,
Peter
---
Peter Ujfalusi (5):
  dma: Support for 3 bytes word size
  ALSA: pcm_dmaengine: Use the available wrapper to get physical width
  ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical
    sample sizes
  ALSA: pcm_dmaengine: Correct support for 24bits physical sample widths
  dma: edma: Declare DMA_SLAVE_BUSWIDTH_3_BYTES as supported buswidth

 drivers/dma/edma.c                    |  1 +
 include/linux/dmaengine.h             |  1 +
 sound/core/pcm_dmaengine.c            |  4 +++-
 sound/soc/soc-generic-dmaengine-pcm.c | 37 ++++++++++++++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.0.0

[PATCH v3 1/5] dma: Support for 3 bytes word size

[Peter Ujfalusi]

Add DMA_SLAVE_BUSWIDTH_3_BYTES to dma_slave_buswidth for engines and users
to select 3 bytes as bus width.
For example eDMA can be configured to use 3bytes mode and in audio we have
formats stored on 3bytes in memory (_XXX_3LE) where this new bus width can
be used.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/dmaengine.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index d2c5cc7c583c..3d1c2aa51530 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -299,6 +299,7 @@ enum dma_slave_buswidth {
 	DMA_SLAVE_BUSWIDTH_UNDEFINED = 0,
 	DMA_SLAVE_BUSWIDTH_1_BYTE = 1,
 	DMA_SLAVE_BUSWIDTH_2_BYTES = 2,
+	DMA_SLAVE_BUSWIDTH_3_BYTES = 3,
 	DMA_SLAVE_BUSWIDTH_4_BYTES = 4,
 	DMA_SLAVE_BUSWIDTH_8_BYTES = 8,
 };
-- 
2.0.0

[PATCH v3 1/5] dma: Support for 3 bytes word size

[Peter Ujfalusi]

Add DMA_SLAVE_BUSWIDTH_3_BYTES to dma_slave_buswidth for engines and users
to select 3 bytes as bus width.
For example eDMA can be configured to use 3bytes mode and in audio we have
formats stored on 3bytes in memory (_XXX_3LE) where this new bus width can
be used.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/dmaengine.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index d2c5cc7c583c..3d1c2aa51530 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -299,6 +299,7 @@ enum dma_slave_buswidth {
 	DMA_SLAVE_BUSWIDTH_UNDEFINED = 0,
 	DMA_SLAVE_BUSWIDTH_1_BYTE = 1,
 	DMA_SLAVE_BUSWIDTH_2_BYTES = 2,
+	DMA_SLAVE_BUSWIDTH_3_BYTES = 3,
 	DMA_SLAVE_BUSWIDTH_4_BYTES = 4,
 	DMA_SLAVE_BUSWIDTH_8_BYTES = 8,
 };
-- 
2.0.0

[PATCH v3 2/5] ALSA: pcm_dmaengine: Use the available wrapper to get physical width

[Peter Ujfalusi]

params_physical_width() is available via pcm_params.h

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/core/pcm_dmaengine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index 76cbb9ec953a..d5611ec80381 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -65,7 +65,7 @@ int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
 	enum dma_slave_buswidth buswidth;
 	int bits;
 
-	bits = snd_pcm_format_physical_width(params_format(params));
+	bits = params_physical_width(params);
 	if (bits < 8 || bits > 64)
 		return -EINVAL;
 	else if (bits == 8)
-- 
2.0.0

[PATCH v3 2/5] ALSA: pcm_dmaengine: Use the available wrapper to get physical width

[Peter Ujfalusi]

params_physical_width() is available via pcm_params.h

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/core/pcm_dmaengine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index 76cbb9ec953a..d5611ec80381 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -65,7 +65,7 @@ int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
 	enum dma_slave_buswidth buswidth;
 	int bits;
 
-	bits = snd_pcm_format_physical_width(params_format(params));
+	bits = params_physical_width(params);
 	if (bits < 8 || bits > 64)
 		return -EINVAL;
 	else if (bits == 8)
-- 
2.0.0

[PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Peter Ujfalusi]

Based on the dma_slave_caps's addr_widths queried from the dma driver
prepare the hw.formats mask to include only formats which is supported by
the DMA engine.
In case the dma driver does not implement the slave_caps the default
assumption is that it supports 1, 2 and 4 bytes widths.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---

I know that the format validation could be done like this:

+		if (bits > 0 && bits <=64 && !(bits % 8) &&
+		    (addr_widths & (1 << (bits / 8))))
+				hw.formats |= (1LL << i);

But I think the switch() implementation is more readable.


 sound/soc/soc-generic-dmaengine-pcm.c | 37 ++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 5bace124ef43..6307f85e871b 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -119,7 +119,10 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
 	struct snd_dmaengine_dai_dma_data *dma_data;
 	struct dma_slave_caps dma_caps;
 	struct snd_pcm_hardware hw;
-	int ret;
+	u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	int i, ret;
 
 	if (pcm->config && pcm->config->pcm_hardware)
 		return snd_soc_set_runtime_hwparams(substream,
@@ -146,6 +149,38 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
 			hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
 		if (dma_caps.residue_granularity <= DMA_RESIDUE_GRANULARITY_SEGMENT)
 			hw.info |= SNDRV_PCM_INFO_BATCH;
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			addr_widths = dma_caps.dstn_addr_widths;
+		else
+			addr_widths = dma_caps.src_addr_widths;
+	}
+
+	/*
+	 * Prepare formats mask for valid/allowed sample types. If the dma does
+	 * not have support for the given physical word size, it needs to be
+	 * masked out so user space can not use the format which produces
+	 * corrupted audio.
+	 * In case the dma driver does not implement the slave_caps the default
+	 * assumption is that it supports 1, 2 and 4 bytes widths.
+	 */
+	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
+		int bits = snd_pcm_format_physical_width(i);
+
+		/* Enable only samples with DMA supported physical widths */
+		switch (bits) {
+		case 8:
+		case 16:
+		case 24:
+		case 32:
+		case 64:
+			if (addr_widths & (1 << (bits / 8)))
+				hw.formats |= (1LL << i);
+			break;
+		default:
+			/* Unsupported types */
+			break;
+		}
 	}
 
 	return snd_soc_set_runtime_hwparams(substream, &hw);
-- 
2.0.0

[PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Peter Ujfalusi]

Based on the dma_slave_caps's addr_widths queried from the dma driver
prepare the hw.formats mask to include only formats which is supported by
the DMA engine.
In case the dma driver does not implement the slave_caps the default
assumption is that it supports 1, 2 and 4 bytes widths.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---

I know that the format validation could be done like this:

+		if (bits > 0 && bits <=64 && !(bits % 8) &&
+		    (addr_widths & (1 << (bits / 8))))
+				hw.formats |= (1LL << i);

But I think the switch() implementation is more readable.


 sound/soc/soc-generic-dmaengine-pcm.c | 37 ++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 5bace124ef43..6307f85e871b 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -119,7 +119,10 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
 	struct snd_dmaengine_dai_dma_data *dma_data;
 	struct dma_slave_caps dma_caps;
 	struct snd_pcm_hardware hw;
-	int ret;
+	u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	int i, ret;
 
 	if (pcm->config && pcm->config->pcm_hardware)
 		return snd_soc_set_runtime_hwparams(substream,
@@ -146,6 +149,38 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
 			hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
 		if (dma_caps.residue_granularity <= DMA_RESIDUE_GRANULARITY_SEGMENT)
 			hw.info |= SNDRV_PCM_INFO_BATCH;
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			addr_widths = dma_caps.dstn_addr_widths;
+		else
+			addr_widths = dma_caps.src_addr_widths;
+	}
+
+	/*
+	 * Prepare formats mask for valid/allowed sample types. If the dma does
+	 * not have support for the given physical word size, it needs to be
+	 * masked out so user space can not use the format which produces
+	 * corrupted audio.
+	 * In case the dma driver does not implement the slave_caps the default
+	 * assumption is that it supports 1, 2 and 4 bytes widths.
+	 */
+	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
+		int bits = snd_pcm_format_physical_width(i);
+
+		/* Enable only samples with DMA supported physical widths */
+		switch (bits) {
+		case 8:
+		case 16:
+		case 24:
+		case 32:
+		case 64:
+			if (addr_widths & (1 << (bits / 8)))
+				hw.formats |= (1LL << i);
+			break;
+		default:
+			/* Unsupported types */
+			break;
+		}
 	}
 
 	return snd_soc_set_runtime_hwparams(substream, &hw);
-- 
2.0.0

[PATCH v3 4/5] ALSA: pcm_dmaengine: Correct support for 24bits physical sample widths

[Peter Ujfalusi]

In case of _3LE/_3BE formats the samples are stored in 3 consecutive bytes
without padding it to 4 bytes. This means that the DMA needs to be able to
support 3 bytes word length in order to read/write the samples from memory
correctly. Originally the code treated 24 bits physical length samples as
they were 32 bits which leads to corruption when playing or recording audio.

The hw.formats field has already been prepared to exclude formats not
supported by the DMA engine in use, which means that only on platforms where
3 bytes is supported by the DMA will be able to use this format.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/core/pcm_dmaengine.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index d5611ec80381..6542c4083594 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -72,6 +72,8 @@ int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
 		buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
 	else if (bits == 16)
 		buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	else if (bits == 24)
+		buswidth = DMA_SLAVE_BUSWIDTH_3_BYTES;
 	else if (bits <= 32)
 		buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	else
-- 
2.0.0

[PATCH v3 4/5] ALSA: pcm_dmaengine: Correct support for 24bits physical sample widths

[Peter Ujfalusi]

In case of _3LE/_3BE formats the samples are stored in 3 consecutive bytes
without padding it to 4 bytes. This means that the DMA needs to be able to
support 3 bytes word length in order to read/write the samples from memory
correctly. Originally the code treated 24 bits physical length samples as
they were 32 bits which leads to corruption when playing or recording audio.

The hw.formats field has already been prepared to exclude formats not
supported by the DMA engine in use, which means that only on platforms where
3 bytes is supported by the DMA will be able to use this format.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/core/pcm_dmaengine.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index d5611ec80381..6542c4083594 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -72,6 +72,8 @@ int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
 		buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;
 	else if (bits == 16)
 		buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES;
+	else if (bits == 24)
+		buswidth = DMA_SLAVE_BUSWIDTH_3_BYTES;
 	else if (bits <= 32)
 		buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	else
-- 
2.0.0

[PATCH v3 5/5] dma: edma: Declare DMA_SLAVE_BUSWIDTH_3_BYTES as supported buswidth

[Peter Ujfalusi]

eDMA can be configured for 3bytes word size for source and destination.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d08c4dedef35..b512caf46944 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -982,6 +982,7 @@ static void __init edma_chan_init(struct edma_cc *ecc,
 
 #define EDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_3_BYTES) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
 
 static int edma_dma_device_slave_caps(struct dma_chan *dchan,
-- 
2.0.0

[PATCH v3 5/5] dma: edma: Declare DMA_SLAVE_BUSWIDTH_3_BYTES as supported buswidth

[Peter Ujfalusi]

eDMA can be configured for 3bytes word size for source and destination.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/edma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d08c4dedef35..b512caf46944 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -982,6 +982,7 @@ static void __init edma_chan_init(struct edma_cc *ecc,
 
 #define EDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
+				 BIT(DMA_SLAVE_BUSWIDTH_3_BYTES) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
 
 static int edma_dma_device_slave_caps(struct dma_chan *dchan,
-- 
2.0.0

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Lars-Peter Clausen]

On 07/03/2014 06:51 AM, Peter Ujfalusi wrote:
> Based on the dma_slave_caps's addr_widths queried from the dma driver
> prepare the hw.formats mask to include only formats which is supported by
> the DMA engine.
> In case the dma driver does not implement the slave_caps the default
> assumption is that it supports 1, 2 and 4 bytes widths.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Looks good, thanks.

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

> ---
>
> I know that the format validation could be done like this:
>
> +		if (bits > 0 && bits <=64 && !(bits % 8) &&
> +		    (addr_widths & (1 << (bits / 8))))
> +				hw.formats |= (1LL << i);
>
> But I think the switch() implementation is more readable.
>
>
>   sound/soc/soc-generic-dmaengine-pcm.c | 37 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> index 5bace124ef43..6307f85e871b 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -119,7 +119,10 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
>   	struct snd_dmaengine_dai_dma_data *dma_data;
>   	struct dma_slave_caps dma_caps;
>   	struct snd_pcm_hardware hw;
> -	int ret;
> +	u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> +			  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +			  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +	int i, ret;
>
>   	if (pcm->config && pcm->config->pcm_hardware)
>   		return snd_soc_set_runtime_hwparams(substream,
> @@ -146,6 +149,38 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
>   			hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
>   		if (dma_caps.residue_granularity <= DMA_RESIDUE_GRANULARITY_SEGMENT)
>   			hw.info |= SNDRV_PCM_INFO_BATCH;
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			addr_widths = dma_caps.dstn_addr_widths;
> +		else
> +			addr_widths = dma_caps.src_addr_widths;
> +	}
> +
> +	/*
> +	 * Prepare formats mask for valid/allowed sample types. If the dma does
> +	 * not have support for the given physical word size, it needs to be
> +	 * masked out so user space can not use the format which produces
> +	 * corrupted audio.
> +	 * In case the dma driver does not implement the slave_caps the default
> +	 * assumption is that it supports 1, 2 and 4 bytes widths.
> +	 */
> +	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
> +		int bits = snd_pcm_format_physical_width(i);
> +
> +		/* Enable only samples with DMA supported physical widths */
> +		switch (bits) {
> +		case 8:
> +		case 16:
> +		case 24:
> +		case 32:
> +		case 64:
> +			if (addr_widths & (1 << (bits / 8)))
> +				hw.formats |= (1LL << i);
> +			break;
> +		default:
> +			/* Unsupported types */
> +			break;
> +		}
>   	}
>
>   	return snd_soc_set_runtime_hwparams(substream, &hw);
>

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Lars-Peter Clausen]

On 07/03/2014 06:51 AM, Peter Ujfalusi wrote:
> Based on the dma_slave_caps's addr_widths queried from the dma driver
> prepare the hw.formats mask to include only formats which is supported by
> the DMA engine.
> In case the dma driver does not implement the slave_caps the default
> assumption is that it supports 1, 2 and 4 bytes widths.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Looks good, thanks.

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

> ---
>
> I know that the format validation could be done like this:
>
> +		if (bits > 0 && bits <=64 && !(bits % 8) &&
> +		    (addr_widths & (1 << (bits / 8))))
> +				hw.formats |= (1LL << i);
>
> But I think the switch() implementation is more readable.
>
>
>   sound/soc/soc-generic-dmaengine-pcm.c | 37 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> index 5bace124ef43..6307f85e871b 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -119,7 +119,10 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
>   	struct snd_dmaengine_dai_dma_data *dma_data;
>   	struct dma_slave_caps dma_caps;
>   	struct snd_pcm_hardware hw;
> -	int ret;
> +	u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> +			  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +			  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +	int i, ret;
>
>   	if (pcm->config && pcm->config->pcm_hardware)
>   		return snd_soc_set_runtime_hwparams(substream,
> @@ -146,6 +149,38 @@ static int dmaengine_pcm_set_runtime_hwparams(struct snd_pcm_substream *substrea
>   			hw.info |= SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME;
>   		if (dma_caps.residue_granularity <= DMA_RESIDUE_GRANULARITY_SEGMENT)
>   			hw.info |= SNDRV_PCM_INFO_BATCH;
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			addr_widths = dma_caps.dstn_addr_widths;
> +		else
> +			addr_widths = dma_caps.src_addr_widths;
> +	}
> +
> +	/*
> +	 * Prepare formats mask for valid/allowed sample types. If the dma does
> +	 * not have support for the given physical word size, it needs to be
> +	 * masked out so user space can not use the format which produces
> +	 * corrupted audio.
> +	 * In case the dma driver does not implement the slave_caps the default
> +	 * assumption is that it supports 1, 2 and 4 bytes widths.
> +	 */
> +	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
> +		int bits = snd_pcm_format_physical_width(i);
> +
> +		/* Enable only samples with DMA supported physical widths */
> +		switch (bits) {
> +		case 8:
> +		case 16:
> +		case 24:
> +		case 32:
> +		case 64:
> +			if (addr_widths & (1 << (bits / 8)))
> +				hw.formats |= (1LL << i);
> +			break;
> +		default:
> +			/* Unsupported types */
> +			break;
> +		}
>   	}
>
>   	return snd_soc_set_runtime_hwparams(substream, &hw);
>

Re: [PATCH v3 4/5] ALSA: pcm_dmaengine: Correct support for 24bits physical sample widths

[Lars-Peter Clausen]

On 07/03/2014 06:51 AM, Peter Ujfalusi wrote:
> In case of _3LE/_3BE formats the samples are stored in 3 consecutive bytes
> without padding it to 4 bytes. This means that the DMA needs to be able to
> support 3 bytes word length in order to read/write the samples from memory
> correctly. Originally the code treated 24 bits physical length samples as
> they were 32 bits which leads to corruption when playing or recording audio.
>
> The hw.formats field has already been prepared to exclude formats not
> supported by the DMA engine in use, which means that only on platforms where
> 3 bytes is supported by the DMA will be able to use this format.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

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

Re: [PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Lars-Peter Clausen]

On 07/03/2014 06:51 AM, Peter Ujfalusi wrote:
[...]
>
> First step is to add DMA_SLAVE_BUSWIDTH_3_BYTES to dma_slave_buswidth for
> engines and users to select 3 bytes as bus width.
>
> Followed by:
> In dmaengine_pcm_set_runtime_hwparams() we should check the slave_caps of the
> dma if it supports the give sample physical width of the sample. Based on this
> information we initialize the hw.formats: masking out all non supported formats
> based on the physical width.
> In case the dma driver does not implement the slave_caps the default assumption
> is that it supports 1, 2 and 4 bytes widths.

The whole series looks good to me. Since both the DMA and ALSA bits changed 
in this series are fairly trivial I think it is best to merge the whole 
series through the ASoC tree.

- Lars

Re: [PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Vinod Koul]

On Thu, Jul 03, 2014 at 07:51:51AM +0300, Peter Ujfalusi wrote:
 
> I have noticed while switching the daVinci audio to dmaengine_pcm that the _3LE
> and _3BE support is not working correctly via dmaengine_pcm:
> In case of _3LE/_3BE formats the samples are stored in 3 consecutive bytes
> without padding it to 4 bytes. This means that the DMA needs to be able to
> support 3 bytes word length in order to read/write the samples from memory
> correctly. Originally the code treated 24 bits physical length samples as
> they were 32 bits which leads to corruption when playing or recording audio.
> 
> To fix the support for 3 bytes physical samples the dma driver also need to have
> support for such data arrangement. eDMA does have support for it in HW and the
> legacy davinci-pcm platform driver w/o dmaengine was able to support such
> formats.
> 
> First step is to add DMA_SLAVE_BUSWIDTH_3_BYTES to dma_slave_buswidth for
> engines and users to select 3 bytes as bus width.
> 
> Followed by:
> In dmaengine_pcm_set_runtime_hwparams() we should check the slave_caps of the
> dma if it supports the give sample physical width of the sample. Based on this
> information we initialize the hw.formats: masking out all non supported formats
> based on the physical width.
> In case the dma driver does not implement the slave_caps the default assumption
> is that it supports 1, 2 and 4 bytes widths.

Whole series:
Acked-by Vinod Koul <vinod.koul@intel.com>

Yes makes sense to go thru ASoC tree. Mark can you keep this in immutable
branch, which I can merge to my tree for any future fixes...

-- 
~Vinod

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Mark Brown]

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

On Thu, Jul 03, 2014 at 07:51:54AM +0300, Peter Ujfalusi wrote:
> Based on the dma_slave_caps's addr_widths queried from the dma driver
> prepare the hw.formats mask to include only formats which is supported by
> the DMA engine.
> In case the dma driver does not implement the slave_caps the default
> assumption is that it supports 1, 2 and 4 bytes widths.

Acked-by: Mark Brown <broonie@linaro.org>

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

Re: [PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Mark Brown]

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

On Thu, Jul 03, 2014 at 03:08:08PM +0530, Vinod Koul wrote:

> Yes makes sense to go thru ASoC tree. Mark can you keep this in immutable
> branch, which I can merge to my tree for any future fixes...

Fine by me - Takashi?

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

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Vinod Koul]

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

On Thu, Jul 03, 2014 at 01:01:53PM +0100, Mark Brown wrote:
> On Thu, Jul 03, 2014 at 07:51:54AM +0300, Peter Ujfalusi wrote:
> > Based on the dma_slave_caps's addr_widths queried from the dma driver
> > prepare the hw.formats mask to include only formats which is supported by
> > the DMA engine.
> > In case the dma driver does not implement the slave_caps the default
> > assumption is that it supports 1, 2 and 4 bytes widths.
> 
> Acked-by: Mark Brown <broonie@linaro.org>

Do you want this to go thru dmaengine tree??

-- 
~Vinod

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

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Mark Brown]

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

On Thu, Jul 03, 2014 at 05:32:56PM +0530, Vinod Koul wrote:
> On Thu, Jul 03, 2014 at 01:01:53PM +0100, Mark Brown wrote:

> > Acked-by: Mark Brown <broonie@linaro.org>

> Do you want this to go thru dmaengine tree??

Well, I was expecting either ASoC or core ALSA.

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

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Mark Brown]

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

On Thu, Jul 03, 2014 at 05:32:56PM +0530, Vinod Koul wrote:
> On Thu, Jul 03, 2014 at 01:01:53PM +0100, Mark Brown wrote:

> > Acked-by: Mark Brown <broonie@linaro.org>

> Do you want this to go thru dmaengine tree??

Well, I was expecting either ASoC or core ALSA.

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

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

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Vinod Koul]

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

On Thu, Jul 03, 2014 at 02:42:10PM +0100, Mark Brown wrote:
> On Thu, Jul 03, 2014 at 05:32:56PM +0530, Vinod Koul wrote:
> > On Thu, Jul 03, 2014 at 01:01:53PM +0100, Mark Brown wrote:
> 
> > > Acked-by: Mark Brown <broonie@linaro.org>
> 
> > Do you want this to go thru dmaengine tree??
> 
> Well, I was expecting either ASoC or core ALSA.
Ah okay, wasnt thinking ALSA, either ways am fine.
Usually its asoc/dmaengine :)

-- 
~Vinod

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

Re: [PATCH v3 3/5] ASoC: generic-dmaengine-pcm: Prepare formats mask for valid physical sample sizes

[Mark Brown]

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

On Thu, Jul 03, 2014 at 09:47:08PM +0530, Vinod Koul wrote:
> On Thu, Jul 03, 2014 at 02:42:10PM +0100, Mark Brown wrote:

> > Well, I was expecting either ASoC or core ALSA.

> Ah okay, wasnt thinking ALSA, either ways am fine.
> Usually its asoc/dmaengine :)

Yeah, I don't mind - I just figured if I ack then I won't be blocking if
it's not ASoC.

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

Re: [PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Takashi Iwai]

At Thu, 3 Jul 2014 13:02:52 +0100,
Mark Brown wrote:
> 
> On Thu, Jul 03, 2014 at 03:08:08PM +0530, Vinod Koul wrote:
> 
> > Yes makes sense to go thru ASoC tree. Mark can you keep this in immutable
> > branch, which I can merge to my tree for any future fixes...
> 
> Fine by me - Takashi?

Sure, please take my ack to the whole series, too:
   Acked-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH v3 0/5] ALSA/ASoC/dmaengine: Fix 3 bytes physical sample support

[Mark Brown]

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

On Thu, Jul 03, 2014 at 07:51:51AM +0300, Peter Ujfalusi wrote:
> Hi,
> 
> Changes since v2 (in patch 3):

Applied all, thanks.

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