[PATCH 0/4] ALSA/dmaengine: Fix 3 bytes physical sample support

[PATCH 0/4] ALSA/dmaengine: Fix 3 bytes physical sample support

[Peter Ujfalusi]

Hi,

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 snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
the slave_caps is not provided by the driver we mask out the 24 bits
physical width sample formats so they will be not available for user space
to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
will not be masked so later on they can be valid if both CPU and codec dai
supports them.

Regards,
Peter
---
Peter Ujfalusi (4):
  dma: Support for 3 bytes word size
  dma: edma: Declare DMA_SLAVE_BUSWIDTH_3_BYTES as supported buswidth
  ALSA: pcm_dmaengine: Use the available wrapper to get physical width
  ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

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

-- 
2.0.0

[PATCH 0/4] ALSA/dmaengine: Fix 3 bytes physical sample support

[Peter Ujfalusi]

Hi,

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 snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
the slave_caps is not provided by the driver we mask out the 24 bits
physical width sample formats so they will be not available for user space
to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
will not be masked so later on they can be valid if both CPU and codec dai
supports them.

Regards,
Peter
---
Peter Ujfalusi (4):
  dma: Support for 3 bytes word size
  dma: edma: Declare DMA_SLAVE_BUSWIDTH_3_BYTES as supported buswidth
  ALSA: pcm_dmaengine: Use the available wrapper to get physical width
  ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

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

-- 
2.0.0

[PATCH 1/4] 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 1/4] 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 2/4] 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 2/4] 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 3/4] 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>
---
 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 3/4] 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>
---
 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 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[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.

In snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
the slave_caps is not provided by the driver we mask out the 24 bits
physical width sample formats so they will be not available for user space
to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
will not be masked so later on they can be valid if both CPU and codec dai
supports them.

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

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index d5611ec80381..519b500c5669 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
@@ -292,8 +294,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_request_channel);
 int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
 	struct dma_chan *chan)
 {
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct dmaengine_pcm_runtime_data *prtd;
-	int ret;
+	struct dma_slave_caps dma_caps;
+	bool dma_3bytes_supported = false;
+	u64 fmt_mask = 0;
+	int i, ret;
 
 	if (!chan)
 		return -ENXIO;
@@ -311,6 +317,37 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
 
 	substream->runtime->private_data = prtd;
 
+	/*
+	 * Prepare formats mask for valid/allowed sample types. If the dma does
+	 * not support 3bytes word size, it needs to be masked out so user space
+	 * can not use the format which produces corrupted audio.
+	 * If the dma driver does not implement get_slave_caps callback, treat
+	 * it as no 3bytes word support.
+	 */
+	if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
+		u32 addr_widths;
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			addr_widths = dma_caps.dstn_addr_widths;
+		else
+			addr_widths = dma_caps.src_addr_widths;
+
+		if (addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES))
+			dma_3bytes_supported = true;
+	}
+
+	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
+		int bits = snd_pcm_format_physical_width(i);
+
+		if (bits == 24 && !dma_3bytes_supported)
+			fmt_mask |= (1LL << i);
+	}
+
+	if (runtime->hw.formats)
+		runtime->hw.formats &= ~fmt_mask;
+	else
+		runtime->hw.formats = ~fmt_mask;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
-- 
2.0.0

[PATCH 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[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.

In snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
the slave_caps is not provided by the driver we mask out the 24 bits
physical width sample formats so they will be not available for user space
to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
will not be masked so later on they can be valid if both CPU and codec dai
supports them.

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

diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index d5611ec80381..519b500c5669 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
@@ -292,8 +294,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_request_channel);
 int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
 	struct dma_chan *chan)
 {
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct dmaengine_pcm_runtime_data *prtd;
-	int ret;
+	struct dma_slave_caps dma_caps;
+	bool dma_3bytes_supported = false;
+	u64 fmt_mask = 0;
+	int i, ret;
 
 	if (!chan)
 		return -ENXIO;
@@ -311,6 +317,37 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
 
 	substream->runtime->private_data = prtd;
 
+	/*
+	 * Prepare formats mask for valid/allowed sample types. If the dma does
+	 * not support 3bytes word size, it needs to be masked out so user space
+	 * can not use the format which produces corrupted audio.
+	 * If the dma driver does not implement get_slave_caps callback, treat
+	 * it as no 3bytes word support.
+	 */
+	if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
+		u32 addr_widths;
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			addr_widths = dma_caps.dstn_addr_widths;
+		else
+			addr_widths = dma_caps.src_addr_widths;
+
+		if (addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES))
+			dma_3bytes_supported = true;
+	}
+
+	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
+		int bits = snd_pcm_format_physical_width(i);
+
+		if (bits == 24 && !dma_3bytes_supported)
+			fmt_mask |= (1LL << i);
+	}
+
+	if (runtime->hw.formats)
+		runtime->hw.formats &= ~fmt_mask;
+	else
+		runtime->hw.formats = ~fmt_mask;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
-- 
2.0.0

