[PATCH v2 0/2] ASoC: hdmi-codec: Allow driver to restrict sample size

[PATCH v2 0/2] ASoC: hdmi-codec: Allow driver to restrict sample size

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds option to allow hdmi-codec drivers to restrict
the audio sample size based on the values that are suppored in hardware.

First patch is adding that formats option to hdmi_codec_pdata and
second patch is a fix in adv7511 codec driver which only supports
16 and 24 bit samples. 

Changes since v1:
	- Added Review by Jyri
	- Updated comment in code suggested by Jyri

Srinivas Kandagatla (2):
  ASoC: hdmi-codec: Allow drivers to restrict sample sizes.
  drm/bridge: adv7511: restrict audio sample sizes

 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 2 ++
 include/sound/hdmi-codec.h                     | 1 +
 sound/soc/codecs/hdmi-codec.c                  | 8 +++++---
 3 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] ASoC: hdmi-codec: Allow drivers to restrict sample sizes.

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Currently hdmi client drivers does have means to limit the
sample sizes that it can only support. Having formats parameter
option would solve this.

This issue was noticed on DB410c board when adv7511 hdmi codec driver
failed to play a 32 bits audio samples, as it does not support them.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Reviewed-by: Jyri Sarha <jsarha@ti.com>
---
 include/sound/hdmi-codec.h    | 1 +
 sound/soc/codecs/hdmi-codec.c | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 9483c55f871b..89fc4cce5785 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -104,6 +104,7 @@ struct hdmi_codec_pdata {
 	uint i2s:1;
 	uint spdif:1;
 	int max_i2s_channels;
+	u64 formats;
 	void *data;
 };
 
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index 22ed0dc88f0a..a7b4d6757ff1 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -653,9 +653,8 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = {
  * This list is only for formats allowed on the I2S bus. So there is
  * some formats listed that are not supported by HDMI interface. For
  * instance allowing the 32-bit formats enables 24-precision with CPU
- * DAIs that do not support 24-bit formats. If the extra formats cause
- * problems, we should add the video side driver an option to disable
- * them.
+ * DAIs that do not support 24-bit formats. Driver can either use this
+ * list or specify supported formats in formats field of hdmi_codec_pdata.
  */
 #define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
 			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
@@ -780,6 +779,9 @@ static int hdmi_codec_probe(struct platform_device *pdev)
 		hcp->daidrv[i] = hdmi_i2s_dai;
 		hcp->daidrv[i].playback.channels_max =
 			hcd->max_i2s_channels;
+
+		if (hcd->formats)
+			hcp->daidrv[i].playback.formats = hcd->formats;
 		i++;
 	}
 
-- 
2.11.0

[PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

ADV7533 only supports audio samples word width from 16-24 bits.
This patch restricts the audio sample sizes to the supported ones,
so that sound card does not report wrong list of supported hwparms.

Without this patch aplay would fail when playing a 32 bit audio.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
index 67469c26bae8..d01d0aa0eef7 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
@@ -214,6 +214,8 @@ static struct hdmi_codec_pdata codec_data = {
 	.ops = &adv7511_codec_ops,
 	.max_i2s_channels = 2,
 	.i2s = 1,
+	.formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |
+		    SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE),
 };
 
 int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511)
-- 
2.11.0

Re: [alsa-devel] [PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[Arnaud Pouliquen]

Hello Srinivas,

On 08/01/2017 12:49 AM, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> ADV7533 only supports audio samples word width from 16-24 bits.
> This patch restricts the audio sample sizes to the supported ones,
> so that sound card does not report wrong list of supported hwparms.
> 
> Without this patch aplay would fail when playing a 32 bit audio.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> index 67469c26bae8..d01d0aa0eef7 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> @@ -214,6 +214,8 @@ static struct hdmi_codec_pdata codec_data = {
>  	.ops = &adv7511_codec_ops,
>  	.max_i2s_channels = 2,
>  	.i2s = 1,
> +	.formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |
> +		    SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE),
>  };

