diff mbox

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

Message ID 1404307763-30557-4-git-send-email-peter.ujfalusi@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Ujfalusi July 2, 2014, 1:29 p.m. UTC 
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>
---
 sound/soc/soc-generic-dmaengine-pcm.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Takashi Iwai July 2, 2014, 1:47 p.m. UTC | #1 
At Wed, 2 Jul 2014 16:29:21 +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.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  sound/soc/soc-generic-dmaengine-pcm.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> index 5bace124ef43..c295e1754d1a 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,27 @@ 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);

Better to add a negative value check.  Some formats return -EINVAL.

> +
> +		/* Enable only samples with DMA supported physical widths */
> +		if (addr_widths & BIT(bits / 8))

Just nitpicking: Using BIT() for 32bit int isn't always a wise choice
since BIT() is defined as unsigned long.  This is a generic problem,
though, as we see many codes using BIT() wrongly (and thus get
compiler warnings on 64bit machines).  We should introduce a new macro 
for 32bit int...


Takashi
Lars-Peter Clausen July 2, 2014, 2:10 p.m. UTC | #2 
On 07/02/2014 03:47 PM, Takashi Iwai wrote:
[...]
>> +	/*
>> +	 * 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);
>
> Better to add a negative value check.  Some formats return -EINVAL.
>

So what should we do with those formats? Not support them?

>> +
>> +		/* Enable only samples with DMA supported physical widths */
>> +		if (addr_widths & BIT(bits / 8))
>
> Just nitpicking: Using BIT() for 32bit int isn't always a wise choice
> since BIT() is defined as unsigned long.  This is a generic problem,
> though, as we see many codes using BIT() wrongly (and thus get
> compiler warnings on 64bit machines).  We should introduce a new macro
> for 32bit int...
Takashi Iwai July 2, 2014, 2:12 p.m. UTC | #3 
At Wed, 02 Jul 2014 16:10:09 +0200,
Lars-Peter Clausen wrote:
> 
> On 07/02/2014 03:47 PM, Takashi Iwai wrote:
> [...]
> >> +	/*
> >> +	 * 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);
> >
> > Better to add a negative value check.  Some formats return -EINVAL.
> >
> 
> So what should we do with those formats? Not support them?

Yes, these are special formats that can't be handled generically.


Takashi
Lars-Peter Clausen July 2, 2014, 2:13 p.m. UTC | #4 
On 07/02/2014 04:12 PM, Takashi Iwai wrote:
> At Wed, 02 Jul 2014 16:10:09 +0200,
> Lars-Peter Clausen wrote:
>>
>> On 07/02/2014 03:47 PM, Takashi Iwai wrote:
>> [...]
>>>> +	/*
>>>> +	 * 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);
>>>
>>> Better to add a negative value check.  Some formats return -EINVAL.
>>>
>>
>> So what should we do with those formats? Not support them?
>
> Yes, these are special formats that can't be handled generically.
>

Looking through the list in pcm_misc.c there are also some formats where the 
physical width is not a multiple of 8. We should probably also skip those.

- Lars
Takashi Iwai July 2, 2014, 2:24 p.m. UTC | #5 
At Wed, 02 Jul 2014 16:13:59 +0200,
Lars-Peter Clausen wrote:
> 
> On 07/02/2014 04:12 PM, Takashi Iwai wrote:
> > At Wed, 02 Jul 2014 16:10:09 +0200,
> > Lars-Peter Clausen wrote:
> >>
> >> On 07/02/2014 03:47 PM, Takashi Iwai wrote:
> >> [...]
> >>>> +	/*
> >>>> +	 * 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);
> >>>
> >>> Better to add a negative value check.  Some formats return -EINVAL.
> >>>
> >>
> >> So what should we do with those formats? Not support them?
> >
> > Yes, these are special formats that can't be handled generically.
> >
> 
> Looking through the list in pcm_misc.c there are also some formats where the 
> physical width is not a multiple of 8. We should probably also skip those.

Right, ADPCM and G723 are such ones.


Takashi
diff mbox

Patch

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index 5bace124ef43..c295e1754d1a 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,27 @@  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 */
+		if (addr_widths & BIT(bits / 8))
+			hw.formats |= (1LL << i);
 	}
 
 	return snd_soc_set_runtime_hwparams(substream, &hw);