Re: [PATCH 3/4] ALSA: pcm_dmaengine: Use the available wrapper to get physical width

[Lars-Peter Clausen]

On 07/02/2014 01:29 PM, Peter Ujfalusi wrote:
> 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>

Thanks.

> ---
>   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)
>

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[Peter Ujfalusi]

On 07/02/2014 02:29 PM, 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.
> 
> In snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
> supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
> the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
> the slave_caps is not provided by the driver we mask out the 24 bits
> physical width sample formats so they will be not available for user space
> to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
> will not be masked so later on they can be valid if both CPU and codec dai
> supports them.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  sound/core/pcm_dmaengine.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> index d5611ec80381..519b500c5669 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
> @@ -292,8 +294,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_request_channel);
>  int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>  	struct dma_chan *chan)
>  {
> +	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct dmaengine_pcm_runtime_data *prtd;
> -	int ret;
> +	struct dma_slave_caps dma_caps;
> +	bool dma_3bytes_supported = false;
> +	u64 fmt_mask = 0;
> +	int i, ret;
>  
>  	if (!chan)
>  		return -ENXIO;
> @@ -311,6 +317,37 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>  
>  	substream->runtime->private_data = prtd;
>  
> +	/*
> +	 * Prepare formats mask for valid/allowed sample types. If the dma does
> +	 * not support 3bytes word size, it needs to be masked out so user space
> +	 * can not use the format which produces corrupted audio.
> +	 * If the dma driver does not implement get_slave_caps callback, treat
> +	 * it as no 3bytes word support.
> +	 */
> +	if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
> +		u32 addr_widths;
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			addr_widths = dma_caps.dstn_addr_widths;
> +		else
> +			addr_widths = dma_caps.src_addr_widths;
> +
> +		if (addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES))
> +			dma_3bytes_supported = true;
> +	}
> +
> +	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
> +		int bits = snd_pcm_format_physical_width(i);
> +
> +		if (bits == 24 && !dma_3bytes_supported)
> +			fmt_mask |= (1LL << i);
> +	}

per discussion over the irc with Lars we could extend the masking to other
widths as well, which would look something like this:
u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
		  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
		  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
...
/*
 * 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.
 */
if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
		addr_widths = dma_caps.dstn_addr_widths;
	else
		addr_widths = dma_caps.src_addr_widths;
}

for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
	int bits = snd_pcm_format_physical_width(i);

	switch (bits) {
	case 8:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_1_BYTE)))
			fmt_mask |= (1LL << i);
		break;
	case 16:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_2_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	case 24:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	case 32:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	case 64:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_8_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	default:
		fmt_mask |= (1LL << i);
		break;
	}
}

Is this sounds better?


> +
> +	if (runtime->hw.formats)
> +		runtime->hw.formats &= ~fmt_mask;
> +	else
> +		runtime->hw.formats = ~fmt_mask;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
> 

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[Peter Ujfalusi]