I'm not sure that this limitation is needed.
I already used ADV7513 and i did not observe this limitation.

I had a look to ADV7533 data-sheet. it should support 32 and 64 bits I2S
format bus, with 16 or 24 bits precision sample. So it should support
SNDRV_PCM_FMTBIT_S32_LE and SNDRV_PCM_FMTBIT_S32_BE

As example, if you configure bus in Left justified format with 24 bits
sample length, 32 bits application samples should be truncated to 24
bits samples at ADV7533 I2S interface level (LSB dropped).

Perhaps i missed something... but look like more an I2S bus
configuration issue in your DT card description, that that a miss in the
drivers.

Regards
Arnaud
>  
>  int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511)
> 

Re: [alsa-devel] [PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[Srinivas Kandagatla]

On 01/08/17 09:42, Arnaud Pouliquen wrote:
> Hello Srinivas,
> 
> On 08/01/2017 12:49 AM, srinivas.kandagatla@linaro.org wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> ADV7533 only supports audio samples word width from 16-24 bits.
>> This patch restricts the audio sample sizes to the supported ones,
>> so that sound card does not report wrong list of supported hwparms.
>>
>> Without this patch aplay would fail when playing a 32 bit audio.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
>> index 67469c26bae8..d01d0aa0eef7 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
>> @@ -214,6 +214,8 @@ static struct hdmi_codec_pdata codec_data = {
>>   	.ops = &adv7511_codec_ops,
>>   	.max_i2s_channels = 2,
>>   	.i2s = 1,
>> +	.formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |
>> +		    SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE),
>>   };
> 
> I'm not sure that this limitation is needed.
> I already used ADV7513 and i did not observe this limitation.
> 
> I had a look to ADV7533 data-sheet. it should support 32 and 64 bits I2S

ADV7511_REG_AUDIO_CFG3(0x14) register definition in datasheet and the 
code in this driver suggest that It only supports 16Bit and 24Bit samples.

> format bus, with 16 or 24 bits precision sample. So it should support
> SNDRV_PCM_FMTBIT_S32_LE and SNDRV_PCM_FMTBIT_S32_BE
> 
> As example, if you configure bus in Left justified format with 24 bits
> sample length, 32 bits application samples should be truncated to 24
> bits samples at ADV7533 I2S interface level (LSB dropped).
May be we can do that to make the user happy but isn't this just 
truncate the resolution to 24Bit then?

And it's a false indication that we are supporting 32bit samples.
Which am not very happy with.


> 
> Perhaps i missed something... but look like more an I2S bus
> configuration issue in your DT card description, that that a miss in the
> drivers.

Nothing to do with DT or other driver, adv7533 audio driver does not 
support 32 bit sample size which is why it would return -EINVAL from
adv7511_hdmi_hw_params().

This patch is fixing the loose ends, by which I mean the card would 
report the exact supported sample sizes to user rather than every thing 
in the I2S_FORMATS list.


Thanks,
srini

> 
> Regards
> Arnaud
>>   
>>   int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511)
>>

