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@xxxxxx>
> ---
> 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);
>
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