On 07/02/2014 02:29 PM, 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.
> 
> In snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
> supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
> the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
> the slave_caps is not provided by the driver we mask out the 24 bits
> physical width sample formats so they will be not available for user space
> to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
> will not be masked so later on they can be valid if both CPU and codec dai
> supports them.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  sound/core/pcm_dmaengine.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> index d5611ec80381..519b500c5669 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
> @@ -292,8 +294,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_request_channel);
>  int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>  	struct dma_chan *chan)
>  {
> +	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct dmaengine_pcm_runtime_data *prtd;
> -	int ret;
> +	struct dma_slave_caps dma_caps;
> +	bool dma_3bytes_supported = false;
> +	u64 fmt_mask = 0;
> +	int i, ret;
>  
>  	if (!chan)
>  		return -ENXIO;
> @@ -311,6 +317,37 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>  
>  	substream->runtime->private_data = prtd;
>  
> +	/*
> +	 * Prepare formats mask for valid/allowed sample types. If the dma does
> +	 * not support 3bytes word size, it needs to be masked out so user space
> +	 * can not use the format which produces corrupted audio.
> +	 * If the dma driver does not implement get_slave_caps callback, treat
> +	 * it as no 3bytes word support.
> +	 */
> +	if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
> +		u32 addr_widths;
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			addr_widths = dma_caps.dstn_addr_widths;
> +		else
> +			addr_widths = dma_caps.src_addr_widths;
> +
> +		if (addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES))
> +			dma_3bytes_supported = true;
> +	}
> +
> +	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
> +		int bits = snd_pcm_format_physical_width(i);
> +
> +		if (bits == 24 && !dma_3bytes_supported)
> +			fmt_mask |= (1LL << i);
> +	}

per discussion over the irc with Lars we could extend the masking to other
widths as well, which would look something like this:
u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
		  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
		  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
...
/*
 * 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.
 */
if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
		addr_widths = dma_caps.dstn_addr_widths;
	else
		addr_widths = dma_caps.src_addr_widths;
}

for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
	int bits = snd_pcm_format_physical_width(i);

	switch (bits) {
	case 8:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_1_BYTE)))
			fmt_mask |= (1LL << i);
		break;
	case 16:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_2_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	case 24:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	case 32:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	case 64:
		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_8_BYTES)))
			fmt_mask |= (1LL << i);
		break;
	default:
		fmt_mask |= (1LL << i);
		break;
	}
}

Is this sounds better?


> +
> +	if (runtime->hw.formats)
> +		runtime->hw.formats &= ~fmt_mask;
> +	else
> +		runtime->hw.formats = ~fmt_mask;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
> 

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[Lars-Peter Clausen]

On 07/02/2014 02:44 PM, Peter Ujfalusi wrote:
> On 07/02/2014 02:29 PM, 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.
>>
>> In snd_dmaengine_pcm_open() we should check the slave_caps of the dma if it
>> supports DMA_SLAVE_BUSWIDTH_3_BYTES. Based on this information we initialize
>> the runtime->hw.formats: if DMA_SLAVE_BUSWIDTH_3_BYTES is not supported or
>> the slave_caps is not provided by the driver we mask out the 24 bits
>> physical width sample formats so they will be not available for user space
>> to pick. If the DMA_SLAVE_BUSWIDTH_3_BYTES is supported _3LE/_3BE formats
>> will not be masked so later on they can be valid if both CPU and codec dai
>> supports them.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>   sound/core/pcm_dmaengine.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
>> index d5611ec80381..519b500c5669 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
>> @@ -292,8 +294,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_request_channel);
>>   int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>>   	struct dma_chan *chan)
>>   {
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>>   	struct dmaengine_pcm_runtime_data *prtd;
>> -	int ret;
>> +	struct dma_slave_caps dma_caps;
>> +	bool dma_3bytes_supported = false;
>> +	u64 fmt_mask = 0;
>> +	int i, ret;
>>
>>   	if (!chan)
>>   		return -ENXIO;
>> @@ -311,6 +317,37 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>>
>>   	substream->runtime->private_data = prtd;
>>
>> +	/*
>> +	 * Prepare formats mask for valid/allowed sample types. If the dma does
>> +	 * not support 3bytes word size, it needs to be masked out so user space
>> +	 * can not use the format which produces corrupted audio.
>> +	 * If the dma driver does not implement get_slave_caps callback, treat
>> +	 * it as no 3bytes word support.
>> +	 */
>> +	if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
>> +		u32 addr_widths;
>> +
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +			addr_widths = dma_caps.dstn_addr_widths;
>> +		else
>> +			addr_widths = dma_caps.src_addr_widths;
>> +
>> +		if (addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES))
>> +			dma_3bytes_supported = true;
>> +	}
>> +
>> +	for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
>> +		int bits = snd_pcm_format_physical_width(i);
>> +
>> +		if (bits == 24 && !dma_3bytes_supported)
>> +			fmt_mask |= (1LL << i);
>> +	}
>
> per discussion over the irc with Lars we could extend the masking to other
> widths as well, which would look something like this:
> u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> 		  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> 		  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> ...
> /*
>   * 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.
>   */
> if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
> 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> 		addr_widths = dma_caps.dstn_addr_widths;
> 	else
> 		addr_widths = dma_caps.src_addr_widths;
> }
>
> for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
> 	int bits = snd_pcm_format_physical_width(i);
>
> 	switch (bits) {
> 	case 8:
> 		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_1_BYTE)))
> 			fmt_mask |= (1LL << i);
> 		break;
> 	case 16:
> 		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_2_BYTES)))
> 			fmt_mask |= (1LL << i);
> 		break;
> 	case 24:
> 		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES)))
> 			fmt_mask |= (1LL << i);
> 		break;
> 	case 32:
> 		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)))
> 			fmt_mask |= (1LL << i);
> 		break;
> 	case 64:
> 		if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_8_BYTES)))
> 			fmt_mask |= (1LL << i);
> 		break;
> 	default:
> 		fmt_mask |= (1LL << i);
> 		break;
> 	}
> }
>
> Is this sounds better?