Re: [alsa-devel] [PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Tue, Aug 01, 2017 at 01:24:03PM +0100, Srinivas Kandagatla wrote:
> On 01/08/17 09:42, Arnaud Pouliquen wrote:
> > On 08/01/2017 12:49 AM, srinivas.kandagatla@linaro.org wrote:

> > I already used ADV7513 and i did not observe this limitation.

> > I had a look to ADV7533 data-sheet. it should support 32 and 64 bits I2S

> ADV7511_REG_AUDIO_CFG3(0x14) register definition in datasheet and the code
> in this driver suggest that It only supports 16Bit and 24Bit samples.

The amount of data it pays attention to in the frame is not the same as
the size of the frame.

> > format bus, with 16 or 24 bits precision sample. So it should support
> > SNDRV_PCM_FMTBIT_S32_LE and SNDRV_PCM_FMTBIT_S32_BE

> > As example, if you configure bus in Left justified format with 24 bits
> > sample length, 32 bits application samples should be truncated to 24
> > bits samples at ADV7533 I2S interface level (LSB dropped).

> May be we can do that to make the user happy but isn't this just truncate
> the resolution to 24Bit then?

> And it's a false indication that we are supporting 32bit samples.
> Which am not very happy with.

This is what the sample_bits field in the DAI structure is for.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[Srinivas Kandagatla]

On 01/08/17 13:28, Mark Brown wrote:
> On Tue, Aug 01, 2017 at 01:24:03PM +0100, Srinivas Kandagatla wrote:
>> On 01/08/17 09:42, Arnaud Pouliquen wrote:
>>> On 08/01/2017 12:49 AM, srinivas.kandagatla@linaro.org wrote:
> 
>>> I already used ADV7513 and i did not observe this limitation.
> 
>>> I had a look to ADV7533 data-sheet. it should support 32 and 64 bits I2S
> 
>> ADV7511_REG_AUDIO_CFG3(0x14) register definition in datasheet and the code
>> in this driver suggest that It only supports 16Bit and 24Bit samples.
> 
> The amount of data it pays attention to in the frame is not the same as
> the size of the frame.
I agree.

> 
>>> format bus, with 16 or 24 bits precision sample. So it should support
>>> SNDRV_PCM_FMTBIT_S32_LE and SNDRV_PCM_FMTBIT_S32_BE
> 
>>> As example, if you configure bus in Left justified format with 24 bits
>>> sample length, 32 bits application samples should be truncated to 24
>>> bits samples at ADV7533 I2S interface level (LSB dropped).
> 
>> May be we can do that to make the user happy but isn't this just truncate
>> the resolution to 24Bit then?
> 
>> And it's a false indication that we are supporting 32bit samples.
>> Which am not very happy with.
> 
> This is what the sample_bits field in the DAI structure is for.ya.
But still reporting that driver supports 32 bit samples when it does not 
really support all 32 bits, is kinda misleading to user.
Isn't it?

And the driver would be end up with hacked up code for each case.


--srini

> 

Re: [alsa-devel] [PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On Tue, Aug 01, 2017 at 01:52:35PM +0100, Srinivas Kandagatla wrote:
> On 01/08/17 13:28, Mark Brown wrote:

> > > And it's a false indication that we are supporting 32bit samples.
> > > Which am not very happy with.

> > This is what the sample_bits field in the DAI structure is for.ya.

> But still reporting that driver supports 32 bit samples when it does not
> really support all 32 bits, is kinda misleading to user.
> Isn't it?

No.  Please read what I wrote and look at what setting sample_bits does.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [alsa-devel] [PATCH v2 2/2] drm/bridge: adv7511: restrict audio sample sizes

[Arnaud Pouliquen]

Hello Srinivas,

On 08/01/2017 02:52 PM, Srinivas Kandagatla wrote:
>>>> As example, if you configure bus in Left justified format with 24 bits
>>>> sample length, 32 bits application samples should be truncated to 24
>>>> bits samples at ADV7533 I2S interface level (LSB dropped).
>>
>>> May be we can do that to make the user happy but isn't this just truncate
>>> the resolution to 24Bit then?
>>
>>> And it's a false indication that we are supporting 32bit samples.
>>> Which am not very happy with.
>>
>> This is what the sample_bits field in the DAI structure is for.ya.
> But still reporting that driver supports 32 bit samples when it does not 
> really support all 32 bits, is kinda misleading to user.
> Isn't it?
> 
> And the driver would be end up with hacked up code for each case.

By experience, this is usual. As example, if you have a look to codec
ad193x (i take one randomly) it support 16, 20, 24 and 32 bits frames.
But if you have a look to AD1939 data-sheet it supports 24-bits conversion.
Some other examples could be 13-bits DAC/ADC with 16 bits samples.

In term of audio quality, truncation to a 24 bits sample should generate
an negligible additional error equal to the LSB bit: -20log(2^24)= -144dB.

It is just a personal opinion, but if have the choice between do
truncation in software (application or alsa-lib) and in hardware, i
would prefer the second one.

Regards
arnaud