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

[Lars-Peter Clausen <lars@metafoo.de>]

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