Sounds good. I think we can assume that DMA_SLAVE_BUSWIDTH_1_BYTE = 1, ...
So (addr_width & BIT(bits / 8)) should work fine and we do not need to 
duplicate the case branches.

The other thing is this should go into dmaengine_pcm_set_runtime_hwparams() 
where we also restrict the other hardware parameters based on the dmaengine 
capabilities.

- Lars

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[Peter Ujfalusi]

On 07/02/2014 03:48 PM, Lars-Peter Clausen wrote:
>> per discussion over the irc with Lars we could extend the masking to other
>> widths as well, which would look something like this:
>> u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>>           BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>>           BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> ...
>> /*
>>   * 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.
>>   */
>> if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
>>     if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>         addr_widths = dma_caps.dstn_addr_widths;
>>     else
>>         addr_widths = dma_caps.src_addr_widths;
>> }
>>
>> for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
>>     int bits = snd_pcm_format_physical_width(i);
>>
>>     switch (bits) {
>>     case 8:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_1_BYTE)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 16:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_2_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 24:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 32:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 64:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_8_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     default:
>>         fmt_mask |= (1LL << i);
>>         break;
>>     }
>> }
>>
>> Is this sounds better?
> 
> Sounds good. I think we can assume that DMA_SLAVE_BUSWIDTH_1_BYTE = 1, ...
> So (addr_width & BIT(bits / 8)) should work fine and we do not need to
> duplicate the case branches.
> 
> The other thing is this should go into dmaengine_pcm_set_runtime_hwparams()
> where we also restrict the other hardware parameters based on the dmaengine
> capabilities.

true

-- 
Péter

Re: [PATCH 4/4] ALSA: pcm_dmaengine: Correct support for 3 physical bytes samples

[Peter Ujfalusi]

On 07/02/2014 03:48 PM, Lars-Peter Clausen wrote:
>> per discussion over the irc with Lars we could extend the masking to other
>> widths as well, which would look something like this:
>> u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>>           BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>>           BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
>> ...
>> /*
>>   * 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.
>>   */
>> if (!dma_get_slave_caps(prtd->dma_chan, &dma_caps)) {
>>     if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>         addr_widths = dma_caps.dstn_addr_widths;
>>     else
>>         addr_widths = dma_caps.src_addr_widths;
>> }
>>
>> for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
>>     int bits = snd_pcm_format_physical_width(i);
>>
>>     switch (bits) {
>>     case 8:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_1_BYTE)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 16:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_2_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 24:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_3_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 32:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     case 64:
>>         if (!(addr_widths & BIT(DMA_SLAVE_BUSWIDTH_8_BYTES)))
>>             fmt_mask |= (1LL << i);
>>         break;
>>     default:
>>         fmt_mask |= (1LL << i);
>>         break;
>>     }
>> }
>>
>> Is this sounds better?
> 
> Sounds good. I think we can assume that DMA_SLAVE_BUSWIDTH_1_BYTE = 1, ...
> So (addr_width & BIT(bits / 8)) should work fine and we do not need to
> duplicate the case branches.
> 
> The other thing is this should go into dmaengine_pcm_set_runtime_hwparams()
> where we also restrict the other hardware parameters based on the dmaengine
> capabilities.

true

-- 
Péter