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@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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/