diff mbox series

[v5,2/7] ASoC: tegra: Allow 24bit and 32bit samples

Message ID 20191018154833.7560-3-ben.dooks@codethink.co.uk
State New
Headers show
Series [v5,1/7] ASoC: tegra: add a TDM configuration callback | expand

Commit Message

Ben Dooks Oct. 18, 2019, 3:48 p.m. UTC
From: Edward Cragg <edward.cragg@codethink.co.uk>

The tegra3 audio can support 24 and 32 bit sample sizes so add the
option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
formats when requested.

Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
[ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
[ben.dooks@codethink.co.uk: add pm calls around ytdm config]
[ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code

ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
---
 sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

Jon Hunter Oct. 24, 2019, 3:54 p.m. UTC | #1
On 18/10/2019 16:48, Ben Dooks wrote:
> From: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> The tegra3 audio can support 24 and 32 bit sample sizes so add the
> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
> formats when requested.
> 
> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
> 
> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
> ---
>  sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index 73f0dddeaef3..063f34c882af 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	struct device *dev = dai->dev;
>  	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>  	unsigned int mask, val, reg;
> -	int ret, sample_size, srate, i2sclock, bitcnt;
> +	int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>  	struct tegra30_ahub_cif_conf cif_conf;
>  
>  	if (params_channels(params) != 2)
> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
>  		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>  		sample_size = 16;
>  		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> +		sample_size = 24;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_32;
> +		sample_size = 32;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	cif_conf.threshold = 0;
>  	cif_conf.audio_channels = 2;
>  	cif_conf.client_channels = 2;
> -	cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
> -	cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
> +	cif_conf.audio_bits = audio_bits;
> +	cif_conf.client_bits = audio_bits;
>  	cif_conf.expand = 0;
>  	cif_conf.stereo_conv = 0;
>  	cif_conf.replicate = 0;
> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
>  		.channels_min = 2,
>  		.channels_max = 2,
>  		.rates = SNDRV_PCM_RATE_8000_96000,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		.formats = SNDRV_PCM_FMTBIT_S32_LE |
> +			   SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S16_LE,
>  	},
>  	.capture = {
>  		.stream_name = "Capture",
>  		.channels_min = 2,
>  		.channels_max = 2,
>  		.rates = SNDRV_PCM_RATE_8000_96000,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		.formats = SNDRV_PCM_FMTBIT_S32_LE |
> +			   SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S16_LE,
>  	},
>  	.ops = &tegra30_i2s_dai_ops,
>  	.symmetric_rates = 1,

Thanks!

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon
Dmitry Osipenko Nov. 23, 2019, 9:09 p.m. UTC | #2
18.10.2019 18:48, Ben Dooks пишет:
> From: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> The tegra3 audio can support 24 and 32 bit sample sizes so add the
> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
> formats when requested.
> 
> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
> 
> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
> ---
>  sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index 73f0dddeaef3..063f34c882af 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	struct device *dev = dai->dev;
>  	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>  	unsigned int mask, val, reg;
> -	int ret, sample_size, srate, i2sclock, bitcnt;
> +	int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>  	struct tegra30_ahub_cif_conf cif_conf;
>  
>  	if (params_channels(params) != 2)
> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
>  		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>  		sample_size = 16;
>  		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> +		sample_size = 24;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_32;
> +		sample_size = 32;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	cif_conf.threshold = 0;
>  	cif_conf.audio_channels = 2;
>  	cif_conf.client_channels = 2;
> -	cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
> -	cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
> +	cif_conf.audio_bits = audio_bits;
> +	cif_conf.client_bits = audio_bits;
>  	cif_conf.expand = 0;
>  	cif_conf.stereo_conv = 0;
>  	cif_conf.replicate = 0;
> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
>  		.channels_min = 2,
>  		.channels_max = 2,
>  		.rates = SNDRV_PCM_RATE_8000_96000,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		.formats = SNDRV_PCM_FMTBIT_S32_LE |
> +			   SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S16_LE,
>  	},
>  	.capture = {
>  		.stream_name = "Capture",
>  		.channels_min = 2,
>  		.channels_max = 2,
>  		.rates = SNDRV_PCM_RATE_8000_96000,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		.formats = SNDRV_PCM_FMTBIT_S32_LE |
> +			   SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S16_LE,
>  	},
>  	.ops = &tegra30_i2s_dai_ops,
>  	.symmetric_rates = 1,
> 

Hello,

This patch breaks audio on Tegra30. I don't see errors anywhere, but
there is no audio and reverting this patch helps. Please fix it.
Ben Dooks Nov. 25, 2019, 10:37 a.m. UTC | #3
On 23/11/2019 21:09, Dmitry Osipenko wrote:
> 18.10.2019 18:48, Ben Dooks пишет:
>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>
>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>> formats when requested.
>>
>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>
>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>> ---
>>   sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
>> index 73f0dddeaef3..063f34c882af 100644
>> --- a/sound/soc/tegra/tegra30_i2s.c
>> +++ b/sound/soc/tegra/tegra30_i2s.c
>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>>   	struct device *dev = dai->dev;
>>   	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>   	unsigned int mask, val, reg;
>> -	int ret, sample_size, srate, i2sclock, bitcnt;
>> +	int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>   	struct tegra30_ahub_cif_conf cif_conf;
>>   
>>   	if (params_channels(params) != 2)
>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>>   	switch (params_format(params)) {
>>   	case SNDRV_PCM_FORMAT_S16_LE:
>>   		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>> +		audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>   		sample_size = 16;
>>   		break;
>> +	case SNDRV_PCM_FORMAT_S24_LE:
>> +		val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>> +		audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>> +		sample_size = 24;
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S32_LE:
>> +		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>> +		audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>> +		sample_size = 32;
>> +		break;
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>>   	cif_conf.threshold = 0;
>>   	cif_conf.audio_channels = 2;
>>   	cif_conf.client_channels = 2;
>> -	cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>> -	cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>> +	cif_conf.audio_bits = audio_bits;
>> +	cif_conf.client_bits = audio_bits;
>>   	cif_conf.expand = 0;
>>   	cif_conf.stereo_conv = 0;
>>   	cif_conf.replicate = 0;
>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
>>   		.channels_min = 2,
>>   		.channels_max = 2,
>>   		.rates = SNDRV_PCM_RATE_8000_96000,
>> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
>> +		.formats = SNDRV_PCM_FMTBIT_S32_LE |
>> +			   SNDRV_PCM_FMTBIT_S24_LE |
>> +			   SNDRV_PCM_FMTBIT_S16_LE,
>>   	},
>>   	.capture = {
>>   		.stream_name = "Capture",
>>   		.channels_min = 2,
>>   		.channels_max = 2,
>>   		.rates = SNDRV_PCM_RATE_8000_96000,
>> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
>> +		.formats = SNDRV_PCM_FMTBIT_S32_LE |
>> +			   SNDRV_PCM_FMTBIT_S24_LE |
>> +			   SNDRV_PCM_FMTBIT_S16_LE,
>>   	},
>>   	.ops = &tegra30_i2s_dai_ops,
>>   	.symmetric_rates = 1,
>>
> 
> Hello,
> 
> This patch breaks audio on Tegra30. I don't see errors anywhere, but
> there is no audio and reverting this patch helps. Please fix it.

What is the failure mode? I can try and take a look at this some time
this week, but I am not sure if I have any boards with an actual useful
audio output?
Dmitry Osipenko Nov. 25, 2019, 5:22 p.m. UTC | #4
25.11.2019 13:37, Ben Dooks пишет:
> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>> 18.10.2019 18:48, Ben Dooks пишет:
>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>
>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>>> formats when requested.
>>>
>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>> ---
>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed
>>> in tdm code
>>>
>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>> ---
>>>   sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>> b/sound/soc/tegra/tegra30_i2s.c
>>> index 73f0dddeaef3..063f34c882af 100644
>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>> snd_pcm_substream *substream,
>>>       struct device *dev = dai->dev;
>>>       struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>       unsigned int mask, val, reg;
>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>       struct tegra30_ahub_cif_conf cif_conf;
>>>         if (params_channels(params) != 2)
>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>> snd_pcm_substream *substream,
>>>       switch (params_format(params)) {
>>>       case SNDRV_PCM_FORMAT_S16_LE:
>>>           val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>           sample_size = 16;
>>>           break;
>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>> +        sample_size = 24;
>>> +        break;
>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>> +        sample_size = 32;
>>> +        break;
>>>       default:
>>>           return -EINVAL;
>>>       }
>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>> snd_pcm_substream *substream,
>>>       cif_conf.threshold = 0;
>>>       cif_conf.audio_channels = 2;
>>>       cif_conf.client_channels = 2;
>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>> +    cif_conf.audio_bits = audio_bits;
>>> +    cif_conf.client_bits = audio_bits;
>>>       cif_conf.expand = 0;
>>>       cif_conf.stereo_conv = 0;
>>>       cif_conf.replicate = 0;
>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>> tegra30_i2s_dai_template = {
>>>           .channels_min = 2,
>>>           .channels_max = 2,
>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>       },
>>>       .capture = {
>>>           .stream_name = "Capture",
>>>           .channels_min = 2,
>>>           .channels_max = 2,
>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>       },
>>>       .ops = &tegra30_i2s_dai_ops,
>>>       .symmetric_rates = 1,
>>>
>>
>> Hello,
>>
>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>> there is no audio and reverting this patch helps. Please fix it.
> 
> What is the failure mode? I can try and take a look at this some time
> this week, but I am not sure if I have any boards with an actual useful
> audio output?

The failure mode is that there no sound. I also noticed that video
playback stutters a lot if movie file has audio track, seems something
times out during of the audio playback. For now I don't have any more info.
Dmitry Osipenko Nov. 25, 2019, 5:28 p.m. UTC | #5
25.11.2019 20:22, Dmitry Osipenko пишет:
> 25.11.2019 13:37, Ben Dooks пишет:
>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>
>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>>>> formats when requested.
>>>>
>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>> ---
>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed
>>>> in tdm code
>>>>
>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>> ---
>>>>   sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>> index 73f0dddeaef3..063f34c882af 100644
>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>> snd_pcm_substream *substream,
>>>>       struct device *dev = dai->dev;
>>>>       struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>       unsigned int mask, val, reg;
>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>       struct tegra30_ahub_cif_conf cif_conf;
>>>>         if (params_channels(params) != 2)
>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>> snd_pcm_substream *substream,
>>>>       switch (params_format(params)) {
>>>>       case SNDRV_PCM_FORMAT_S16_LE:
>>>>           val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>           sample_size = 16;
>>>>           break;
>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>> +        sample_size = 24;
>>>> +        break;
>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>> +        sample_size = 32;
>>>> +        break;
>>>>       default:
>>>>           return -EINVAL;
>>>>       }
>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>> snd_pcm_substream *substream,
>>>>       cif_conf.threshold = 0;
>>>>       cif_conf.audio_channels = 2;
>>>>       cif_conf.client_channels = 2;
>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>> +    cif_conf.audio_bits = audio_bits;
>>>> +    cif_conf.client_bits = audio_bits;
>>>>       cif_conf.expand = 0;
>>>>       cif_conf.stereo_conv = 0;
>>>>       cif_conf.replicate = 0;
>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>> tegra30_i2s_dai_template = {
>>>>           .channels_min = 2,
>>>>           .channels_max = 2,
>>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>       },
>>>>       .capture = {
>>>>           .stream_name = "Capture",
>>>>           .channels_min = 2,
>>>>           .channels_max = 2,
>>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>       },
>>>>       .ops = &tegra30_i2s_dai_ops,
>>>>       .symmetric_rates = 1,
>>>>
>>>
>>> Hello,
>>>
>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>> there is no audio and reverting this patch helps. Please fix it.
>>
>> What is the failure mode? I can try and take a look at this some time
>> this week, but I am not sure if I have any boards with an actual useful
>> audio output?
> 
> The failure mode is that there no sound. I also noticed that video
> playback stutters a lot if movie file has audio track, seems something
> times out during of the audio playback. For now I don't have any more info.
> 

Oh, I didn't say how to reproduce it.. for example simply playing
big_buck_bunny_720p_h264.mov in MPV has the audio problem.

https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
Dmitry Osipenko Dec. 19, 2019, 9:21 p.m. UTC | #6
25.11.2019 20:28, Dmitry Osipenko пишет:
> 25.11.2019 20:22, Dmitry Osipenko пишет:
>> 25.11.2019 13:37, Ben Dooks пишет:
>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>
>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>>>>> formats when requested.
>>>>>
>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>> ---
>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed
>>>>> in tdm code
>>>>>
>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>> ---
>>>>>   sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>       struct device *dev = dai->dev;
>>>>>       struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>       unsigned int mask, val, reg;
>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>       struct tegra30_ahub_cif_conf cif_conf;
>>>>>         if (params_channels(params) != 2)
>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>       switch (params_format(params)) {
>>>>>       case SNDRV_PCM_FORMAT_S16_LE:
>>>>>           val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>           sample_size = 16;
>>>>>           break;
>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>> +        sample_size = 24;
>>>>> +        break;
>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>> +        sample_size = 32;
>>>>> +        break;
>>>>>       default:
>>>>>           return -EINVAL;
>>>>>       }
>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>       cif_conf.threshold = 0;
>>>>>       cif_conf.audio_channels = 2;
>>>>>       cif_conf.client_channels = 2;
>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>       cif_conf.expand = 0;
>>>>>       cif_conf.stereo_conv = 0;
>>>>>       cif_conf.replicate = 0;
>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>> tegra30_i2s_dai_template = {
>>>>>           .channels_min = 2,
>>>>>           .channels_max = 2,
>>>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>       },
>>>>>       .capture = {
>>>>>           .stream_name = "Capture",
>>>>>           .channels_min = 2,
>>>>>           .channels_max = 2,
>>>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>       },
>>>>>       .ops = &tegra30_i2s_dai_ops,
>>>>>       .symmetric_rates = 1,
>>>>>
>>>>
>>>> Hello,
>>>>
>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>> there is no audio and reverting this patch helps. Please fix it.
>>>
>>> What is the failure mode? I can try and take a look at this some time
>>> this week, but I am not sure if I have any boards with an actual useful
>>> audio output?
>>
>> The failure mode is that there no sound. I also noticed that video
>> playback stutters a lot if movie file has audio track, seems something
>> times out during of the audio playback. For now I don't have any more info.
>>
> 
> Oh, I didn't say how to reproduce it.. for example simply playing
> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
> 
> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
> 

Hello Ben,

Do you have any updates? I just re-check whether problem persists and
it's still there using a recent linux-next.

Interestingly, I can hear some sound now, but it's very distorted.

If you don't have a solution, then what about to revert the patches for
now and try again later on?
Ben Dooks Dec. 20, 2019, 10:56 a.m. UTC | #7
On 19/12/2019 21:21, Dmitry Osipenko wrote:
> 25.11.2019 20:28, Dmitry Osipenko пишет:
>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>
>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>>>>>> formats when requested.
>>>>>>
>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>> ---
>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed
>>>>>> in tdm code
>>>>>>
>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>> ---
>>>>>>    sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>    1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>> snd_pcm_substream *substream,
>>>>>>        struct device *dev = dai->dev;
>>>>>>        struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>        unsigned int mask, val, reg;
>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>        struct tegra30_ahub_cif_conf cif_conf;
>>>>>>          if (params_channels(params) != 2)
>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>> snd_pcm_substream *substream,
>>>>>>        switch (params_format(params)) {
>>>>>>        case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>            val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>            sample_size = 16;
>>>>>>            break;
>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>> +        sample_size = 24;
>>>>>> +        break;
>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>> +        sample_size = 32;
>>>>>> +        break;
>>>>>>        default:
>>>>>>            return -EINVAL;
>>>>>>        }
>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>> snd_pcm_substream *substream,
>>>>>>        cif_conf.threshold = 0;
>>>>>>        cif_conf.audio_channels = 2;
>>>>>>        cif_conf.client_channels = 2;
>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>        cif_conf.expand = 0;
>>>>>>        cif_conf.stereo_conv = 0;
>>>>>>        cif_conf.replicate = 0;
>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>> tegra30_i2s_dai_template = {
>>>>>>            .channels_min = 2,
>>>>>>            .channels_max = 2,
>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>        },
>>>>>>        .capture = {
>>>>>>            .stream_name = "Capture",
>>>>>>            .channels_min = 2,
>>>>>>            .channels_max = 2,
>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>        },
>>>>>>        .ops = &tegra30_i2s_dai_ops,
>>>>>>        .symmetric_rates = 1,
>>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>
>>>> What is the failure mode? I can try and take a look at this some time
>>>> this week, but I am not sure if I have any boards with an actual useful
>>>> audio output?
>>>
>>> The failure mode is that there no sound. I also noticed that video
>>> playback stutters a lot if movie file has audio track, seems something
>>> times out during of the audio playback. For now I don't have any more info.
>>>
>>
>> Oh, I didn't say how to reproduce it.. for example simply playing
>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>
>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>
> 
> Hello Ben,
> 
> Do you have any updates? I just re-check whether problem persists and
> it's still there using a recent linux-next.
> 
> Interestingly, I can hear some sound now, but it's very distorted.
> 
> If you don't have a solution, then what about to revert the patches for
> now and try again later on?

I will try and have a look later, I had completely forgotten these had
been merged.
Jon Hunter Dec. 20, 2019, 11:30 a.m. UTC | #8
On 25/11/2019 17:28, Dmitry Osipenko wrote:
> 25.11.2019 20:22, Dmitry Osipenko пишет:
>> 25.11.2019 13:37, Ben Dooks пишет:
>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>
>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>>>>> formats when requested.
>>>>>
>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>> ---
>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed
>>>>> in tdm code
>>>>>
>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>> ---
>>>>>   sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>       struct device *dev = dai->dev;
>>>>>       struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>       unsigned int mask, val, reg;
>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>       struct tegra30_ahub_cif_conf cif_conf;
>>>>>         if (params_channels(params) != 2)
>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>       switch (params_format(params)) {
>>>>>       case SNDRV_PCM_FORMAT_S16_LE:
>>>>>           val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>           sample_size = 16;
>>>>>           break;
>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>> +        sample_size = 24;
>>>>> +        break;
>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>> +        sample_size = 32;
>>>>> +        break;
>>>>>       default:
>>>>>           return -EINVAL;
>>>>>       }
>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>       cif_conf.threshold = 0;
>>>>>       cif_conf.audio_channels = 2;
>>>>>       cif_conf.client_channels = 2;
>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>       cif_conf.expand = 0;
>>>>>       cif_conf.stereo_conv = 0;
>>>>>       cif_conf.replicate = 0;
>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>> tegra30_i2s_dai_template = {
>>>>>           .channels_min = 2,
>>>>>           .channels_max = 2,
>>>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>       },
>>>>>       .capture = {
>>>>>           .stream_name = "Capture",
>>>>>           .channels_min = 2,
>>>>>           .channels_max = 2,
>>>>>           .rates = SNDRV_PCM_RATE_8000_96000,
>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>       },
>>>>>       .ops = &tegra30_i2s_dai_ops,
>>>>>       .symmetric_rates = 1,
>>>>>
>>>>
>>>> Hello,
>>>>
>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>> there is no audio and reverting this patch helps. Please fix it.
>>>
>>> What is the failure mode? I can try and take a look at this some time
>>> this week, but I am not sure if I have any boards with an actual useful
>>> audio output?
>>
>> The failure mode is that there no sound. I also noticed that video
>> playback stutters a lot if movie file has audio track, seems something
>> times out during of the audio playback. For now I don't have any more info.
>>
> 
> Oh, I didn't say how to reproduce it.. for example simply playing
> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
> 
> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov

Given that the audio drivers uses regmap, it could be good to dump the
I2S/AHUB registers while the clip if playing with and without this patch
to see the differences. I am curious if the audio is now being played as
24 or 32-bit instead of 16-bit now these are available.

You could also dump the hw_params to see the format while playing as
well ...

$ /proc/asound/<scard-name>/pcm0p/sub0/hw_params

Jon
Ben Dooks Dec. 20, 2019, 11:38 a.m. UTC | #9
On 20/12/2019 11:30, Jon Hunter wrote:
> 
> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>
>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE
>>>>>> formats when requested.
>>>>>>
>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>> ---
>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed
>>>>>> in tdm code
>>>>>>
>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>> ---
>>>>>>    sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>    1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>> snd_pcm_substream *substream,
>>>>>>        struct device *dev = dai->dev;
>>>>>>        struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>        unsigned int mask, val, reg;
>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>        struct tegra30_ahub_cif_conf cif_conf;
>>>>>>          if (params_channels(params) != 2)
>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>> snd_pcm_substream *substream,
>>>>>>        switch (params_format(params)) {
>>>>>>        case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>            val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>            sample_size = 16;
>>>>>>            break;
>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>> +        sample_size = 24;
>>>>>> +        break;
>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>> +        sample_size = 32;
>>>>>> +        break;
>>>>>>        default:
>>>>>>            return -EINVAL;
>>>>>>        }
>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>> snd_pcm_substream *substream,
>>>>>>        cif_conf.threshold = 0;
>>>>>>        cif_conf.audio_channels = 2;
>>>>>>        cif_conf.client_channels = 2;
>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>        cif_conf.expand = 0;
>>>>>>        cif_conf.stereo_conv = 0;
>>>>>>        cif_conf.replicate = 0;
>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>> tegra30_i2s_dai_template = {
>>>>>>            .channels_min = 2,
>>>>>>            .channels_max = 2,
>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>        },
>>>>>>        .capture = {
>>>>>>            .stream_name = "Capture",
>>>>>>            .channels_min = 2,
>>>>>>            .channels_max = 2,
>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>        },
>>>>>>        .ops = &tegra30_i2s_dai_ops,
>>>>>>        .symmetric_rates = 1,
>>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>
>>>> What is the failure mode? I can try and take a look at this some time
>>>> this week, but I am not sure if I have any boards with an actual useful
>>>> audio output?
>>>
>>> The failure mode is that there no sound. I also noticed that video
>>> playback stutters a lot if movie file has audio track, seems something
>>> times out during of the audio playback. For now I don't have any more info.
>>>
>>
>> Oh, I didn't say how to reproduce it.. for example simply playing
>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>
>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
> 
> Given that the audio drivers uses regmap, it could be good to dump the
> I2S/AHUB registers while the clip if playing with and without this patch
> to see the differences. I am curious if the audio is now being played as
> 24 or 32-bit instead of 16-bit now these are available.
> 
> You could also dump the hw_params to see the format while playing as
> well ...
> 
> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params

I suppose it is also possible that the codec isn't properly doing >16
bits and the fact we now offer 24 and 32 could be an issue. I've not
got anything with an audio output on it that would be easy to test.
Jon Hunter Dec. 20, 2019, 1:57 p.m. UTC | #10
On 20/12/2019 11:38, Ben Dooks wrote:
> On 20/12/2019 11:30, Jon Hunter wrote:
>>
>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>
>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>> S32_LE
>>>>>>> formats when requested.
>>>>>>>
>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>> ---
>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>> needed
>>>>>>> in tdm code
>>>>>>>
>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>> ---
>>>>>>>    sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>>    1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>> snd_pcm_substream *substream,
>>>>>>>        struct device *dev = dai->dev;
>>>>>>>        struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>>        unsigned int mask, val, reg;
>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>        struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>          if (params_channels(params) != 2)
>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>> snd_pcm_substream *substream,
>>>>>>>        switch (params_format(params)) {
>>>>>>>        case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>            val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>            sample_size = 16;
>>>>>>>            break;
>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>> +        sample_size = 24;
>>>>>>> +        break;
>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>> +        sample_size = 32;
>>>>>>> +        break;
>>>>>>>        default:
>>>>>>>            return -EINVAL;
>>>>>>>        }
>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>> snd_pcm_substream *substream,
>>>>>>>        cif_conf.threshold = 0;
>>>>>>>        cif_conf.audio_channels = 2;
>>>>>>>        cif_conf.client_channels = 2;
>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>        cif_conf.expand = 0;
>>>>>>>        cif_conf.stereo_conv = 0;
>>>>>>>        cif_conf.replicate = 0;
>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>            .channels_min = 2,
>>>>>>>            .channels_max = 2,
>>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>        },
>>>>>>>        .capture = {
>>>>>>>            .stream_name = "Capture",
>>>>>>>            .channels_min = 2,
>>>>>>>            .channels_max = 2,
>>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>        },
>>>>>>>        .ops = &tegra30_i2s_dai_ops,
>>>>>>>        .symmetric_rates = 1,
>>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>
>>>>> What is the failure mode? I can try and take a look at this some time
>>>>> this week, but I am not sure if I have any boards with an actual
>>>>> useful
>>>>> audio output?
>>>>
>>>> The failure mode is that there no sound. I also noticed that video
>>>> playback stutters a lot if movie file has audio track, seems something
>>>> times out during of the audio playback. For now I don't have any
>>>> more info.
>>>>
>>>
>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>
>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>
>>
>> Given that the audio drivers uses regmap, it could be good to dump the
>> I2S/AHUB registers while the clip if playing with and without this patch
>> to see the differences. I am curious if the audio is now being played as
>> 24 or 32-bit instead of 16-bit now these are available.
>>
>> You could also dump the hw_params to see the format while playing as
>> well ...
>>
>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
> 
> I suppose it is also possible that the codec isn't properly doing >16
> bits and the fact we now offer 24 and 32 could be an issue. I've not
> got anything with an audio output on it that would be easy to test.

I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
back. However, admittedly I may have only done simple 16-bit testing
with speaker-test.

We do verify that all soundcards are detected and registered as expected
during daily testing, but at the moment we don't have anything that
verifies actual playback.

Jon
Dmitry Osipenko Dec. 20, 2019, 2:43 p.m. UTC | #11
20.12.2019 16:57, Jon Hunter пишет:
> 
> On 20/12/2019 11:38, Ben Dooks wrote:
>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>
>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>
>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>>> S32_LE
>>>>>>>> formats when requested.
>>>>>>>>
>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>> ---
>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>>> needed
>>>>>>>> in tdm code
>>>>>>>>
>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>> ---
>>>>>>>>    sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>>>    1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>        struct device *dev = dai->dev;
>>>>>>>>        struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>>>        unsigned int mask, val, reg;
>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>>        struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>          if (params_channels(params) != 2)
>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>        switch (params_format(params)) {
>>>>>>>>        case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>            val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>            sample_size = 16;
>>>>>>>>            break;
>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>> +        sample_size = 24;
>>>>>>>> +        break;
>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>> +        sample_size = 32;
>>>>>>>> +        break;
>>>>>>>>        default:
>>>>>>>>            return -EINVAL;
>>>>>>>>        }
>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>        cif_conf.threshold = 0;
>>>>>>>>        cif_conf.audio_channels = 2;
>>>>>>>>        cif_conf.client_channels = 2;
>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>        cif_conf.expand = 0;
>>>>>>>>        cif_conf.stereo_conv = 0;
>>>>>>>>        cif_conf.replicate = 0;
>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>            .channels_min = 2,
>>>>>>>>            .channels_max = 2,
>>>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>        },
>>>>>>>>        .capture = {
>>>>>>>>            .stream_name = "Capture",
>>>>>>>>            .channels_min = 2,
>>>>>>>>            .channels_max = 2,
>>>>>>>>            .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>        },
>>>>>>>>        .ops = &tegra30_i2s_dai_ops,
>>>>>>>>        .symmetric_rates = 1,
>>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>>
>>>>>> What is the failure mode? I can try and take a look at this some time
>>>>>> this week, but I am not sure if I have any boards with an actual
>>>>>> useful
>>>>>> audio output?
>>>>>
>>>>> The failure mode is that there no sound. I also noticed that video
>>>>> playback stutters a lot if movie file has audio track, seems something
>>>>> times out during of the audio playback. For now I don't have any
>>>>> more info.
>>>>>
>>>>
>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>
>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>
>>>
>>> Given that the audio drivers uses regmap, it could be good to dump the
>>> I2S/AHUB registers while the clip if playing with and without this patch
>>> to see the differences. I am curious if the audio is now being played as
>>> 24 or 32-bit instead of 16-bit now these are available.
>>>
>>> You could also dump the hw_params to see the format while playing as
>>> well ...
>>>
>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>
>> I suppose it is also possible that the codec isn't properly doing >16
>> bits and the fact we now offer 24 and 32 could be an issue. I've not
>> got anything with an audio output on it that would be easy to test.
> 
> I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
> back. However, admittedly I may have only done simple 16-bit testing
> with speaker-test.
> 
> We do verify that all soundcards are detected and registered as expected
> during daily testing, but at the moment we don't have anything that
> verifies actual playback.

Please take a look at the attached logs.
Works
-----

# cat /sys/class/i2c-dev/i2c-2/name
7000d000.i2c

...
	i2c@7000d000 {
		clock-frequency = <100000>;
		status = "okay";

		rt5640: rt5640@1c {
			compatible = "realtek,rt5640";
			reg = <0x1c>;

			interrupt-parent = <&gpio>;
			interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_EDGE_FALLING>;

			realtek,dmic1-data-pin = <1>;
			realtek,dmic2-data-pin = <0>;
			realtek,in1-differential;
		};
...

# cat /proc/asound/card0/pcm0p/sub0/hw_params
access: MMAP_INTERLEAVED
format: S16_LE
subformat: STD
channels: 2
rate: 48000 (48000/1)
period_size: 1024
buffer_size: 8192

# trace-cmd record -e regmap:*
# trace-cmd report

CPU 0 is empty
CPU 1 is empty
cpus=4
             mpv-308   [002]   171.268104: regmap_cache_only:    70080000.ahub flag=0
             mpv-308   [002]   171.268116: regmap_cache_only:    70080000.ahub flag=0
             mpv-308   [002]   171.268131: regmap_cache_only:    70080400.i2s flag=0
             mpv-308   [002]   171.272549: regmap_reg_read_cache: 2-001c reg=64 val=0
             mpv-308   [002]   171.272556: regmap_reg_read_cache: 2-001c reg=80 val=0
             mpv-308   [002]   171.272564: regmap_reg_read_cache: 2-001c reg=70 val=8000
             mpv-308   [002]   171.272567: regmap_reg_read_cache: 2-001c reg=70 val=8000
             mpv-308   [002]   171.272569: regmap_reg_read_cache: 2-001c reg=73 val=1114
             mpv-308   [002]   171.272572: regmap_reg_write:     2-001c reg=73 val=114
             mpv-308   [002]   171.272581: regmap_hw_write_start: 2-001c reg=73 count=1
             mpv-308   [002]   171.273332: regmap_hw_write_done: 2-001c reg=73 count=1
             mpv-308   [002]   171.273379: regmap_reg_read_cache: 70080400.i2s reg=0 val=400
             mpv-308   [002]   171.273382: regmap_reg_write:     70080400.i2s reg=0 val=403
             mpv-308   [002]   171.273395: regmap_reg_write:     70080400.i2s reg=4 val=1f
             mpv-308   [002]   171.273398: regmap_reg_write:     70080400.i2s reg=14 val=1013304
             mpv-308   [002]   171.273401: regmap_reg_write:     70080400.i2s reg=8 val=10001
    kworker/u8:2-145   [003]   171.273992: regmap_reg_read_cache: 2-001c reg=63 val=0
    kworker/u8:2-145   [003]   171.273999: regmap_reg_write:     2-001c reg=63 val=a810
    kworker/u8:2-145   [003]   171.274006: regmap_hw_write_start: 2-001c reg=63 count=1
    kworker/u8:2-145   [003]   171.274478: regmap_hw_write_done: 2-001c reg=63 count=1
    kworker/u8:2-145   [003]   171.286067: regmap_reg_read_cache: 2-001c reg=63 val=a810
    kworker/u8:2-145   [003]   171.286076: regmap_reg_write:     2-001c reg=63 val=e818
    kworker/u8:2-145   [003]   171.286083: regmap_hw_write_start: 2-001c reg=63 count=1
    kworker/u8:2-145   [003]   171.286568: regmap_hw_write_done: 2-001c reg=63 count=1
    kworker/u8:2-145   [003]   171.286575: regmap_reg_read_cache: 2-001c reg=fa val=3f01
    kworker/u8:2-145   [003]   171.286577: regmap_reg_read_cache: 2-001c reg=93 val=3030
             mpv-308   [002]   171.286643: regmap_reg_read_cache: 2-001c reg=61 val=0
             mpv-308   [002]   171.286647: regmap_reg_write:     2-001c reg=61 val=9800
             mpv-308   [002]   171.286650: regmap_hw_write_start: 2-001c reg=61 count=1
             mpv-308   [002]   171.287345: regmap_hw_write_done: 2-001c reg=61 count=1
             mpv-308   [002]   171.287379: regmap_reg_read_cache: 2-001c reg=63 val=e818
             mpv-308   [002]   171.287381: regmap_reg_write:     2-001c reg=63 val=e8d8
             mpv-308   [002]   171.287384: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-308   [002]   171.287845: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-308   [002]   171.287875: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.289021: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-308   [002]   171.289025: regmap_reg_read:      2-001c reg=6a val=23
             mpv-308   [002]   171.289027: regmap_reg_write:     2-001c reg=6a val=24
             mpv-308   [002]   171.289029: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.289561: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-308   [002]   171.289565: regmap_hw_read_start: 2-001c reg=6c count=1
             mpv-308   [002]   171.290174: regmap_hw_read_done:  2-001c reg=6c count=1
             mpv-308   [002]   171.290177: regmap_reg_read:      2-001c reg=124 val=420
             mpv-308   [002]   171.290180: regmap_reg_write:     2-001c reg=124 val=220
             mpv-308   [002]   171.290185: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.290800: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-308   [002]   171.290804: regmap_reg_read:      2-001c reg=6a val=24
             mpv-308   [002]   171.290807: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-308   [002]   171.291300: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-308   [002]   171.291333: regmap_reg_read_cache: 2-001c reg=8f val=1100
             mpv-308   [002]   171.291335: regmap_reg_write:     2-001c reg=8f val=3100
             mpv-308   [002]   171.291339: regmap_hw_write_start: 2-001c reg=8f count=1
             mpv-308   [002]   171.291802: regmap_hw_write_done: 2-001c reg=8f count=1
             mpv-308   [002]   171.291830: regmap_reg_read_cache: 2-001c reg=8e val=4
             mpv-308   [002]   171.291833: regmap_reg_write:     2-001c reg=8e val=9
             mpv-308   [002]   171.291838: regmap_hw_write_start: 2-001c reg=8e count=1
             mpv-308   [002]   171.292433: regmap_hw_write_done: 2-001c reg=8e count=1
             mpv-308   [002]   171.292461: regmap_reg_write:     2-001c reg=177 val=9f00
             mpv-308   [002]   171.292466: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.293274: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-308   [002]   171.293278: regmap_reg_read:      2-001c reg=6a val=24
             mpv-308   [002]   171.293281: regmap_reg_write:     2-001c reg=6a val=77
             mpv-308   [002]   171.293284: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.293894: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-308   [002]   171.293897: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-308   [002]   171.294484: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-308   [002]   171.294510: regmap_reg_read_cache: 2-001c reg=63 val=e8d8
             mpv-308   [002]   171.294513: regmap_reg_write:     2-001c reg=63 val=a8d0
             mpv-308   [002]   171.294516: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-308   [002]   171.294976: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-308   [002]   171.295001: regmap_reg_read_cache: 2-001c reg=63 val=a8d0
             mpv-308   [002]   171.295004: regmap_reg_write:     2-001c reg=63 val=a8f0
             mpv-308   [002]   171.295006: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-308   [002]   171.295680: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-308   [002]   171.306100: regmap_reg_read_cache: 2-001c reg=63 val=a8f0
             mpv-308   [002]   171.306108: regmap_reg_write:     2-001c reg=63 val=e8f8
             mpv-308   [002]   171.306114: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-308   [002]   171.306885: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-308   [002]   171.306954: regmap_reg_read_cache: 2-001c reg=8f val=3100
             mpv-308   [002]   171.306957: regmap_reg_write:     2-001c reg=8f val=1140
             mpv-308   [002]   171.306961: regmap_hw_write_start: 2-001c reg=8f count=1
             mpv-308   [002]   171.307532: regmap_hw_write_done: 2-001c reg=8f count=1
             mpv-308   [002]   171.307559: regmap_reg_read_cache: 2-001c reg=91 val=c00
             mpv-308   [002]   171.307561: regmap_reg_write:     2-001c reg=91 val=e00
             mpv-308   [002]   171.307564: regmap_hw_write_start: 2-001c reg=91 count=1
             mpv-308   [002]   171.308068: regmap_hw_write_done: 2-001c reg=91 count=1
             mpv-308   [002]   171.308093: regmap_reg_read_cache: 2-001c reg=90 val=646
             mpv-308   [002]   171.308096: regmap_reg_write:     2-001c reg=90 val=737
             mpv-308   [002]   171.308099: regmap_hw_write_start: 2-001c reg=90 count=1
             mpv-308   [002]   171.308573: regmap_hw_write_done: 2-001c reg=90 count=1
             mpv-308   [002]   171.308600: regmap_reg_write:     2-001c reg=137 val=1c00
             mpv-308   [002]   171.308607: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.309204: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-308   [002]   171.309212: regmap_reg_read:      2-001c reg=6a val=77
             mpv-308   [002]   171.309215: regmap_reg_write:     2-001c reg=6a val=37
             mpv-308   [002]   171.309217: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.309994: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-308   [002]   171.309999: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-308   [002]   171.310714: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-308   [002]   171.310747: regmap_reg_read_cache: 2-001c reg=8e val=9
             mpv-308   [002]   171.310750: regmap_reg_write:     2-001c reg=8e val=5
             mpv-308   [002]   171.310755: regmap_hw_write_start: 2-001c reg=8e count=1
             mpv-308   [002]   171.311331: regmap_hw_write_done: 2-001c reg=8e count=1
             mpv-308   [002]   171.311361: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.312384: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-308   [002]   171.312388: regmap_reg_read:      2-001c reg=6a val=37
             mpv-308   [002]   171.312391: regmap_reg_write:     2-001c reg=6a val=24
             mpv-308   [002]   171.312394: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.312891: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-308   [002]   171.312897: regmap_hw_read_start: 2-001c reg=6c count=1
             mpv-308   [002]   171.313657: regmap_hw_read_done:  2-001c reg=6c count=1
             mpv-308   [002]   171.313661: regmap_reg_read:      2-001c reg=124 val=220
             mpv-308   [002]   171.313664: regmap_reg_write:     2-001c reg=124 val=420
             mpv-308   [002]   171.313667: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-308   [002]   171.314977: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-308   [002]   171.314984: regmap_reg_read:      2-001c reg=6a val=24
             mpv-308   [002]   171.314990: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-308   [002]   171.315479: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-308   [002]   171.315519: regmap_reg_read_cache: 2-001c reg=2 val=cbcb
             mpv-308   [002]   171.315522: regmap_reg_write:     2-001c reg=2 val=4b4b
             mpv-308   [002]   171.315525: regmap_hw_write_start: 2-001c reg=2 count=1
             mpv-308   [002]   171.316002: regmap_hw_write_done: 2-001c reg=2 count=1
          mpv/ao-318   [003]   171.744407: regmap_reg_read_cache: 70080000.ahub reg=0 val=70777
          mpv/ao-318   [003]   171.744424: regmap_reg_write:     70080000.ahub reg=0 val=80070777
          mpv/ao-318   [003]   171.744433: regmap_reg_read_cache: 70080400.i2s reg=0 val=403
          mpv/ao-318   [003]   171.744435: regmap_reg_write:     70080400.i2s reg=0 val=80000403
             mpv-308   [002]   173.755178: regmap_reg_read_cache: 70080000.ahub reg=0 val=80070777
             mpv-308   [002]   173.755188: regmap_reg_write:     70080000.ahub reg=0 val=70777
             mpv-308   [002]   173.755196: regmap_reg_read_cache: 70080400.i2s reg=0 val=80000403
             mpv-308   [002]   173.755198: regmap_reg_write:     70080400.i2s reg=0 val=403
Broken
------

# cat /sys/class/i2c-dev/i2c-2/name
7000d000.i2c

...
	i2c@7000d000 {
		clock-frequency = <100000>;
		status = "okay";

		rt5640: rt5640@1c {
			compatible = "realtek,rt5640";
			reg = <0x1c>;

			interrupt-parent = <&gpio>;
			interrupts = <TEGRA_GPIO(W, 3) IRQ_TYPE_EDGE_FALLING>;

			realtek,dmic1-data-pin = <1>;
			realtek,dmic2-data-pin = <0>;
			realtek,in1-differential;
		};
...

# cat /proc/asound/card0/pcm0p/sub0/hw_params
access: MMAP_INTERLEAVED
format: S24_LE
subformat: STD
channels: 2
rate: 48000 (48000/1)
period_size: 512
buffer_size: 4096

# trace-cmd record -e regmap:*
# trace-cmd report

CPU 0 is empty
CPU 1 is empty
cpus=4
             mpv-281   [002]    40.227541: regmap_cache_only:    70080000.ahub flag=0
             mpv-281   [002]    40.227554: regmap_cache_only:    70080000.ahub flag=0
             mpv-281   [002]    40.227572: regmap_cache_only:    70080400.i2s flag=0
             mpv-281   [002]    40.236905: regmap_reg_read_cache: 2-001c reg=64 val=0
             mpv-281   [002]    40.236921: regmap_reg_read_cache: 2-001c reg=80 val=0
             mpv-281   [002]    40.236931: regmap_reg_read_cache: 2-001c reg=70 val=8000
             mpv-281   [002]    40.236935: regmap_reg_read_cache: 2-001c reg=70 val=8000
             mpv-281   [002]    40.236939: regmap_reg_write:     2-001c reg=70 val=8008
             mpv-281   [002]    40.236950: regmap_hw_write_start: 2-001c reg=70 count=1
             mpv-281   [002]    40.237776: regmap_hw_write_done: 2-001c reg=70 count=1
             mpv-281   [002]    40.237828: regmap_reg_read_cache: 2-001c reg=73 val=1114
             mpv-281   [002]    40.237831: regmap_reg_write:     2-001c reg=73 val=8114
             mpv-281   [002]    40.237836: regmap_hw_write_start: 2-001c reg=73 count=1
             mpv-281   [002]    40.241723: regmap_hw_write_done: 2-001c reg=73 count=1
             mpv-281   [002]    40.241794: regmap_reg_read_cache: 70080400.i2s reg=0 val=400
             mpv-281   [002]    40.241798: regmap_reg_write:     70080400.i2s reg=0 val=405
             mpv-281   [002]    40.241817: regmap_reg_write:     70080400.i2s reg=4 val=2f
             mpv-281   [002]    40.241820: regmap_reg_write:     70080400.i2s reg=14 val=1015504
             mpv-281   [002]    40.241823: regmap_reg_write:     70080400.i2s reg=8 val=10001
    kworker/u8:1-36    [003]    40.242987: regmap_reg_read_cache: 2-001c reg=63 val=0
    kworker/u8:1-36    [003]    40.242992: regmap_reg_write:     2-001c reg=63 val=a810
    kworker/u8:1-36    [003]    40.243002: regmap_hw_write_start: 2-001c reg=63 count=1
    kworker/u8:1-36    [003]    40.243519: regmap_hw_write_done: 2-001c reg=63 count=1
    kworker/u8:1-36    [003]    40.256915: regmap_reg_read_cache: 2-001c reg=63 val=a810
    kworker/u8:1-36    [003]    40.256924: regmap_reg_write:     2-001c reg=63 val=e818
    kworker/u8:1-36    [003]    40.256933: regmap_hw_write_start: 2-001c reg=63 count=1
    kworker/u8:1-36    [003]    40.257590: regmap_hw_write_done: 2-001c reg=63 count=1
    kworker/u8:1-36    [003]    40.257597: regmap_reg_read_cache: 2-001c reg=fa val=3f01
    kworker/u8:1-36    [003]    40.257600: regmap_reg_read_cache: 2-001c reg=93 val=3030
             mpv-281   [002]    40.257670: regmap_reg_read_cache: 2-001c reg=61 val=0
             mpv-281   [002]    40.257674: regmap_reg_write:     2-001c reg=61 val=9800
             mpv-281   [002]    40.257678: regmap_hw_write_start: 2-001c reg=61 count=1
             mpv-281   [002]    40.258409: regmap_hw_write_done: 2-001c reg=61 count=1
             mpv-281   [002]    40.258448: regmap_reg_read_cache: 2-001c reg=63 val=e818
             mpv-281   [002]    40.258451: regmap_reg_write:     2-001c reg=63 val=e8d8
             mpv-281   [002]    40.258454: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-281   [002]    40.259701: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-281   [002]    40.259751: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.260357: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-281   [002]    40.260361: regmap_reg_read:      2-001c reg=6a val=23
             mpv-281   [002]    40.260365: regmap_reg_write:     2-001c reg=6a val=24
             mpv-281   [002]    40.260367: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.260881: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-281   [002]    40.260885: regmap_hw_read_start: 2-001c reg=6c count=1
             mpv-281   [002]    40.263245: regmap_hw_read_done:  2-001c reg=6c count=1
             mpv-281   [002]    40.263251: regmap_reg_read:      2-001c reg=124 val=420
             mpv-281   [002]    40.263255: regmap_reg_write:     2-001c reg=124 val=220
             mpv-281   [002]    40.263260: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.264325: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-281   [002]    40.264330: regmap_reg_read:      2-001c reg=6a val=24
             mpv-281   [002]    40.264334: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-281   [002]    40.264827: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-281   [002]    40.264859: regmap_reg_read_cache: 2-001c reg=8f val=1100
             mpv-281   [002]    40.264867: regmap_reg_write:     2-001c reg=8f val=3100
             mpv-281   [002]    40.264871: regmap_hw_write_start: 2-001c reg=8f count=1
             mpv-281   [002]    40.265939: regmap_hw_write_done: 2-001c reg=8f count=1
             mpv-281   [002]    40.265976: regmap_reg_read_cache: 2-001c reg=8e val=4
             mpv-281   [002]    40.265981: regmap_reg_write:     2-001c reg=8e val=9
             mpv-281   [002]    40.265986: regmap_hw_write_start: 2-001c reg=8e count=1
             mpv-281   [002]    40.267142: regmap_hw_write_done: 2-001c reg=8e count=1
             mpv-281   [002]    40.267172: regmap_reg_write:     2-001c reg=177 val=9f00
             mpv-281   [002]    40.267182: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.267842: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-281   [002]    40.267845: regmap_reg_read:      2-001c reg=6a val=24
             mpv-281   [002]    40.267848: regmap_reg_write:     2-001c reg=6a val=77
             mpv-281   [002]    40.267851: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.268937: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-281   [002]    40.268943: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-281   [002]    40.269454: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-281   [002]    40.269484: regmap_reg_read_cache: 2-001c reg=63 val=e8d8
             mpv-281   [002]    40.269487: regmap_reg_write:     2-001c reg=63 val=a8d0
             mpv-281   [002]    40.269490: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-281   [002]    40.270012: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-281   [002]    40.271740: regmap_reg_read_cache: 2-001c reg=63 val=a8d0
             mpv-281   [002]    40.271748: regmap_reg_write:     2-001c reg=63 val=a8f0
             mpv-281   [002]    40.271753: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-281   [002]    40.272240: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-281   [002]    40.286888: regmap_reg_read_cache: 2-001c reg=63 val=a8f0
             mpv-281   [002]    40.286901: regmap_reg_write:     2-001c reg=63 val=e8f8
             mpv-281   [002]    40.286917: regmap_hw_write_start: 2-001c reg=63 count=1
             mpv-281   [002]    40.287748: regmap_hw_write_done: 2-001c reg=63 count=1
             mpv-281   [002]    40.287841: regmap_reg_read_cache: 2-001c reg=8f val=3100
             mpv-281   [002]    40.287844: regmap_reg_write:     2-001c reg=8f val=1140
             mpv-281   [002]    40.287847: regmap_hw_write_start: 2-001c reg=8f count=1
             mpv-281   [002]    40.288310: regmap_hw_write_done: 2-001c reg=8f count=1
             mpv-281   [002]    40.288339: regmap_reg_read_cache: 2-001c reg=91 val=c00
             mpv-281   [002]    40.288341: regmap_reg_write:     2-001c reg=91 val=e00
             mpv-281   [002]    40.288344: regmap_hw_write_start: 2-001c reg=91 count=1
             mpv-281   [002]    40.288808: regmap_hw_write_done: 2-001c reg=91 count=1
             mpv-281   [002]    40.288838: regmap_reg_read_cache: 2-001c reg=90 val=646
             mpv-281   [002]    40.288840: regmap_reg_write:     2-001c reg=90 val=737
             mpv-281   [002]    40.288844: regmap_hw_write_start: 2-001c reg=90 count=1
             mpv-281   [002]    40.289792: regmap_hw_write_done: 2-001c reg=90 count=1
             mpv-281   [002]    40.289828: regmap_reg_write:     2-001c reg=137 val=1c00
             mpv-281   [002]    40.289837: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.291772: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-281   [002]    40.291782: regmap_reg_read:      2-001c reg=6a val=77
             mpv-281   [002]    40.291788: regmap_reg_write:     2-001c reg=6a val=37
             mpv-281   [002]    40.291792: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.292320: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-281   [002]    40.292324: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-281   [002]    40.293579: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-281   [002]    40.293616: regmap_reg_read_cache: 2-001c reg=8e val=9
             mpv-281   [002]    40.293618: regmap_reg_write:     2-001c reg=8e val=5
             mpv-281   [002]    40.293623: regmap_hw_write_start: 2-001c reg=8e count=1
             mpv-281   [002]    40.294179: regmap_hw_write_done: 2-001c reg=8e count=1
             mpv-281   [002]    40.294211: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.295183: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-281   [002]    40.295187: regmap_reg_read:      2-001c reg=6a val=37
             mpv-281   [002]    40.295194: regmap_reg_write:     2-001c reg=6a val=24
             mpv-281   [002]    40.295196: regmap_hw_write_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.296301: regmap_hw_write_done: 2-001c reg=6a count=1
             mpv-281   [002]    40.296308: regmap_hw_read_start: 2-001c reg=6c count=1
             mpv-281   [002]    40.298769: regmap_hw_read_done:  2-001c reg=6c count=1
             mpv-281   [002]    40.298777: regmap_reg_read:      2-001c reg=124 val=220
             mpv-281   [002]    40.298784: regmap_reg_write:     2-001c reg=124 val=420
             mpv-281   [002]    40.298790: regmap_hw_read_start: 2-001c reg=6a count=1
             mpv-281   [002]    40.299542: regmap_hw_read_done:  2-001c reg=6a count=1
             mpv-281   [002]    40.299549: regmap_reg_read:      2-001c reg=6a val=24
             mpv-281   [002]    40.299555: regmap_hw_write_start: 2-001c reg=6c count=1
             mpv-281   [002]    40.300054: regmap_hw_write_done: 2-001c reg=6c count=1
             mpv-281   [002]    40.300107: regmap_reg_read_cache: 2-001c reg=2 val=cbcb
             mpv-281   [002]    40.300110: regmap_reg_write:     2-001c reg=2 val=4b4b
             mpv-281   [002]    40.300115: regmap_hw_write_start: 2-001c reg=2 count=1
             mpv-281   [002]    40.300756: regmap_hw_write_done: 2-001c reg=2 count=1
          mpv/ao-290   [003]    40.721759: regmap_reg_read_cache: 70080000.ahub reg=0 val=70777
          mpv/ao-290   [003]    40.721776: regmap_reg_write:     70080000.ahub reg=0 val=80070777
          mpv/ao-290   [003]    40.721787: regmap_reg_read_cache: 70080400.i2s reg=0 val=405
          mpv/ao-290   [003]    40.721789: regmap_reg_write:     70080400.i2s reg=0 val=80000405
             mpv-281   [002]    41.693159: regmap_reg_read_cache: 70080000.ahub reg=0 val=80070777
             mpv-281   [002]    41.693185: regmap_reg_write:     70080000.ahub reg=0 val=70777
             mpv-281   [002]    41.693200: regmap_reg_read_cache: 70080400.i2s reg=0 val=80000405
             mpv-281   [002]    41.693203: regmap_reg_write:     70080400.i2s reg=0 val=405
Ben Dooks Dec. 20, 2019, 2:56 p.m. UTC | #12
On 20/12/2019 14:43, Dmitry Osipenko wrote:
> 20.12.2019 16:57, Jon Hunter пишет:
>>
>> On 20/12/2019 11:38, Ben Dooks wrote:
>>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>>
>>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>
>>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add the
>>>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>>>> S32_LE
>>>>>>>>> formats when requested.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>>> ---
>>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>>>> needed
>>>>>>>>> in tdm code
>>>>>>>>>
>>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>>> ---
>>>>>>>>>     sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>>>>     1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>         struct device *dev = dai->dev;
>>>>>>>>>         struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>>>>         unsigned int mask, val, reg;
>>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>>>         struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>>           if (params_channels(params) != 2)
>>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>         switch (params_format(params)) {
>>>>>>>>>         case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>>             val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>             sample_size = 16;
>>>>>>>>>             break;
>>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>>> +        sample_size = 24;
>>>>>>>>> +        break;
>>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>>> +        sample_size = 32;
>>>>>>>>> +        break;
>>>>>>>>>         default:
>>>>>>>>>             return -EINVAL;
>>>>>>>>>         }
>>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>         cif_conf.threshold = 0;
>>>>>>>>>         cif_conf.audio_channels = 2;
>>>>>>>>>         cif_conf.client_channels = 2;
>>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>>         cif_conf.expand = 0;
>>>>>>>>>         cif_conf.stereo_conv = 0;
>>>>>>>>>         cif_conf.replicate = 0;
>>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>>             .channels_min = 2,
>>>>>>>>>             .channels_max = 2,
>>>>>>>>>             .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>         },
>>>>>>>>>         .capture = {
>>>>>>>>>             .stream_name = "Capture",
>>>>>>>>>             .channels_min = 2,
>>>>>>>>>             .channels_max = 2,
>>>>>>>>>             .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>         },
>>>>>>>>>         .ops = &tegra30_i2s_dai_ops,
>>>>>>>>>         .symmetric_rates = 1,
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> This patch breaks audio on Tegra30. I don't see errors anywhere, but
>>>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>>>
>>>>>>> What is the failure mode? I can try and take a look at this some time
>>>>>>> this week, but I am not sure if I have any boards with an actual
>>>>>>> useful
>>>>>>> audio output?
>>>>>>
>>>>>> The failure mode is that there no sound. I also noticed that video
>>>>>> playback stutters a lot if movie file has audio track, seems something
>>>>>> times out during of the audio playback. For now I don't have any
>>>>>> more info.
>>>>>>
>>>>>
>>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>>
>>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>>
>>>>
>>>> Given that the audio drivers uses regmap, it could be good to dump the
>>>> I2S/AHUB registers while the clip if playing with and without this patch
>>>> to see the differences. I am curious if the audio is now being played as
>>>> 24 or 32-bit instead of 16-bit now these are available.
>>>>
>>>> You could also dump the hw_params to see the format while playing as
>>>> well ...
>>>>
>>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>>
>>> I suppose it is also possible that the codec isn't properly doing >16
>>> bits and the fact we now offer 24 and 32 could be an issue. I've not
>>> got anything with an audio output on it that would be easy to test.
>>
>> I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
>> back. However, admittedly I may have only done simple 16-bit testing
>> with speaker-test.
>>
>> We do verify that all soundcards are detected and registered as expected
>> during daily testing, but at the moment we don't have anything that
>> verifies actual playback.
> 
> Please take a look at the attached logs.

I wonder if we are falling into FIFO configuration issues with the
non-16 bit case.

Can you try adding the following two patches?
Dmitry Osipenko Dec. 20, 2019, 3:02 p.m. UTC | #13
20.12.2019 17:56, Ben Dooks пишет:
> On 20/12/2019 14:43, Dmitry Osipenko wrote:
>> 20.12.2019 16:57, Jon Hunter пишет:
>>>
>>> On 20/12/2019 11:38, Ben Dooks wrote:
>>>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>>>
>>>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>
>>>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add
>>>>>>>>>> the
>>>>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>>>>> S32_LE
>>>>>>>>>> formats when requested.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>>>> ---
>>>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>>>>> needed
>>>>>>>>>> in tdm code
>>>>>>>>>>
>>>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>>>> ---
>>>>>>>>>>     sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>>>>>     1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>         struct device *dev = dai->dev;
>>>>>>>>>>         struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>>>>>         unsigned int mask, val, reg;
>>>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>>>>         struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>>>           if (params_channels(params) != 2)
>>>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>         switch (params_format(params)) {
>>>>>>>>>>         case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>>>             val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>             sample_size = 16;
>>>>>>>>>>             break;
>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>>>> +        sample_size = 24;
>>>>>>>>>> +        break;
>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>>>> +        sample_size = 32;
>>>>>>>>>> +        break;
>>>>>>>>>>         default:
>>>>>>>>>>             return -EINVAL;
>>>>>>>>>>         }
>>>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>         cif_conf.threshold = 0;
>>>>>>>>>>         cif_conf.audio_channels = 2;
>>>>>>>>>>         cif_conf.client_channels = 2;
>>>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>>>         cif_conf.expand = 0;
>>>>>>>>>>         cif_conf.stereo_conv = 0;
>>>>>>>>>>         cif_conf.replicate = 0;
>>>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>>>             .channels_min = 2,
>>>>>>>>>>             .channels_max = 2,
>>>>>>>>>>             .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>         },
>>>>>>>>>>         .capture = {
>>>>>>>>>>             .stream_name = "Capture",
>>>>>>>>>>             .channels_min = 2,
>>>>>>>>>>             .channels_max = 2,
>>>>>>>>>>             .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>         },
>>>>>>>>>>         .ops = &tegra30_i2s_dai_ops,
>>>>>>>>>>         .symmetric_rates = 1,
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> This patch breaks audio on Tegra30. I don't see errors
>>>>>>>>> anywhere, but
>>>>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>>>>
>>>>>>>> What is the failure mode? I can try and take a look at this some
>>>>>>>> time
>>>>>>>> this week, but I am not sure if I have any boards with an actual
>>>>>>>> useful
>>>>>>>> audio output?
>>>>>>>
>>>>>>> The failure mode is that there no sound. I also noticed that video
>>>>>>> playback stutters a lot if movie file has audio track, seems
>>>>>>> something
>>>>>>> times out during of the audio playback. For now I don't have any
>>>>>>> more info.
>>>>>>>
>>>>>>
>>>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>>>
>>>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>>>
>>>>>>
>>>>>
>>>>> Given that the audio drivers uses regmap, it could be good to dump the
>>>>> I2S/AHUB registers while the clip if playing with and without this
>>>>> patch
>>>>> to see the differences. I am curious if the audio is now being
>>>>> played as
>>>>> 24 or 32-bit instead of 16-bit now these are available.
>>>>>
>>>>> You could also dump the hw_params to see the format while playing as
>>>>> well ...
>>>>>
>>>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>>>
>>>> I suppose it is also possible that the codec isn't properly doing >16
>>>> bits and the fact we now offer 24 and 32 could be an issue. I've not
>>>> got anything with an audio output on it that would be easy to test.
>>>
>>> I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
>>> back. However, admittedly I may have only done simple 16-bit testing
>>> with speaker-test.
>>>
>>> We do verify that all soundcards are detected and registered as expected
>>> during daily testing, but at the moment we don't have anything that
>>> verifies actual playback.
>>
>> Please take a look at the attached logs.
> 
> I wonder if we are falling into FIFO configuration issues with the
> non-16 bit case.
> 
> Can you try adding the following two patches?

It is much better now! There is no horrible noise and no stuttering, but
audio still has a "robotic" effect, like freq isn't correct.
Ben Dooks Dec. 20, 2019, 3:25 p.m. UTC | #14
On 20/12/2019 15:02, Dmitry Osipenko wrote:
> 20.12.2019 17:56, Ben Dooks пишет:
>> On 20/12/2019 14:43, Dmitry Osipenko wrote:
>>> 20.12.2019 16:57, Jon Hunter пишет:
>>>>
>>>> On 20/12/2019 11:38, Ben Dooks wrote:
>>>>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>>>>
>>>>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>
>>>>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add
>>>>>>>>>>> the
>>>>>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>>>>>> S32_LE
>>>>>>>>>>> formats when requested.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>>>>> ---
>>>>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>>>>>> needed
>>>>>>>>>>> in tdm code
>>>>>>>>>>>
>>>>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>>>>> ---
>>>>>>>>>>>      sound/soc/tegra/tegra30_i2s.c | 25 ++++++++++++++++++++-----
>>>>>>>>>>>      1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>          struct device *dev = dai->dev;
>>>>>>>>>>>          struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>>>>>>>>>>>          unsigned int mask, val, reg;
>>>>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>>>>>          struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>>>>            if (params_channels(params) != 2)
>>>>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>          switch (params_format(params)) {
>>>>>>>>>>>          case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>>>>              val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>              sample_size = 16;
>>>>>>>>>>>              break;
>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>>>>> +        sample_size = 24;
>>>>>>>>>>> +        break;
>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>>>>> +        sample_size = 32;
>>>>>>>>>>> +        break;
>>>>>>>>>>>          default:
>>>>>>>>>>>              return -EINVAL;
>>>>>>>>>>>          }
>>>>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>          cif_conf.threshold = 0;
>>>>>>>>>>>          cif_conf.audio_channels = 2;
>>>>>>>>>>>          cif_conf.client_channels = 2;
>>>>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>>>>          cif_conf.expand = 0;
>>>>>>>>>>>          cif_conf.stereo_conv = 0;
>>>>>>>>>>>          cif_conf.replicate = 0;
>>>>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>>>>              .channels_min = 2,
>>>>>>>>>>>              .channels_max = 2,
>>>>>>>>>>>              .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>          },
>>>>>>>>>>>          .capture = {
>>>>>>>>>>>              .stream_name = "Capture",
>>>>>>>>>>>              .channels_min = 2,
>>>>>>>>>>>              .channels_max = 2,
>>>>>>>>>>>              .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>          },
>>>>>>>>>>>          .ops = &tegra30_i2s_dai_ops,
>>>>>>>>>>>          .symmetric_rates = 1,
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> This patch breaks audio on Tegra30. I don't see errors
>>>>>>>>>> anywhere, but
>>>>>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>>>>>
>>>>>>>>> What is the failure mode? I can try and take a look at this some
>>>>>>>>> time
>>>>>>>>> this week, but I am not sure if I have any boards with an actual
>>>>>>>>> useful
>>>>>>>>> audio output?
>>>>>>>>
>>>>>>>> The failure mode is that there no sound. I also noticed that video
>>>>>>>> playback stutters a lot if movie file has audio track, seems
>>>>>>>> something
>>>>>>>> times out during of the audio playback. For now I don't have any
>>>>>>>> more info.
>>>>>>>>
>>>>>>>
>>>>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>>>>
>>>>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Given that the audio drivers uses regmap, it could be good to dump the
>>>>>> I2S/AHUB registers while the clip if playing with and without this
>>>>>> patch
>>>>>> to see the differences. I am curious if the audio is now being
>>>>>> played as
>>>>>> 24 or 32-bit instead of 16-bit now these are available.
>>>>>>
>>>>>> You could also dump the hw_params to see the format while playing as
>>>>>> well ...
>>>>>>
>>>>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>>>>
>>>>> I suppose it is also possible that the codec isn't properly doing >16
>>>>> bits and the fact we now offer 24 and 32 could be an issue. I've not
>>>>> got anything with an audio output on it that would be easy to test.
>>>>
>>>> I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
>>>> back. However, admittedly I may have only done simple 16-bit testing
>>>> with speaker-test.
>>>>
>>>> We do verify that all soundcards are detected and registered as expected
>>>> during daily testing, but at the moment we don't have anything that
>>>> verifies actual playback.
>>>
>>> Please take a look at the attached logs.
>>
>> I wonder if we are falling into FIFO configuration issues with the
>> non-16 bit case.
>>
>> Can you try adding the following two patches?
> 
> It is much better now! There is no horrible noise and no stuttering, but
> audio still has a "robotic" effect, like freq isn't correct.

I wonder if there's an issue with FIFO stoking? I should also possibly
add the correctly stop FIFOs patch as well.
Dmitry Osipenko Dec. 20, 2019, 4:40 p.m. UTC | #15
20.12.2019 18:25, Ben Dooks пишет:
> On 20/12/2019 15:02, Dmitry Osipenko wrote:
>> 20.12.2019 17:56, Ben Dooks пишет:
>>> On 20/12/2019 14:43, Dmitry Osipenko wrote:
>>>> 20.12.2019 16:57, Jon Hunter пишет:
>>>>>
>>>>> On 20/12/2019 11:38, Ben Dooks wrote:
>>>>>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>>>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>>
>>>>>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add
>>>>>>>>>>>> the
>>>>>>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>>>>>>> S32_LE
>>>>>>>>>>>> formats when requested.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>>>>>> ---
>>>>>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>>>>>>> needed
>>>>>>>>>>>> in tdm code
>>>>>>>>>>>>
>>>>>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>>>>>> ---
>>>>>>>>>>>>      sound/soc/tegra/tegra30_i2s.c | 25
>>>>>>>>>>>> ++++++++++++++++++++-----
>>>>>>>>>>>>      1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>          struct device *dev = dai->dev;
>>>>>>>>>>>>          struct tegra30_i2s *i2s =
>>>>>>>>>>>> snd_soc_dai_get_drvdata(dai);
>>>>>>>>>>>>          unsigned int mask, val, reg;
>>>>>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>>>>>>          struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>>>>>            if (params_channels(params) != 2)
>>>>>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>          switch (params_format(params)) {
>>>>>>>>>>>>          case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>>>>>              val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>              sample_size = 16;
>>>>>>>>>>>>              break;
>>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>>>>>> +        sample_size = 24;
>>>>>>>>>>>> +        break;
>>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>>>>>> +        sample_size = 32;
>>>>>>>>>>>> +        break;
>>>>>>>>>>>>          default:
>>>>>>>>>>>>              return -EINVAL;
>>>>>>>>>>>>          }
>>>>>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>          cif_conf.threshold = 0;
>>>>>>>>>>>>          cif_conf.audio_channels = 2;
>>>>>>>>>>>>          cif_conf.client_channels = 2;
>>>>>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>>>>>          cif_conf.expand = 0;
>>>>>>>>>>>>          cif_conf.stereo_conv = 0;
>>>>>>>>>>>>          cif_conf.replicate = 0;
>>>>>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>>>>>              .channels_min = 2,
>>>>>>>>>>>>              .channels_max = 2,
>>>>>>>>>>>>              .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>          },
>>>>>>>>>>>>          .capture = {
>>>>>>>>>>>>              .stream_name = "Capture",
>>>>>>>>>>>>              .channels_min = 2,
>>>>>>>>>>>>              .channels_max = 2,
>>>>>>>>>>>>              .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>          },
>>>>>>>>>>>>          .ops = &tegra30_i2s_dai_ops,
>>>>>>>>>>>>          .symmetric_rates = 1,
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> This patch breaks audio on Tegra30. I don't see errors
>>>>>>>>>>> anywhere, but
>>>>>>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>>>>>>
>>>>>>>>>> What is the failure mode? I can try and take a look at this some
>>>>>>>>>> time
>>>>>>>>>> this week, but I am not sure if I have any boards with an actual
>>>>>>>>>> useful
>>>>>>>>>> audio output?
>>>>>>>>>
>>>>>>>>> The failure mode is that there no sound. I also noticed that video
>>>>>>>>> playback stutters a lot if movie file has audio track, seems
>>>>>>>>> something
>>>>>>>>> times out during of the audio playback. For now I don't have any
>>>>>>>>> more info.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>>>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>>>>>
>>>>>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Given that the audio drivers uses regmap, it could be good to
>>>>>>> dump the
>>>>>>> I2S/AHUB registers while the clip if playing with and without this
>>>>>>> patch
>>>>>>> to see the differences. I am curious if the audio is now being
>>>>>>> played as
>>>>>>> 24 or 32-bit instead of 16-bit now these are available.
>>>>>>>
>>>>>>> You could also dump the hw_params to see the format while playing as
>>>>>>> well ...
>>>>>>>
>>>>>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>>>>>
>>>>>> I suppose it is also possible that the codec isn't properly doing >16
>>>>>> bits and the fact we now offer 24 and 32 could be an issue. I've not
>>>>>> got anything with an audio output on it that would be easy to test.
>>>>>
>>>>> I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
>>>>> back. However, admittedly I may have only done simple 16-bit testing
>>>>> with speaker-test.
>>>>>
>>>>> We do verify that all soundcards are detected and registered as
>>>>> expected
>>>>> during daily testing, but at the moment we don't have anything that
>>>>> verifies actual playback.
>>>>
>>>> Please take a look at the attached logs.
>>>
>>> I wonder if we are falling into FIFO configuration issues with the
>>> non-16 bit case.
>>>
>>> Can you try adding the following two patches?
>>
>> It is much better now! There is no horrible noise and no stuttering, but
>> audio still has a "robotic" effect, like freq isn't correct.
> 
> I wonder if there's an issue with FIFO stoking? I should also possibly
> add the correctly stop FIFOs patch as well.

I'll be happy to try more patches.

Meanwhile sound on v5.5+ is broken and thus the incomplete patches
should be reverted.
Ben Dooks Dec. 20, 2019, 5:06 p.m. UTC | #16
On 20/12/2019 16:40, Dmitry Osipenko wrote:
> 20.12.2019 18:25, Ben Dooks пишет:
>> On 20/12/2019 15:02, Dmitry Osipenko wrote:
>>> 20.12.2019 17:56, Ben Dooks пишет:
>>>> On 20/12/2019 14:43, Dmitry Osipenko wrote:
>>>>> 20.12.2019 16:57, Jon Hunter пишет:
>>>>>>
>>>>>> On 20/12/2019 11:38, Ben Dooks wrote:
>>>>>>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>>>>>>
>>>>>>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>>>>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>>>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>>>
>>>>>>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so add
>>>>>>>>>>>>> the
>>>>>>>>>>>>> option to the tegra30_i2s_hw_params to configure the S24_LE or
>>>>>>>>>>>>> S32_LE
>>>>>>>>>>>>> formats when requested.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is
>>>>>>>>>>>>> needed
>>>>>>>>>>>>> in tdm code
>>>>>>>>>>>>>
>>>>>>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>       sound/soc/tegra/tegra30_i2s.c | 25
>>>>>>>>>>>>> ++++++++++++++++++++-----
>>>>>>>>>>>>>       1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>>           struct device *dev = dai->dev;
>>>>>>>>>>>>>           struct tegra30_i2s *i2s =
>>>>>>>>>>>>> snd_soc_dai_get_drvdata(dai);
>>>>>>>>>>>>>           unsigned int mask, val, reg;
>>>>>>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
>>>>>>>>>>>>>           struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>>>>>>             if (params_channels(params) != 2)
>>>>>>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>>           switch (params_format(params)) {
>>>>>>>>>>>>>           case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>>>>>>               val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>>               sample_size = 16;
>>>>>>>>>>>>>               break;
>>>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>>>>>>> +        sample_size = 24;
>>>>>>>>>>>>> +        break;
>>>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>>>>>>> +        sample_size = 32;
>>>>>>>>>>>>> +        break;
>>>>>>>>>>>>>           default:
>>>>>>>>>>>>>               return -EINVAL;
>>>>>>>>>>>>>           }
>>>>>>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>>           cif_conf.threshold = 0;
>>>>>>>>>>>>>           cif_conf.audio_channels = 2;
>>>>>>>>>>>>>           cif_conf.client_channels = 2;
>>>>>>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>>>>>>           cif_conf.expand = 0;
>>>>>>>>>>>>>           cif_conf.stereo_conv = 0;
>>>>>>>>>>>>>           cif_conf.replicate = 0;
>>>>>>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>>>>>>               .channels_min = 2,
>>>>>>>>>>>>>               .channels_max = 2,
>>>>>>>>>>>>>               .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>>           },
>>>>>>>>>>>>>           .capture = {
>>>>>>>>>>>>>               .stream_name = "Capture",
>>>>>>>>>>>>>               .channels_min = 2,
>>>>>>>>>>>>>               .channels_max = 2,
>>>>>>>>>>>>>               .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>>           },
>>>>>>>>>>>>>           .ops = &tegra30_i2s_dai_ops,
>>>>>>>>>>>>>           .symmetric_rates = 1,
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Hello,
>>>>>>>>>>>>
>>>>>>>>>>>> This patch breaks audio on Tegra30. I don't see errors
>>>>>>>>>>>> anywhere, but
>>>>>>>>>>>> there is no audio and reverting this patch helps. Please fix it.
>>>>>>>>>>>
>>>>>>>>>>> What is the failure mode? I can try and take a look at this some
>>>>>>>>>>> time
>>>>>>>>>>> this week, but I am not sure if I have any boards with an actual
>>>>>>>>>>> useful
>>>>>>>>>>> audio output?
>>>>>>>>>>
>>>>>>>>>> The failure mode is that there no sound. I also noticed that video
>>>>>>>>>> playback stutters a lot if movie file has audio track, seems
>>>>>>>>>> something
>>>>>>>>>> times out during of the audio playback. For now I don't have any
>>>>>>>>>> more info.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>>>>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>>>>>>
>>>>>>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Given that the audio drivers uses regmap, it could be good to
>>>>>>>> dump the
>>>>>>>> I2S/AHUB registers while the clip if playing with and without this
>>>>>>>> patch
>>>>>>>> to see the differences. I am curious if the audio is now being
>>>>>>>> played as
>>>>>>>> 24 or 32-bit instead of 16-bit now these are available.
>>>>>>>>
>>>>>>>> You could also dump the hw_params to see the format while playing as
>>>>>>>> well ...
>>>>>>>>
>>>>>>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>>>>>>
>>>>>>> I suppose it is also possible that the codec isn't properly doing >16
>>>>>>> bits and the fact we now offer 24 and 32 could be an issue. I've not
>>>>>>> got anything with an audio output on it that would be easy to test.
>>>>>>
>>>>>> I thought I had tested on a Jetson TK1 (Tegra124) but it was sometime
>>>>>> back. However, admittedly I may have only done simple 16-bit testing
>>>>>> with speaker-test.
>>>>>>
>>>>>> We do verify that all soundcards are detected and registered as
>>>>>> expected
>>>>>> during daily testing, but at the moment we don't have anything that
>>>>>> verifies actual playback.
>>>>>
>>>>> Please take a look at the attached logs.
>>>>
>>>> I wonder if we are falling into FIFO configuration issues with the
>>>> non-16 bit case.
>>>>
>>>> Can you try adding the following two patches?
>>>
>>> It is much better now! There is no horrible noise and no stuttering, but
>>> audio still has a "robotic" effect, like freq isn't correct.
>>
>> I wonder if there's an issue with FIFO stoking? I should also possibly
>> add the correctly stop FIFOs patch as well.
> 
> I'll be happy to try more patches.
> 
> Meanwhile sound on v5.5+ is broken and thus the incomplete patches
> should be reverted.

Have you checked if just removing the 24/32 bits from .formats in
the dai template and see if the problem continues? I will try and
see if I can find the code that does the fifo level checking and
see if the problem is in the FIFO fill or something else in the
audio hub setup.
Dmitry Osipenko Dec. 22, 2019, 5:08 p.m. UTC | #17
20.12.2019 20:06, Ben Dooks пишет:
> On 20/12/2019 16:40, Dmitry Osipenko wrote:
>> 20.12.2019 18:25, Ben Dooks пишет:
>>> On 20/12/2019 15:02, Dmitry Osipenko wrote:
>>>> 20.12.2019 17:56, Ben Dooks пишет:
>>>>> On 20/12/2019 14:43, Dmitry Osipenko wrote:
>>>>>> 20.12.2019 16:57, Jon Hunter пишет:
>>>>>>>
>>>>>>> On 20/12/2019 11:38, Ben Dooks wrote:
>>>>>>>> On 20/12/2019 11:30, Jon Hunter wrote:
>>>>>>>>>
>>>>>>>>> On 25/11/2019 17:28, Dmitry Osipenko wrote:
>>>>>>>>>> 25.11.2019 20:22, Dmitry Osipenko пишет:
>>>>>>>>>>> 25.11.2019 13:37, Ben Dooks пишет:
>>>>>>>>>>>> On 23/11/2019 21:09, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 18.10.2019 18:48, Ben Dooks пишет:
>>>>>>>>>>>>>> From: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The tegra3 audio can support 24 and 32 bit sample sizes so
>>>>>>>>>>>>>> add
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> option to the tegra30_i2s_hw_params to configure the
>>>>>>>>>>>>>> S24_LE or
>>>>>>>>>>>>>> S32_LE
>>>>>>>>>>>>>> formats when requested.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
>>>>>>>>>>>>>> [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit]
>>>>>>>>>>>>>> [ben.dooks@codethink.co.uk: add pm calls around ytdm config]
>>>>>>>>>>>>>> [ben.dooks@codethink.co.uk: drop debug printing to dev_dbg]
>>>>>>>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> squash 5aeca5a055fd ASoC: tegra: i2s:
>>>>>>>>>>>>>> pm_runtime_get_sync() is
>>>>>>>>>>>>>> needed
>>>>>>>>>>>>>> in tdm code
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>       sound/soc/tegra/tegra30_i2s.c | 25
>>>>>>>>>>>>>> ++++++++++++++++++++-----
>>>>>>>>>>>>>>       1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>>> index 73f0dddeaef3..063f34c882af 100644
>>>>>>>>>>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>>>>>>>>>>> @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>>>           struct device *dev = dai->dev;
>>>>>>>>>>>>>>           struct tegra30_i2s *i2s =
>>>>>>>>>>>>>> snd_soc_dai_get_drvdata(dai);
>>>>>>>>>>>>>>           unsigned int mask, val, reg;
>>>>>>>>>>>>>> -    int ret, sample_size, srate, i2sclock, bitcnt;
>>>>>>>>>>>>>> +    int ret, sample_size, srate, i2sclock, bitcnt,
>>>>>>>>>>>>>> audio_bits;
>>>>>>>>>>>>>>           struct tegra30_ahub_cif_conf cif_conf;
>>>>>>>>>>>>>>             if (params_channels(params) != 2)
>>>>>>>>>>>>>> @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>>>           switch (params_format(params)) {
>>>>>>>>>>>>>>           case SNDRV_PCM_FORMAT_S16_LE:
>>>>>>>>>>>>>>               val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>>>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>>>               sample_size = 16;
>>>>>>>>>>>>>>               break;
>>>>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S24_LE:
>>>>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>>>>>>>>>> +        sample_size = 24;
>>>>>>>>>>>>>> +        break;
>>>>>>>>>>>>>> +    case SNDRV_PCM_FORMAT_S32_LE:
>>>>>>>>>>>>>> +        val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>>>>>>>>>>>>>> +        audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>>>>>>>>>>>>>> +        sample_size = 32;
>>>>>>>>>>>>>> +        break;
>>>>>>>>>>>>>>           default:
>>>>>>>>>>>>>>               return -EINVAL;
>>>>>>>>>>>>>>           }
>>>>>>>>>>>>>> @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct
>>>>>>>>>>>>>> snd_pcm_substream *substream,
>>>>>>>>>>>>>>           cif_conf.threshold = 0;
>>>>>>>>>>>>>>           cif_conf.audio_channels = 2;
>>>>>>>>>>>>>>           cif_conf.client_channels = 2;
>>>>>>>>>>>>>> -    cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>>> -    cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>>>>>>>>>> +    cif_conf.audio_bits = audio_bits;
>>>>>>>>>>>>>> +    cif_conf.client_bits = audio_bits;
>>>>>>>>>>>>>>           cif_conf.expand = 0;
>>>>>>>>>>>>>>           cif_conf.stereo_conv = 0;
>>>>>>>>>>>>>>           cif_conf.replicate = 0;
>>>>>>>>>>>>>> @@ -306,14 +317,18 @@ static const struct snd_soc_dai_driver
>>>>>>>>>>>>>> tegra30_i2s_dai_template = {
>>>>>>>>>>>>>>               .channels_min = 2,
>>>>>>>>>>>>>>               .channels_max = 2,
>>>>>>>>>>>>>>               .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>>>           },
>>>>>>>>>>>>>>           .capture = {
>>>>>>>>>>>>>>               .stream_name = "Capture",
>>>>>>>>>>>>>>               .channels_min = 2,
>>>>>>>>>>>>>>               .channels_max = 2,
>>>>>>>>>>>>>>               .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>>>>>>> -        .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>>> +        .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>>>>>>>> +               SNDRV_PCM_FMTBIT_S16_LE,
>>>>>>>>>>>>>>           },
>>>>>>>>>>>>>>           .ops = &tegra30_i2s_dai_ops,
>>>>>>>>>>>>>>           .symmetric_rates = 1,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>
>>>>>>>>>>>>> This patch breaks audio on Tegra30. I don't see errors
>>>>>>>>>>>>> anywhere, but
>>>>>>>>>>>>> there is no audio and reverting this patch helps. Please
>>>>>>>>>>>>> fix it.
>>>>>>>>>>>>
>>>>>>>>>>>> What is the failure mode? I can try and take a look at this
>>>>>>>>>>>> some
>>>>>>>>>>>> time
>>>>>>>>>>>> this week, but I am not sure if I have any boards with an
>>>>>>>>>>>> actual
>>>>>>>>>>>> useful
>>>>>>>>>>>> audio output?
>>>>>>>>>>>
>>>>>>>>>>> The failure mode is that there no sound. I also noticed that
>>>>>>>>>>> video
>>>>>>>>>>> playback stutters a lot if movie file has audio track, seems
>>>>>>>>>>> something
>>>>>>>>>>> times out during of the audio playback. For now I don't have any
>>>>>>>>>>> more info.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Oh, I didn't say how to reproduce it.. for example simply playing
>>>>>>>>>> big_buck_bunny_720p_h264.mov in MPV has the audio problem.
>>>>>>>>>>
>>>>>>>>>> https://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_720p_h264.mov
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Given that the audio drivers uses regmap, it could be good to
>>>>>>>>> dump the
>>>>>>>>> I2S/AHUB registers while the clip if playing with and without this
>>>>>>>>> patch
>>>>>>>>> to see the differences. I am curious if the audio is now being
>>>>>>>>> played as
>>>>>>>>> 24 or 32-bit instead of 16-bit now these are available.
>>>>>>>>>
>>>>>>>>> You could also dump the hw_params to see the format while
>>>>>>>>> playing as
>>>>>>>>> well ...
>>>>>>>>>
>>>>>>>>> $ /proc/asound/<scard-name>/pcm0p/sub0/hw_params
>>>>>>>>
>>>>>>>> I suppose it is also possible that the codec isn't properly
>>>>>>>> doing >16
>>>>>>>> bits and the fact we now offer 24 and 32 could be an issue. I've
>>>>>>>> not
>>>>>>>> got anything with an audio output on it that would be easy to test.
>>>>>>>
>>>>>>> I thought I had tested on a Jetson TK1 (Tegra124) but it was
>>>>>>> sometime
>>>>>>> back. However, admittedly I may have only done simple 16-bit testing
>>>>>>> with speaker-test.
>>>>>>>
>>>>>>> We do verify that all soundcards are detected and registered as
>>>>>>> expected
>>>>>>> during daily testing, but at the moment we don't have anything that
>>>>>>> verifies actual playback.
>>>>>>
>>>>>> Please take a look at the attached logs.
>>>>>
>>>>> I wonder if we are falling into FIFO configuration issues with the
>>>>> non-16 bit case.
>>>>>
>>>>> Can you try adding the following two patches?
>>>>
>>>> It is much better now! There is no horrible noise and no stuttering,
>>>> but
>>>> audio still has a "robotic" effect, like freq isn't correct.
>>>
>>> I wonder if there's an issue with FIFO stoking? I should also possibly
>>> add the correctly stop FIFOs patch as well.
>>
>> I'll be happy to try more patches.
>>
>> Meanwhile sound on v5.5+ is broken and thus the incomplete patches
>> should be reverted.
> 
> Have you checked if just removing the 24/32 bits from .formats in
> the dai template and see if the problem continues? 

It works.

> I will try and
> see if I can find the code that does the fifo level checking and
> see if the problem is in the FIFO fill or something else in the
> audio hub setup.

Ok
Ben Dooks Jan. 5, 2020, 12:04 a.m. UTC | #18
[snip]

I've just gone through testing.

Some simple data tests show 16 and 32-bits work.

The 24 bit case seems to be weird, it looks like the 24-bit expects
24 bit samples in 32 bit words. I can't see any packing options to
do 24 bit in 24 bit, so we may have to remove 24 bit sample support
(which is a shame)

My preference is to remove the 24-bit support and keep the 32 bit in.
Dmitry Osipenko Jan. 5, 2020, 1:48 a.m. UTC | #19
05.01.2020 03:04, Ben Dooks пишет:
> [snip]
> 
> I've just gone through testing.
> 
> Some simple data tests show 16 and 32-bits work.
> 
> The 24 bit case seems to be weird, it looks like the 24-bit expects
> 24 bit samples in 32 bit words. I can't see any packing options to
> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
> (which is a shame)
> 
> My preference is to remove the 24-bit support and keep the 32 bit in.
> 

Interesting.. Jon, could you please confirm that 24bit format isn't
usable on T30?
Ben Dooks Jan. 5, 2020, 10:53 a.m. UTC | #20
On 2020-01-05 01:48, Dmitry Osipenko wrote:
> 05.01.2020 03:04, Ben Dooks пишет:
>> [snip]
>> 
>> I've just gone through testing.
>> 
>> Some simple data tests show 16 and 32-bits work.
>> 
>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>> 24 bit samples in 32 bit words. I can't see any packing options to
>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>> (which is a shame)
>> 
>> My preference is to remove the 24-bit support and keep the 32 bit in.
>> 
> 
> Interesting.. Jon, could you please confirm that 24bit format isn't
> usable on T30?

If there is an option of 24 packed into 32, then I think that would 
work.

I can try testing that with raw data on Monday.
Ben Dooks Jan. 6, 2020, 7 p.m. UTC | #21
On 05/01/2020 10:53, Ben Dooks wrote:
> 
> 
> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>> 05.01.2020 03:04, Ben Dooks пишет:
>>> [snip]
>>>
>>> I've just gone through testing.
>>>
>>> Some simple data tests show 16 and 32-bits work.
>>>
>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>> (which is a shame)
>>>
>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>
>>
>> Interesting.. Jon, could you please confirm that 24bit format isn't
>> usable on T30?
> 
> If there is an option of 24 packed into 32, then I think that would work.
> 
> I can try testing that with raw data on Monday.

I need to check some things, I assumed 24 was 24 packed bits, it looks
like the default is 24 in 32 bits so we may be ok. However I need to
re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).

I'll follow up later,
Dmitry Osipenko Jan. 7, 2020, 1:39 a.m. UTC | #22
06.01.2020 22:00, Ben Dooks пишет:
> On 05/01/2020 10:53, Ben Dooks wrote:
>>
>>
>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>> [snip]
>>>>
>>>> I've just gone through testing.
>>>>
>>>> Some simple data tests show 16 and 32-bits work.
>>>>
>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>> (which is a shame)
>>>>
>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>
>>>
>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>> usable on T30?
>>
>> If there is an option of 24 packed into 32, then I think that would work.
>>
>> I can try testing that with raw data on Monday.
> 
> I need to check some things, I assumed 24 was 24 packed bits, it looks
> like the default is 24 in 32 bits so we may be ok. However I need to
> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
> 
> I'll follow up later,

Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
looked through the TRM doc and got impression that AHUB could re-pack
data stream into something that codec supports, but maybe it's a wrong
impression.
Jon Hunter Jan. 7, 2020, 10:29 a.m. UTC | #23
On 05/01/2020 10:53, Ben Dooks wrote:
> 
> 
> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>> 05.01.2020 03:04, Ben Dooks пишет:
>>> [snip]
>>>
>>> I've just gone through testing.
>>>
>>> Some simple data tests show 16 and 32-bits work.
>>>
>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>> (which is a shame)
>>>
>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>
>>
>> Interesting.. Jon, could you please confirm that 24bit format isn't
>> usable on T30?
> 
> If there is an option of 24 packed into 32, then I think that would work.
> 
> I can try testing that with raw data on Monday.

I will check on this. I would have thought that S24_LE (24-bits packed
into 32-bit elements) would be fine. Typically we don't support S24_3LE
(24-bits in 24-bit elements).

Jon
Ben Dooks Jan. 7, 2020, 10:35 a.m. UTC | #24
On 07/01/2020 10:29, Jon Hunter wrote:
> 
> On 05/01/2020 10:53, Ben Dooks wrote:
>>
>>
>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>> [snip]
>>>>
>>>> I've just gone through testing.
>>>>
>>>> Some simple data tests show 16 and 32-bits work.
>>>>
>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>> (which is a shame)
>>>>
>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>
>>>
>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>> usable on T30?
>>
>> If there is an option of 24 packed into 32, then I think that would work.
>>
>> I can try testing that with raw data on Monday.
> 
> I will check on this. I would have thought that S24_LE (24-bits packed
> into 32-bit elements) would be fine. Typically we don't support S24_3LE
> (24-bits in 24-bit elements).

I will have a look again, I thought S24 was 24-in-24, so wrote my test
generator code to do that. I'll go and see if I can re-test this as soon
as possible (may be Wed/Thu by the time I can get to check it)
Jon Hunter Jan. 8, 2020, 11:37 a.m. UTC | #25
On 07/01/2020 01:39, Dmitry Osipenko wrote:
> 06.01.2020 22:00, Ben Dooks пишет:
>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>
>>>
>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>> [snip]
>>>>>
>>>>> I've just gone through testing.
>>>>>
>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>
>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>> (which is a shame)
>>>>>
>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>
>>>>
>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>> usable on T30?
>>>
>>> If there is an option of 24 packed into 32, then I think that would work.
>>>
>>> I can try testing that with raw data on Monday.
>>
>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>> like the default is 24 in 32 bits so we may be ok. However I need to
>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>
>> I'll follow up later,
> 
> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
> looked through the TRM doc and got impression that AHUB could re-pack
> data stream into something that codec supports, but maybe it's a wrong
> impression.

I chatted with Sameer about this, so yes the AHUB can repack, but there
is a problem with S24_LE where if we try to extract 24-bits we actually
get the upper 24-bits and not the lower LSBs in the 32-bit data element.
So actually we don't support S24_LE.

Ben do you need 24-bit support or 32-bit or both?

Jon
Dmitry Osipenko Jan. 20, 2020, 4:50 p.m. UTC | #26
08.01.2020 14:37, Jon Hunter пишет:
> 
> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>> 06.01.2020 22:00, Ben Dooks пишет:
>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>
>>>>
>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>> [snip]
>>>>>>
>>>>>> I've just gone through testing.
>>>>>>
>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>
>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>> (which is a shame)
>>>>>>
>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>
>>>>>
>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>> usable on T30?
>>>>
>>>> If there is an option of 24 packed into 32, then I think that would work.
>>>>
>>>> I can try testing that with raw data on Monday.
>>>
>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>
>>> I'll follow up later,
>>
>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>> looked through the TRM doc and got impression that AHUB could re-pack
>> data stream into something that codec supports, but maybe it's a wrong
>> impression.
> 
> I chatted with Sameer about this, so yes the AHUB can repack, but there
> is a problem with S24_LE where if we try to extract 24-bits we actually
> get the upper 24-bits and not the lower LSBs in the 32-bit data element.
> So actually we don't support S24_LE.
> 
> Ben do you need 24-bit support or 32-bit or both?

Any updates? Should we revert all the applied patches for now?
Ben Dooks Jan. 20, 2020, 5:36 p.m. UTC | #27
On 20/01/2020 16:50, Dmitry Osipenko wrote:
> 08.01.2020 14:37, Jon Hunter пишет:
>>
>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>
>>>>>
>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>> [snip]
>>>>>>>
>>>>>>> I've just gone through testing.
>>>>>>>
>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>
>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>> (which is a shame)
>>>>>>>
>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>
>>>>>>
>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>> usable on T30?
>>>>>
>>>>> If there is an option of 24 packed into 32, then I think that would work.
>>>>>
>>>>> I can try testing that with raw data on Monday.
>>>>
>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>
>>>> I'll follow up later,
>>>
>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>> looked through the TRM doc and got impression that AHUB could re-pack
>>> data stream into something that codec supports, but maybe it's a wrong
>>> impression.
>>
>> I chatted with Sameer about this, so yes the AHUB can repack, but there
>> is a problem with S24_LE where if we try to extract 24-bits we actually
>> get the upper 24-bits and not the lower LSBs in the 32-bit data element.
>> So actually we don't support S24_LE.
>>
>> Ben do you need 24-bit support or 32-bit or both?

I think the S24 should work unpacked. The packed just doesn't seem to
be an option on tegra2/tegra3 hardware (the manual does not talk about
it either).

I will try and get this looked at again on Thursday 23rd and see if
I can run some more tests with 24 sample data in the input format and
a logic analyser on the output.
Ben Dooks Jan. 21, 2020, 6:15 p.m. UTC | #28
On 07/01/2020 10:29, Jon Hunter wrote:
> 
> On 05/01/2020 10:53, Ben Dooks wrote:
>>
>>
>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>> [snip]
>>>>
>>>> I've just gone through testing.
>>>>
>>>> Some simple data tests show 16 and 32-bits work.
>>>>
>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>> (which is a shame)
>>>>
>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>
>>>
>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>> usable on T30?
>>
>> If there is an option of 24 packed into 32, then I think that would work.
>>
>> I can try testing that with raw data on Monday.
> 
> I will check on this. I would have thought that S24_LE (24-bits packed
> into 32-bit elements) would be fine. Typically we don't support S24_3LE
> (24-bits in 24-bit elements).
> 

I've just had to spend time fixing pulseview/sigrok's i2s handling for
this, but have run a simple test of S24_LE using a pattern generator
and the low level output looks ok.

I will test a bit more tomorrow, but I suspect something else is either
getting S24_LE wrong or we have some other issue.
Dmitry Osipenko Jan. 21, 2020, 6:54 p.m. UTC | #29
21.01.2020 21:15, Ben Dooks пишет:
> On 07/01/2020 10:29, Jon Hunter wrote:
>>
>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>
>>>
>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>> [snip]
>>>>>
>>>>> I've just gone through testing.
>>>>>
>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>
>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>> (which is a shame)
>>>>>
>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>
>>>>
>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>> usable on T30?
>>>
>>> If there is an option of 24 packed into 32, then I think that would
>>> work.
>>>
>>> I can try testing that with raw data on Monday.
>>
>> I will check on this. I would have thought that S24_LE (24-bits packed
>> into 32-bit elements) would be fine. Typically we don't support S24_3LE
>> (24-bits in 24-bit elements).
>>
> 
> I've just had to spend time fixing pulseview/sigrok's i2s handling for
> this, but have run a simple test of S24_LE using a pattern generator
> and the low level output looks ok.
> 
> I will test a bit more tomorrow, but I suspect something else is either
> getting S24_LE wrong or we have some other issue.

Okay, thanks for the update.
Ben Dooks Jan. 23, 2020, 7:38 p.m. UTC | #30
On 07/01/2020 01:39, Dmitry Osipenko wrote:
> 06.01.2020 22:00, Ben Dooks пишет:
>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>
>>>
>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>> [snip]
>>>>>
>>>>> I've just gone through testing.
>>>>>
>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>
>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>> (which is a shame)
>>>>>
>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>
>>>>
>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>> usable on T30?
>>>
>>> If there is an option of 24 packed into 32, then I think that would work.
>>>
>>> I can try testing that with raw data on Monday.
>>
>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>> like the default is 24 in 32 bits so we may be ok. However I need to
>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>
>> I'll follow up later,
> 
> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
> looked through the TRM doc and got impression that AHUB could re-pack
> data stream into something that codec supports, but maybe it's a wrong
> impression.
> _________________________________

I did a quick test with the following:

  sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
  sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
  sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5

The 16 and 32 work fine, the 24 is showing a playback output freq
of 440Hz instead of 500Hz... this suggests the clock is off, or there
is something else weird going on...
Ben Dooks Jan. 23, 2020, 9:59 p.m. UTC | #31
On 23/01/2020 19:38, Ben Dooks wrote:
> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>> 06.01.2020 22:00, Ben Dooks пишет:
>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>
>>>>
>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>> [snip]
>>>>>>
>>>>>> I've just gone through testing.
>>>>>>
>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>
>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>> (which is a shame)
>>>>>>
>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>
>>>>>
>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>> usable on T30?
>>>>
>>>> If there is an option of 24 packed into 32, then I think that would 
>>>> work.
>>>>
>>>> I can try testing that with raw data on Monday.
>>>
>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>
>>> I'll follow up later,
>>
>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>> looked through the TRM doc and got impression that AHUB could re-pack
>> data stream into something that codec supports, but maybe it's a wrong
>> impression.
>> _________________________________
> 
> I did a quick test with the following:
> 
>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
> 
> The 16 and 32 work fine, the 24 is showing a playback output freq
> of 440Hz instead of 500Hz... this suggests the clock is off, or there
> is something else weird going on...
> 

I should have checked pll_a_out0 rate, for 24bit 2ch, I get
pll_a_out at which makes:

11289600/(24*2*44100) = 5.3333333333

For some reason the PLL can't get a decent divisor for this.
Dmitry Osipenko Jan. 23, 2020, 10:11 p.m. UTC | #32
24.01.2020 00:59, Ben Dooks пишет:
> On 23/01/2020 19:38, Ben Dooks wrote:
>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>
>>>>>
>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>> [snip]
>>>>>>>
>>>>>>> I've just gone through testing.
>>>>>>>
>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>
>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>> (which is a shame)
>>>>>>>
>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit
>>>>>>> in.
>>>>>>>
>>>>>>
>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>> usable on T30?
>>>>>
>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>> work.
>>>>>
>>>>> I can try testing that with raw data on Monday.
>>>>
>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>
>>>> I'll follow up later,
>>>
>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>> looked through the TRM doc and got impression that AHUB could re-pack
>>> data stream into something that codec supports, but maybe it's a wrong
>>> impression.
>>> _________________________________
>>
>> I did a quick test with the following:
>>
>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>
>> The 16 and 32 work fine, the 24 is showing a playback output freq
>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>> is something else weird going on...
>>
> 
> I should have checked pll_a_out0 rate, for 24bit 2ch, I get
> pll_a_out at which makes:
> 
> 11289600/(24*2*44100) = 5.3333333333
> 
> For some reason the PLL can't get a decent divisor for this.

Have you tried to adjust the predefined PLLA rate? Please see
tegra_clk_init_table in drivers/clk/tegra/clk-tegra30.c. Will be
interesting if it works with that.

Sowjanya said that the PLLA rate setup is going to be moved to the audio
driver [1], maybe that's what we already need for 24bit.

[1] https://lkml.org/lkml/2020/1/21/744
Dmitry Osipenko Jan. 24, 2020, 4:31 a.m. UTC | #33
24.01.2020 01:11, Dmitry Osipenko пишет:
> 24.01.2020 00:59, Ben Dooks пишет:
>> On 23/01/2020 19:38, Ben Dooks wrote:
>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>>
>>>>>>
>>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>> I've just gone through testing.
>>>>>>>>
>>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>>
>>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>>> (which is a shame)
>>>>>>>>
>>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit
>>>>>>>> in.
>>>>>>>>
>>>>>>>
>>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>>> usable on T30?
>>>>>>
>>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>>> work.
>>>>>>
>>>>>> I can try testing that with raw data on Monday.
>>>>>
>>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>>
>>>>> I'll follow up later,
>>>>
>>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>>> looked through the TRM doc and got impression that AHUB could re-pack
>>>> data stream into something that codec supports, but maybe it's a wrong
>>>> impression.
>>>> _________________________________
>>>
>>> I did a quick test with the following:
>>>
>>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>
>>> The 16 and 32 work fine, the 24 is showing a playback output freq
>>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>>> is something else weird going on...
>>>
>>
>> I should have checked pll_a_out0 rate, for 24bit 2ch, I get
>> pll_a_out at which makes:
>>
>> 11289600/(24*2*44100) = 5.3333333333
>>
>> For some reason the PLL can't get a decent divisor for this.
> 
> Have you tried to adjust the predefined PLLA rate? Please see
> tegra_clk_init_table in drivers/clk/tegra/clk-tegra30.c. Will be
> interesting if it works with that.
> 
> Sowjanya said that the PLLA rate setup is going to be moved to the audio
> driver [1], maybe that's what we already need for 24bit.
> 
> [1] https://lkml.org/lkml/2020/1/21/744

Actually, tegra_asoc_utils_set_rate() sets the PLLA rate, but the values
are hardcoded there.
Jon Hunter Jan. 24, 2020, 4:50 p.m. UTC | #34
On 23/01/2020 19:38, Ben Dooks wrote:
> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>> 06.01.2020 22:00, Ben Dooks пишет:
>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>
>>>>
>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>> [snip]
>>>>>>
>>>>>> I've just gone through testing.
>>>>>>
>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>
>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>> (which is a shame)
>>>>>>
>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>
>>>>>
>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>> usable on T30?
>>>>
>>>> If there is an option of 24 packed into 32, then I think that would
>>>> work.
>>>>
>>>> I can try testing that with raw data on Monday.
>>>
>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>
>>> I'll follow up later,
>>
>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>> looked through the TRM doc and got impression that AHUB could re-pack
>> data stream into something that codec supports, but maybe it's a wrong
>> impression.
>> _________________________________
> 
> I did a quick test with the following:
> 
>  sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>  sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>  sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
> 
> The 16 and 32 work fine, the 24 is showing a playback output freq
> of 440Hz instead of 500Hz... this suggests the clock is off, or there
> is something else weird going on...

I was looking at using sox to create such as file, but the above command
generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
supports S24_LE but does not support S24_3LE and so I cannot test this.
Anyway, we really need to test S24_LE and not S24_3LE because this is
the problem that Dmitry is having.

Ben is S24_3LE what you really need to support?

Dmitry, does the following fix your problem?

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index dbed3c5408e7..92845c4b63f4 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
snd_pcm_substream *substream,
                audio_bits = TEGRA30_AUDIOCIF_BITS_16;
                sample_size = 16;
                break;
-       case SNDRV_PCM_FORMAT_S24_LE:
+       case SNDRV_PCM_FORMAT_S24_3LE:
                val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
                audio_bits = TEGRA30_AUDIOCIF_BITS_24;
                sample_size = 24;
@@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
tegra30_i2s_dai_template = {
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_8000_96000,
                .formats = SNDRV_PCM_FMTBIT_S32_LE |
-                          SNDRV_PCM_FMTBIT_S24_LE |
+                          SNDRV_PCM_FMTBIT_S24_3LE |
                           SNDRV_PCM_FMTBIT_S16_LE,
        },
        .capture = {
@@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
tegra30_i2s_dai_template = {
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_8000_96000,
                .formats = SNDRV_PCM_FMTBIT_S32_LE |
-                          SNDRV_PCM_FMTBIT_S24_LE |
+                          SNDRV_PCM_FMTBIT_S24_3LE |
                           SNDRV_PCM_FMTBIT_S16_LE,
        },
        .ops = &tegra30_i2s_dai_ops,

Jon
Jon Hunter Jan. 24, 2020, 4:56 p.m. UTC | #35
On 23/01/2020 21:59, Ben Dooks wrote:
> On 23/01/2020 19:38, Ben Dooks wrote:
>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>
>>>>>
>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>> [snip]
>>>>>>>
>>>>>>> I've just gone through testing.
>>>>>>>
>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>
>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>> (which is a shame)
>>>>>>>
>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit
>>>>>>> in.
>>>>>>>
>>>>>>
>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>> usable on T30?
>>>>>
>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>> work.
>>>>>
>>>>> I can try testing that with raw data on Monday.
>>>>
>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>
>>>> I'll follow up later,
>>>
>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>> looked through the TRM doc and got impression that AHUB could re-pack
>>> data stream into something that codec supports, but maybe it's a wrong
>>> impression.
>>> _________________________________
>>
>> I did a quick test with the following:
>>
>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>
>> The 16 and 32 work fine, the 24 is showing a playback output freq
>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>> is something else weird going on...
>>
> 
> I should have checked pll_a_out0 rate, for 24bit 2ch, I get
> pll_a_out at which makes:
> 
> 11289600/(24*2*44100) = 5.3333333333
> 
> For some reason the PLL can't get a decent divisor for this.

Yes that is going to be a problem. I assume that your codec wants a
256*fs MCLK? Maybe in that case you are better off having the codec
drive the bit clock and fsync?

Is 24-bit critical to what you are doing?

Otherwise maybe we should drop the 24-bit support for now and just keep
32-bit.

Jon
Ben Dooks Jan. 24, 2020, 5 p.m. UTC | #36
On 24/01/2020 16:50, Jon Hunter wrote:
> 
> On 23/01/2020 19:38, Ben Dooks wrote:
>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>
>>>>>
>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>> [snip]
>>>>>>>
>>>>>>> I've just gone through testing.
>>>>>>>
>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>
>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>> (which is a shame)
>>>>>>>
>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>
>>>>>>
>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>> usable on T30?
>>>>>
>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>> work.
>>>>>
>>>>> I can try testing that with raw data on Monday.
>>>>
>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>
>>>> I'll follow up later,
>>>
>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>> looked through the TRM doc and got impression that AHUB could re-pack
>>> data stream into something that codec supports, but maybe it's a wrong
>>> impression.
>>> _________________________________
>>
>> I did a quick test with the following:
>>
>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>
>> The 16 and 32 work fine, the 24 is showing a playback output freq
>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>> is something else weird going on...
> 
> I was looking at using sox to create such as file, but the above command
> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
> supports S24_LE but does not support S24_3LE and so I cannot test this.
> Anyway, we really need to test S24_LE and not S24_3LE because this is
> the problem that Dmitry is having.
> 
> Ben is S24_3LE what you really need to support?

No, it is S24_LE the format this hardware supports. I wonder if
aplay is transforming it.

Plug PCM: Linear conversion PCM (S24_LE)
Its setup is:
   stream       : PLAYBACK
   access       : RW_INTERLEAVED
   format       : S24_3LE
   subformat    : STD
   channels     : 2

So I assume aplay has turned the S24_3LE -> S24_LE
Mark Brown Jan. 24, 2020, 5 p.m. UTC | #37
On Fri, Jan 24, 2020 at 04:56:41PM +0000, Jon Hunter wrote:

> Yes that is going to be a problem. I assume that your codec wants a
> 256*fs MCLK? Maybe in that case you are better off having the codec
> drive the bit clock and fsync?

> Is 24-bit critical to what you are doing?

> Otherwise maybe we should drop the 24-bit support for now and just keep
> 32-bit.

Removing the support because one particular board has limited clocks
isn't good - it'd be better to have components with clocking
restrictions impose constraints as needed.
Ben Dooks Jan. 24, 2020, 5:03 p.m. UTC | #38
On 24/01/2020 17:00, Mark Brown wrote:
> On Fri, Jan 24, 2020 at 04:56:41PM +0000, Jon Hunter wrote:
> 
>> Yes that is going to be a problem. I assume that your codec wants a
>> 256*fs MCLK? Maybe in that case you are better off having the codec
>> drive the bit clock and fsync?

Would be lovely, but tegra i2s clock slave is still on the list
of things I have to get into the kernel (it doesn't work and no-one
in the kernel currently uses it...)

>> Is 24-bit critical to what you are doing?
> 
>> Otherwise maybe we should drop the 24-bit support for now and just keep
>> 32-bit.
> 
> Removing the support because one particular board has limited clocks
> isn't good - it'd be better to have components with clocking
> restrictions impose constraints as needed.
>
Ben Dooks Jan. 24, 2020, 5:06 p.m. UTC | #39
On 24/01/2020 16:50, Jon Hunter wrote:
> 
> On 23/01/2020 19:38, Ben Dooks wrote:
>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>
>>>>>
>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>> [snip]
>>>>>>>
>>>>>>> I've just gone through testing.
>>>>>>>
>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>
>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>> (which is a shame)
>>>>>>>
>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>
>>>>>>
>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>> usable on T30?
>>>>>
>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>> work.
>>>>>
>>>>> I can try testing that with raw data on Monday.
>>>>
>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>
>>>> I'll follow up later,
>>>
>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>> looked through the TRM doc and got impression that AHUB could re-pack
>>> data stream into something that codec supports, but maybe it's a wrong
>>> impression.
>>> _________________________________
>>
>> I did a quick test with the following:
>>
>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>
>> The 16 and 32 work fine, the 24 is showing a playback output freq
>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>> is something else weird going on...
> 
> I was looking at using sox to create such as file, but the above command
> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
> supports S24_LE but does not support S24_3LE and so I cannot test this.
> Anyway, we really need to test S24_LE and not S24_3LE because this is
> the problem that Dmitry is having.
> 
> Ben is S24_3LE what you really need to support?

I was fairly sure it was saying S24 format, the aplay just prints
S24_LE (but seems to hide the implementtjon)
Dmitry Osipenko Jan. 27, 2020, 7:20 p.m. UTC | #40
24.01.2020 19:50, Jon Hunter пишет:
> 
> On 23/01/2020 19:38, Ben Dooks wrote:
>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>
>>>>>
>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>> [snip]
>>>>>>>
>>>>>>> I've just gone through testing.
>>>>>>>
>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>
>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>> (which is a shame)
>>>>>>>
>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>
>>>>>>
>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>> usable on T30?
>>>>>
>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>> work.
>>>>>
>>>>> I can try testing that with raw data on Monday.
>>>>
>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>
>>>> I'll follow up later,
>>>
>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>> looked through the TRM doc and got impression that AHUB could re-pack
>>> data stream into something that codec supports, but maybe it's a wrong
>>> impression.
>>> _________________________________
>>
>> I did a quick test with the following:
>>
>>  sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>  sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>  sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>
>> The 16 and 32 work fine, the 24 is showing a playback output freq
>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>> is something else weird going on...
> 
> I was looking at using sox to create such as file, but the above command
> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
> supports S24_LE but does not support S24_3LE and so I cannot test this.
> Anyway, we really need to test S24_LE and not S24_3LE because this is
> the problem that Dmitry is having.
> 
> Ben is S24_3LE what you really need to support?
> 
> Dmitry, does the following fix your problem?
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index dbed3c5408e7..92845c4b63f4 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
> snd_pcm_substream *substream,
>                 audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>                 sample_size = 16;
>                 break;
> -       case SNDRV_PCM_FORMAT_S24_LE:
> +       case SNDRV_PCM_FORMAT_S24_3LE:
>                 val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>                 audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>                 sample_size = 24;
> @@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
> tegra30_i2s_dai_template = {
>                 .channels_max = 2,
>                 .rates = SNDRV_PCM_RATE_8000_96000,
>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
> -                          SNDRV_PCM_FMTBIT_S24_LE |
> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>                            SNDRV_PCM_FMTBIT_S16_LE,
>         },
>         .capture = {
> @@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
> tegra30_i2s_dai_template = {
>                 .channels_max = 2,
>                 .rates = SNDRV_PCM_RATE_8000_96000,
>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
> -                          SNDRV_PCM_FMTBIT_S24_LE |
> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>                            SNDRV_PCM_FMTBIT_S16_LE,
>         },
>         .ops = &tegra30_i2s_dai_ops,
> 
> Jon
> 

It should solve the problem in my particular case, but I'm not sure that
the solution is correct.

The v5.5 kernel is released now with the broken audio and apparently
getting 24bit to work won't be trivial (if possible at all). Ben, could
you please send a patch to fix v5.5 by removing the S24 support
advertisement from the driver?
Dmitry Osipenko Jan. 27, 2020, 7:23 p.m. UTC | #41
27.01.2020 22:20, Dmitry Osipenko пишет:
> 24.01.2020 19:50, Jon Hunter пишет:
>>
>> On 23/01/2020 19:38, Ben Dooks wrote:
>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>>
>>>>>>
>>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>> I've just gone through testing.
>>>>>>>>
>>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>>
>>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>>> (which is a shame)
>>>>>>>>
>>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>>
>>>>>>>
>>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>>> usable on T30?
>>>>>>
>>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>>> work.
>>>>>>
>>>>>> I can try testing that with raw data on Monday.
>>>>>
>>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>>
>>>>> I'll follow up later,
>>>>
>>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>>> looked through the TRM doc and got impression that AHUB could re-pack
>>>> data stream into something that codec supports, but maybe it's a wrong
>>>> impression.
>>>> _________________________________
>>>
>>> I did a quick test with the following:
>>>
>>>  sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>  sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>  sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>
>>> The 16 and 32 work fine, the 24 is showing a playback output freq
>>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>>> is something else weird going on...
>>
>> I was looking at using sox to create such as file, but the above command
>> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
>> supports S24_LE but does not support S24_3LE and so I cannot test this.
>> Anyway, we really need to test S24_LE and not S24_3LE because this is
>> the problem that Dmitry is having.
>>
>> Ben is S24_3LE what you really need to support?
>>
>> Dmitry, does the following fix your problem?
>>
>> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
>> index dbed3c5408e7..92845c4b63f4 100644
>> --- a/sound/soc/tegra/tegra30_i2s.c
>> +++ b/sound/soc/tegra/tegra30_i2s.c
>> @@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
>> snd_pcm_substream *substream,
>>                 audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>                 sample_size = 16;
>>                 break;
>> -       case SNDRV_PCM_FORMAT_S24_LE:
>> +       case SNDRV_PCM_FORMAT_S24_3LE:
>>                 val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>                 audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>                 sample_size = 24;
>> @@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
>> tegra30_i2s_dai_template = {
>>                 .channels_max = 2,
>>                 .rates = SNDRV_PCM_RATE_8000_96000,
>>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>                            SNDRV_PCM_FMTBIT_S16_LE,
>>         },
>>         .capture = {
>> @@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
>> tegra30_i2s_dai_template = {
>>                 .channels_max = 2,
>>                 .rates = SNDRV_PCM_RATE_8000_96000,
>>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>                            SNDRV_PCM_FMTBIT_S16_LE,
>>         },
>>         .ops = &tegra30_i2s_dai_ops,
>>
>> Jon
>>
> 
> It should solve the problem in my particular case, but I'm not sure that
> the solution is correct.
> 
> The v5.5 kernel is released now with the broken audio and apparently
> getting 24bit to work won't be trivial (if possible at all). Ben, could
> you please send a patch to fix v5.5 by removing the S24 support
> advertisement from the driver?

I also suspect that s32 may need some extra patches and thus could be
worthwhile to stop advertising it as well.
Ricard Wanderlof Jan. 28, 2020, 7:49 a.m. UTC | #42
On Fri, 24 Jan 2020, Ben Dooks wrote:

> So I assume aplay has turned the S24_3LE -> S24_LE

A couple of years ago (so may be inaccurate now) I concluded after some 
testing that S24_LE was broken in arecord and aplay, at least when it came 
to .wav files. Meaning that files I recorded with arecord could be played 
back by aplay, but not in any other application I tried, such as Audacity.

I can try to dig out the gory details, but ISTR it came down to the 
header(s) in the .wav file being incorrectly filled in with regard to 
number of bits per sample vs valid bits per sample.

/Ricard
Ben Dooks Jan. 28, 2020, 8:58 a.m. UTC | #43
On 27/01/2020 19:20, Dmitry Osipenko wrote:
> 24.01.2020 19:50, Jon Hunter пишет:
>>
>> On 23/01/2020 19:38, Ben Dooks wrote:
>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>>
>>>>>>
>>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>> I've just gone through testing.
>>>>>>>>
>>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>>
>>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>>> (which is a shame)
>>>>>>>>
>>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>>
>>>>>>>
>>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>>> usable on T30?
>>>>>>
>>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>>> work.
>>>>>>
>>>>>> I can try testing that with raw data on Monday.
>>>>>
>>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>>
>>>>> I'll follow up later,
>>>>
>>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>>> looked through the TRM doc and got impression that AHUB could re-pack
>>>> data stream into something that codec supports, but maybe it's a wrong
>>>> impression.
>>>> _________________________________
>>>
>>> I did a quick test with the following:
>>>
>>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>
>>> The 16 and 32 work fine, the 24 is showing a playback output freq
>>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>>> is something else weird going on...
>>
>> I was looking at using sox to create such as file, but the above command
>> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
>> supports S24_LE but does not support S24_3LE and so I cannot test this.
>> Anyway, we really need to test S24_LE and not S24_3LE because this is
>> the problem that Dmitry is having.
>>
>> Ben is S24_3LE what you really need to support?
>>
>> Dmitry, does the following fix your problem?
>>
>> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
>> index dbed3c5408e7..92845c4b63f4 100644
>> --- a/sound/soc/tegra/tegra30_i2s.c
>> +++ b/sound/soc/tegra/tegra30_i2s.c
>> @@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
>> snd_pcm_substream *substream,
>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>                  sample_size = 16;
>>                  break;
>> -       case SNDRV_PCM_FORMAT_S24_LE:
>> +       case SNDRV_PCM_FORMAT_S24_3LE:
>>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>                  sample_size = 24;
>> @@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
>> tegra30_i2s_dai_template = {
>>                  .channels_max = 2,
>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>          },
>>          .capture = {
>> @@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
>> tegra30_i2s_dai_template = {
>>                  .channels_max = 2,
>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>          },
>>          .ops = &tegra30_i2s_dai_ops,
>>
>> Jon
>>
> 
> It should solve the problem in my particular case, but I'm not sure that
> the solution is correct.
> 
> The v5.5 kernel is released now with the broken audio and apparently
> getting 24bit to work won't be trivial (if possible at all). Ben, could
> you please send a patch to fix v5.5 by removing the S24 support
> advertisement from the driver?

That might be the best for the moment.

As far as my testing so far is that just the audio rate is off
.
It might be worth putting it in as a config option or run time
command option?
Ben Dooks Jan. 28, 2020, 8:59 a.m. UTC | #44
On 27/01/2020 19:23, Dmitry Osipenko wrote:
> 27.01.2020 22:20, Dmitry Osipenko пишет:
>> 24.01.2020 19:50, Jon Hunter пишет:
>>>
>>> On 23/01/2020 19:38, Ben Dooks wrote:
>>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>>>> [snip]
>>>>>>>>>
>>>>>>>>> I've just gone through testing.
>>>>>>>>>
>>>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>>>
>>>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit expects
>>>>>>>>> 24 bit samples in 32 bit words. I can't see any packing options to
>>>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample support
>>>>>>>>> (which is a shame)
>>>>>>>>>
>>>>>>>>> My preference is to remove the 24-bit support and keep the 32 bit in.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Interesting.. Jon, could you please confirm that 24bit format isn't
>>>>>>>> usable on T30?
>>>>>>>
>>>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>>>> work.
>>>>>>>
>>>>>>> I can try testing that with raw data on Monday.
>>>>>>
>>>>>> I need to check some things, I assumed 24 was 24 packed bits, it looks
>>>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>>>> re-write my test case which assumed it was 24bits in 3 bytes (S24_3LE).
>>>>>>
>>>>>> I'll follow up later,
>>>>>
>>>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I briefly
>>>>> looked through the TRM doc and got impression that AHUB could re-pack
>>>>> data stream into something that codec supports, but maybe it's a wrong
>>>>> impression.
>>>>> _________________________________
>>>>
>>>> I did a quick test with the following:
>>>>
>>>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>
>>>> The 16 and 32 work fine, the 24 is showing a playback output freq
>>>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>>>> is something else weird going on...
>>>
>>> I was looking at using sox to create such as file, but the above command
>>> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
>>> supports S24_LE but does not support S24_3LE and so I cannot test this.
>>> Anyway, we really need to test S24_LE and not S24_3LE because this is
>>> the problem that Dmitry is having.
>>>
>>> Ben is S24_3LE what you really need to support?
>>>
>>> Dmitry, does the following fix your problem?
>>>
>>> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
>>> index dbed3c5408e7..92845c4b63f4 100644
>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>> @@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
>>> snd_pcm_substream *substream,
>>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>                  sample_size = 16;
>>>                  break;
>>> -       case SNDRV_PCM_FORMAT_S24_LE:
>>> +       case SNDRV_PCM_FORMAT_S24_3LE:
>>>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>                  sample_size = 24;
>>> @@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
>>> tegra30_i2s_dai_template = {
>>>                  .channels_max = 2,
>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>>          },
>>>          .capture = {
>>> @@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
>>> tegra30_i2s_dai_template = {
>>>                  .channels_max = 2,
>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>>          },
>>>          .ops = &tegra30_i2s_dai_ops,
>>>
>>> Jon
>>>
>>
>> It should solve the problem in my particular case, but I'm not sure that
>> the solution is correct.
>>
>> The v5.5 kernel is released now with the broken audio and apparently
>> getting 24bit to work won't be trivial (if possible at all). Ben, could
>> you please send a patch to fix v5.5 by removing the S24 support
>> advertisement from the driver?
> 
> I also suspect that s32 may need some extra patches and thus could be
> worthwhile to stop advertising it as well.

As far as I am aware that works and we can hit the audio rates for it.
Mark Brown Jan. 28, 2020, 12:13 p.m. UTC | #45
On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
> 24.01.2020 19:50, Jon Hunter пишет:

> >                 .rates = SNDRV_PCM_RATE_8000_96000,
> >                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
> > -                          SNDRV_PCM_FMTBIT_S24_LE |
> > +                          SNDRV_PCM_FMTBIT_S24_3LE |

> It should solve the problem in my particular case, but I'm not sure that
> the solution is correct.

If the format implemented by the driver is S24_3LE the driver should
advertise S24_3LE.

> The v5.5 kernel is released now with the broken audio and apparently
> getting 24bit to work won't be trivial (if possible at all). Ben, could
> you please send a patch to fix v5.5 by removing the S24 support
> advertisement from the driver?

Why is that the best fix rather than just advertising the format
implemented by the driver?

I really don't understand why this is all taking so long, this thread
just seems to be going round in interminable circles long after it
looked like the issue was understood.  I have to admit I've not read
every single message in the thread but it's difficult to see why it
doesn't seem to be making any progress.
Jon Hunter Jan. 28, 2020, 1:19 p.m. UTC | #46
On 28/01/2020 08:59, Ben Dooks wrote:
> On 27/01/2020 19:23, Dmitry Osipenko wrote:
>> 27.01.2020 22:20, Dmitry Osipenko пишет:
>>> 24.01.2020 19:50, Jon Hunter пишет:
>>>>
>>>> On 23/01/2020 19:38, Ben Dooks wrote:
>>>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>>>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>>>>> [snip]
>>>>>>>>>>
>>>>>>>>>> I've just gone through testing.
>>>>>>>>>>
>>>>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>>>>
>>>>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit
>>>>>>>>>> expects
>>>>>>>>>> 24 bit samples in 32 bit words. I can't see any packing
>>>>>>>>>> options to
>>>>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample
>>>>>>>>>> support
>>>>>>>>>> (which is a shame)
>>>>>>>>>>
>>>>>>>>>> My preference is to remove the 24-bit support and keep the 32
>>>>>>>>>> bit in.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Interesting.. Jon, could you please confirm that 24bit format
>>>>>>>>> isn't
>>>>>>>>> usable on T30?
>>>>>>>>
>>>>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>>>>> work.
>>>>>>>>
>>>>>>>> I can try testing that with raw data on Monday.
>>>>>>>
>>>>>>> I need to check some things, I assumed 24 was 24 packed bits, it
>>>>>>> looks
>>>>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>>>>> re-write my test case which assumed it was 24bits in 3 bytes
>>>>>>> (S24_3LE).
>>>>>>>
>>>>>>> I'll follow up later,
>>>>>>
>>>>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I
>>>>>> briefly
>>>>>> looked through the TRM doc and got impression that AHUB could re-pack
>>>>>> data stream into something that codec supports, but maybe it's a
>>>>>> wrong
>>>>>> impression.
>>>>>> _________________________________
>>>>>
>>>>> I did a quick test with the following:
>>>>>
>>>>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>>
>>>>> The 16 and 32 work fine, the 24 is showing a playback output freq
>>>>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>>>>> is something else weird going on...
>>>>
>>>> I was looking at using sox to create such as file, but the above
>>>> command
>>>> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
>>>> supports S24_LE but does not support S24_3LE and so I cannot test this.
>>>> Anyway, we really need to test S24_LE and not S24_3LE because this is
>>>> the problem that Dmitry is having.
>>>>
>>>> Ben is S24_3LE what you really need to support?
>>>>
>>>> Dmitry, does the following fix your problem?
>>>>
>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>> index dbed3c5408e7..92845c4b63f4 100644
>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>> @@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
>>>> snd_pcm_substream *substream,
>>>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>                  sample_size = 16;
>>>>                  break;
>>>> -       case SNDRV_PCM_FORMAT_S24_LE:
>>>> +       case SNDRV_PCM_FORMAT_S24_3LE:
>>>>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>                  sample_size = 24;
>>>> @@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
>>>> tegra30_i2s_dai_template = {
>>>>                  .channels_max = 2,
>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>>>          },
>>>>          .capture = {
>>>> @@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
>>>> tegra30_i2s_dai_template = {
>>>>                  .channels_max = 2,
>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>>>          },
>>>>          .ops = &tegra30_i2s_dai_ops,
>>>>
>>>> Jon
>>>>
>>>
>>> It should solve the problem in my particular case, but I'm not sure that
>>> the solution is correct.
>>>
>>> The v5.5 kernel is released now with the broken audio and apparently
>>> getting 24bit to work won't be trivial (if possible at all). Ben, could
>>> you please send a patch to fix v5.5 by removing the S24 support
>>> advertisement from the driver?
>>
>> I also suspect that s32 may need some extra patches and thus could be
>> worthwhile to stop advertising it as well.
> 
> As far as I am aware that works and we can hit the audio rates for it.

I ran a test on Tegra124 Jetson-TK1 and 24-bit playback seems to work as
Ben has indicated. So I don't think it is broken.

Can you try Ben's testcase on Tegra30 (ie. generate a tone using sox and
use aplay to play)?

Jon
Dmitry Osipenko Jan. 28, 2020, 3:25 p.m. UTC | #47
28.01.2020 16:19, Jon Hunter пишет:
> 
> On 28/01/2020 08:59, Ben Dooks wrote:
>> On 27/01/2020 19:23, Dmitry Osipenko wrote:
>>> 27.01.2020 22:20, Dmitry Osipenko пишет:
>>>> 24.01.2020 19:50, Jon Hunter пишет:
>>>>>
>>>>> On 23/01/2020 19:38, Ben Dooks wrote:
>>>>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
>>>>>>> 06.01.2020 22:00, Ben Dooks пишет:
>>>>>>>> On 05/01/2020 10:53, Ben Dooks wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020-01-05 01:48, Dmitry Osipenko wrote:
>>>>>>>>>> 05.01.2020 03:04, Ben Dooks пишет:
>>>>>>>>>>> [snip]
>>>>>>>>>>>
>>>>>>>>>>> I've just gone through testing.
>>>>>>>>>>>
>>>>>>>>>>> Some simple data tests show 16 and 32-bits work.
>>>>>>>>>>>
>>>>>>>>>>> The 24 bit case seems to be weird, it looks like the 24-bit
>>>>>>>>>>> expects
>>>>>>>>>>> 24 bit samples in 32 bit words. I can't see any packing
>>>>>>>>>>> options to
>>>>>>>>>>> do 24 bit in 24 bit, so we may have to remove 24 bit sample
>>>>>>>>>>> support
>>>>>>>>>>> (which is a shame)
>>>>>>>>>>>
>>>>>>>>>>> My preference is to remove the 24-bit support and keep the 32
>>>>>>>>>>> bit in.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Interesting.. Jon, could you please confirm that 24bit format
>>>>>>>>>> isn't
>>>>>>>>>> usable on T30?
>>>>>>>>>
>>>>>>>>> If there is an option of 24 packed into 32, then I think that would
>>>>>>>>> work.
>>>>>>>>>
>>>>>>>>> I can try testing that with raw data on Monday.
>>>>>>>>
>>>>>>>> I need to check some things, I assumed 24 was 24 packed bits, it
>>>>>>>> looks
>>>>>>>> like the default is 24 in 32 bits so we may be ok. However I need to
>>>>>>>> re-write my test case which assumed it was 24bits in 3 bytes
>>>>>>>> (S24_3LE).
>>>>>>>>
>>>>>>>> I'll follow up later,
>>>>>>>
>>>>>>> Okay, the S24_3LE isn't supported by RT5640 codec in my case. I
>>>>>>> briefly
>>>>>>> looked through the TRM doc and got impression that AHUB could re-pack
>>>>>>> data stream into something that codec supports, but maybe it's a
>>>>>>> wrong
>>>>>>> impression.
>>>>>>> _________________________________
>>>>>>
>>>>>> I did a quick test with the following:
>>>>>>
>>>>>>   sox -n -b 16 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>>>   sox -n -b 24 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>>>   sox -n -b 32 -c 2 -r 44100 /tmp/tmp.wav  synth sine 500 vol 0.5
>>>>>>
>>>>>> The 16 and 32 work fine, the 24 is showing a playback output freq
>>>>>> of 440Hz instead of 500Hz... this suggests the clock is off, or there
>>>>>> is something else weird going on...
>>>>>
>>>>> I was looking at using sox to create such as file, but the above
>>>>> command
>>>>> generates a S24_3LE file and not S24_LE file. The codec on Jetson-TK1
>>>>> supports S24_LE but does not support S24_3LE and so I cannot test this.
>>>>> Anyway, we really need to test S24_LE and not S24_3LE because this is
>>>>> the problem that Dmitry is having.
>>>>>
>>>>> Ben is S24_3LE what you really need to support?
>>>>>
>>>>> Dmitry, does the following fix your problem?
>>>>>
>>>>> diff --git a/sound/soc/tegra/tegra30_i2s.c
>>>>> b/sound/soc/tegra/tegra30_i2s.c
>>>>> index dbed3c5408e7..92845c4b63f4 100644
>>>>> --- a/sound/soc/tegra/tegra30_i2s.c
>>>>> +++ b/sound/soc/tegra/tegra30_i2s.c
>>>>> @@ -140,7 +140,7 @@ static int tegra30_i2s_hw_params(struct
>>>>> snd_pcm_substream *substream,
>>>>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>>>>>                  sample_size = 16;
>>>>>                  break;
>>>>> -       case SNDRV_PCM_FORMAT_S24_LE:
>>>>> +       case SNDRV_PCM_FORMAT_S24_3LE:
>>>>>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>>>>>                  audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>>>>>                  sample_size = 24;
>>>>> @@ -318,7 +318,7 @@ static const struct snd_soc_dai_driver
>>>>> tegra30_i2s_dai_template = {
>>>>>                  .channels_max = 2,
>>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>>>>          },
>>>>>          .capture = {
>>>>> @@ -327,7 +327,7 @@ static const struct snd_soc_dai_driver
>>>>> tegra30_i2s_dai_template = {
>>>>>                  .channels_max = 2,
>>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>>                             SNDRV_PCM_FMTBIT_S16_LE,
>>>>>          },
>>>>>          .ops = &tegra30_i2s_dai_ops,
>>>>>
>>>>> Jon
>>>>>
>>>>
>>>> It should solve the problem in my particular case, but I'm not sure that
>>>> the solution is correct.
>>>>
>>>> The v5.5 kernel is released now with the broken audio and apparently
>>>> getting 24bit to work won't be trivial (if possible at all). Ben, could
>>>> you please send a patch to fix v5.5 by removing the S24 support
>>>> advertisement from the driver?
>>>
>>> I also suspect that s32 may need some extra patches and thus could be
>>> worthwhile to stop advertising it as well.
>>
>> As far as I am aware that works and we can hit the audio rates for it.
> 
> I ran a test on Tegra124 Jetson-TK1 and 24-bit playback seems to work as
> Ben has indicated. So I don't think it is broken.

Have you tried to replicate my case by playing the video file?

> Can you try Ben's testcase on Tegra30 (ie. generate a tone using sox and
> use aplay to play)?

Surely I can try, but only if you'll share the sample file with me and
give precise instructions how to test it.
Mark Brown Jan. 28, 2020, 3:26 p.m. UTC | #48
On Tue, Jan 28, 2020 at 01:19:17PM +0000, Jon Hunter wrote:
> On 28/01/2020 08:59, Ben Dooks wrote:
> > On 27/01/2020 19:23, Dmitry Osipenko wrote:
> >> 27.01.2020 22:20, Dmitry Osipenko пишет:
> >>> 24.01.2020 19:50, Jon Hunter пишет:
> >>>>
> >>>> On 23/01/2020 19:38, Ben Dooks wrote:
> >>>>> On 07/01/2020 01:39, Dmitry Osipenko wrote:
> >>>>>> 06.01.2020 22:00, Ben Dooks пишет:
> >>>>>>> On 05/01/2020 10:53, Ben Dooks wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> >> I also suspect that s32 may need some extra patches and thus could be
> >> worthwhile to stop advertising it as well.

> > As far as I am aware that works and we can hit the audio rates for it.

> I ran a test on Tegra124 Jetson-TK1 and 24-bit playback seems to work as
> Ben has indicated. So I don't think it is broken.

> Can you try Ben's testcase on Tegra30 (ie. generate a tone using sox and
> use aplay to play)?

Another test application that's quite useful for this sort of stuff is
speaker-test, it generates audio data directly in arbatrary formats and
it's part of alsa-utils so if you've got aplay and friends you may
already have it already installed.
Dmitry Osipenko Jan. 28, 2020, 5:42 p.m. UTC | #49
28.01.2020 15:13, Mark Brown пишет:
> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>> 24.01.2020 19:50, Jon Hunter пишет:
> 
>>>                 .rates = SNDRV_PCM_RATE_8000_96000,
>>>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
> 
>> It should solve the problem in my particular case, but I'm not sure that
>> the solution is correct.
> 
> If the format implemented by the driver is S24_3LE the driver should
> advertise S24_3LE.

It should be S24_LE, but seems we still don't know for sure.

>> The v5.5 kernel is released now with the broken audio and apparently
>> getting 24bit to work won't be trivial (if possible at all). Ben, could
>> you please send a patch to fix v5.5 by removing the S24 support
>> advertisement from the driver?
> 
> Why is that the best fix rather than just advertising the format
> implemented by the driver?

The currently supported format that is known to work well is S16_LE.

I'm suggesting to drop the S24_LE and S32_LE that were added by the
applied patches simply because this series wasn't tested properly before
it was sent out and turned out that it doesn't work well.

> I really don't understand why this is all taking so long, this thread
> just seems to be going round in interminable circles long after it
> looked like the issue was understood.  I have to admit I've not read
> every single message in the thread but it's difficult to see why it
> doesn't seem to be making any progress.

Ben was trying to make a fix for the introduced problem, but it's not
easy as we see now.

Perhaps the best solution should be to revert all of the three applied
patches and try again later on, once all current problems will be resolved.
Dmitry Osipenko Jan. 28, 2020, 5:45 p.m. UTC | #50
28.01.2020 18:26, Mark Brown пишет:
> On Tue, Jan 28, 2020 at 01:19:17PM +0000, Jon Hunter wrote:
>> On 28/01/2020 08:59, Ben Dooks wrote:
>>> On 27/01/2020 19:23, Dmitry Osipenko wrote:
>>>> 27.01.2020 22:20, Dmitry Osipenko пишет:
>>>> I also suspect that s32 may need some extra patches and thus could be
>>>> worthwhile to stop advertising it as well.
> 
>>> As far as I am aware that works and we can hit the audio rates for it.
> 
>> I ran a test on Tegra124 Jetson-TK1 and 24-bit playback seems to work as
>> Ben has indicated. So I don't think it is broken.
> 
>> Can you try Ben's testcase on Tegra30 (ie. generate a tone using sox and
>> use aplay to play)?
> 
> Another test application that's quite useful for this sort of stuff is
> speaker-test, it generates audio data directly in arbatrary formats and
> it's part of alsa-utils so if you've got aplay and friends you may
> already have it already installed.

I tried speaker-test and it doesn't support S24_LE:

# speaker-test -h

speaker-test 1.1.9

Usage: speaker-test [OPTION]...
-h,--help       help
-D,--device     playback device
-r,--rate       stream rate in Hz
-c,--channels   count of channels in stream
-f,--frequency  sine wave frequency in Hz
-F,--format     sample format
-b,--buffer     ring buffer size in us
-p,--period     period size in us
-P,--nperiods   number of periods
-t,--test       pink=use pink noise, sine=use sine wave, wav=WAV file
-l,--nloops     specify number of loops to test, 0 = infinite
-s,--speaker    single speaker test. Values 1=Left, 2=right, etc
-w,--wavfile    Use the given WAV file as a test sound
-W,--wavdir     Specify the directory containing WAV files
-m,--chmap      Specify the channel map to override
-X,--force-frequency    force frequencies outside the 30-8000hz range
-S,--scale      Scale of generated test tones in percent (default=80)

Recognized sample formats are: S8 S16_LE S16_BE FLOAT_LE S24_3LE S24_3BE
S32_LE S32_BE
Jon Hunter Jan. 28, 2020, 6:19 p.m. UTC | #51
On 28/01/2020 17:42, Dmitry Osipenko wrote:
> 28.01.2020 15:13, Mark Brown пишет:
>> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>>> 24.01.2020 19:50, Jon Hunter пишет:
>>
>>>>                 .rates = SNDRV_PCM_RATE_8000_96000,
>>>>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>
>>> It should solve the problem in my particular case, but I'm not sure that
>>> the solution is correct.
>>
>> If the format implemented by the driver is S24_3LE the driver should
>> advertise S24_3LE.
> 
> It should be S24_LE, but seems we still don't know for sure.

Why?

>>> The v5.5 kernel is released now with the broken audio and apparently
>>> getting 24bit to work won't be trivial (if possible at all). Ben, could
>>> you please send a patch to fix v5.5 by removing the S24 support
>>> advertisement from the driver?
>>
>> Why is that the best fix rather than just advertising the format
>> implemented by the driver?
> 
> The currently supported format that is known to work well is S16_LE.
> 
> I'm suggesting to drop the S24_LE and S32_LE that were added by the
> applied patches simply because this series wasn't tested properly before
> it was sent out and turned out that it doesn't work well.

S32_LE should be fine, however, I do have some concerns about S24_LE.

Jon
Jon Hunter Jan. 28, 2020, 6:42 p.m. UTC | #52
On 28/01/2020 13:19, Jon Hunter wrote:

...

> I ran a test on Tegra124 Jetson-TK1 and 24-bit playback seems to work as
> Ben has indicated. So I don't think it is broken.

Actually, it does not work on TK1. Pulseaudio was converting from
S24_3LE to S16_LE. If I use plughw to do the conversion it sounds
distorted indeed.

Ben what audio codec are you testing with?

Jon

Playing WAVE 'tmp.wav' : Signed 24 bit Little Endian in 3bytes, Rate
44100 Hz, Stereo
Plug PCM: Linear conversion PCM (S24_LE)
Its setup is:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S24_3LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 24
  buffer_size  : 4096
  period_size  : 512
  period_time  : 11609
  tstamp_mode  : NONE
  tstamp_type  : MONOTONIC
  period_step  : 1
  avail_min    : 512
  period_event : 0
  start_threshold  : 4096
  stop_threshold   : 4096
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
Slave: Hardware PCM card 0 'NVIDIA Tegra Jetson TK1' device 0 subdevice 0
Its setup is:
  stream       : PLAYBACK
  access       : MMAP_INTERLEAVED
  format       : S24_LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 32
  buffer_size  : 4096
  period_size  : 512
  period_time  : 11609
  tstamp_mode  : NONE
  tstamp_type  : MONOTONIC
  period_step  : 1
  avail_min    : 512
  period_event : 0
  start_threshold  : 4096
  stop_threshold   : 4096
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0
Dmitry Osipenko Jan. 29, 2020, 12:17 a.m. UTC | #53
28.01.2020 21:19, Jon Hunter пишет:
> 
> On 28/01/2020 17:42, Dmitry Osipenko wrote:
>> 28.01.2020 15:13, Mark Brown пишет:
>>> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>>>> 24.01.2020 19:50, Jon Hunter пишет:
>>>
>>>>>                 .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>                 .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>
>>>> It should solve the problem in my particular case, but I'm not sure that
>>>> the solution is correct.
>>>
>>> If the format implemented by the driver is S24_3LE the driver should
>>> advertise S24_3LE.
>>
>> It should be S24_LE, but seems we still don't know for sure.
> 
> Why?
/I think/ sound should be much more distorted if it was S24_3LE, but
maybe I'm wrong.
Jon Hunter Jan. 29, 2020, 10:49 a.m. UTC | #54
On 28/01/2020 12:13, Mark Brown wrote:
> I really don't understand why this is all taking so long, this thread
> just seems to be going round in interminable circles long after it
> looked like the issue was understood.  I have to admit I've not read
> every single message in the thread but it's difficult to see why it
> doesn't seem to be making any progress.

Sorry about that. On reviewing this with the audio team at NVIDIA, I was
told we don't support S24_LE for I2S. The reason being that the crossbar
between the DMA and I2S is not able to extract the correct 24-bits from
the 32-bit sample to feed to the I2S interface. The Tegra documentation
does show support for 24-bits, but not state explicit support for S24_LE.

Now Ben says that he has this working, but I am unable to reproduce
this, so before just dropping the S24_LE support, I would like to
understand how this is working for Ben in case there is something that
we have overlooked here.

Jon
Jon Hunter Jan. 29, 2020, 2:33 p.m. UTC | #55
On 29/01/2020 10:49, Jon Hunter wrote:
> 
> On 28/01/2020 12:13, Mark Brown wrote:
>> I really don't understand why this is all taking so long, this thread
>> just seems to be going round in interminable circles long after it
>> looked like the issue was understood.  I have to admit I've not read
>> every single message in the thread but it's difficult to see why it
>> doesn't seem to be making any progress.
> 
> Sorry about that. On reviewing this with the audio team at NVIDIA, I was
> told we don't support S24_LE for I2S. The reason being that the crossbar
> between the DMA and I2S is not able to extract the correct 24-bits from
> the 32-bit sample to feed to the I2S interface. The Tegra documentation
> does show support for 24-bits, but not state explicit support for S24_LE.
> 
> Now Ben says that he has this working, but I am unable to reproduce
> this, so before just dropping the S24_LE support, I would like to
> understand how this is working for Ben in case there is something that
> we have overlooked here.

Ah, I see that part of the problem is that patches 6 and 7 are yet to be
applied and without these the audio is completely distorted because
there is a mismatch in the data size between the APBIF and I2S
controller. Applying these patches it is not distorted but now I am
observing the clocking issue Ben reported and so the tone is not quite
right.

Ben, I was able to workaround the clocking issue by making the I2S word
clock 64 bits long and not 48.

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index bbf81b5aa723..3c9b4779e61b 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -143,7 +143,7 @@ static int tegra30_i2s_hw_params(struct
snd_pcm_substream *substream,
        case SNDRV_PCM_FORMAT_S24_LE:
                val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
                audio_bits = TEGRA30_AUDIOCIF_BITS_24;
-               sample_size = 24;
+               sample_size = 32;
                break;
        case SNDRV_PCM_FORMAT_S32_LE:
                val = TEGRA30_I2S_CTRL_BIT_SIZE_32;

For I2S I believe we only care about the edges of the word clock and so
we make the overall period of the word clock 64 bit clocks then we no
longer have an issue with the bit clock frequency. I assume that this
should also be fine for TDM modes as well.

Can you let me know if this works for you?

Cheers
Jon
Dmitry Osipenko Jan. 29, 2020, 3:22 p.m. UTC | #56
29.01.2020 17:33, Jon Hunter пишет:
> 
> On 29/01/2020 10:49, Jon Hunter wrote:
>>
>> On 28/01/2020 12:13, Mark Brown wrote:
>>> I really don't understand why this is all taking so long, this thread
>>> just seems to be going round in interminable circles long after it
>>> looked like the issue was understood.  I have to admit I've not read
>>> every single message in the thread but it's difficult to see why it
>>> doesn't seem to be making any progress.
>>
>> Sorry about that. On reviewing this with the audio team at NVIDIA, I was
>> told we don't support S24_LE for I2S. The reason being that the crossbar
>> between the DMA and I2S is not able to extract the correct 24-bits from
>> the 32-bit sample to feed to the I2S interface. The Tegra documentation
>> does show support for 24-bits, but not state explicit support for S24_LE.
>>
>> Now Ben says that he has this working, but I am unable to reproduce
>> this, so before just dropping the S24_LE support, I would like to
>> understand how this is working for Ben in case there is something that
>> we have overlooked here.
> 
> Ah, I see that part of the problem is that patches 6 and 7 are yet to be
> applied and without these the audio is completely distorted because
> there is a mismatch in the data size between the APBIF and I2S
> controller. Applying these patches it is not distorted but now I am
> observing the clocking issue Ben reported and so the tone is not quite
> right.
> 
> Ben, I was able to workaround the clocking issue by making the I2S word
> clock 64 bits long and not 48.
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index bbf81b5aa723..3c9b4779e61b 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -143,7 +143,7 @@ static int tegra30_i2s_hw_params(struct
> snd_pcm_substream *substream,
>         case SNDRV_PCM_FORMAT_S24_LE:
>                 val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>                 audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> -               sample_size = 24;
> +               sample_size = 32;
>                 break;
>         case SNDRV_PCM_FORMAT_S32_LE:
>                 val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> 
> For I2S I believe we only care about the edges of the word clock and so
> we make the overall period of the word clock 64 bit clocks then we no
> longer have an issue with the bit clock frequency. I assume that this
> should also be fine for TDM modes as well.
> 
> Can you let me know if this works for you?

cat /proc/asound/card0/pcm0p/sub0/hw_params

access: MMAP_INTERLEAVED
format: S24_LE
subformat: STD
channels: 2
rate: 48000 (48000/1)
period_size: 512
buffer_size: 4096

No distortion at all! Jon, thank you very much!
Ben Dooks Jan. 29, 2020, 5:52 p.m. UTC | #57
On 28/01/2020 12:13, Mark Brown wrote:
> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>> 24.01.2020 19:50, Jon Hunter пишет:
> 
>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
> 
>> It should solve the problem in my particular case, but I'm not sure that
>> the solution is correct.
> 
> If the format implemented by the driver is S24_3LE the driver should
> advertise S24_3LE.

I am fairly sure the format is S24_LE, the tegra3 hardware only
supports 24bits padded to 32 bits.

>> The v5.5 kernel is released now with the broken audio and apparently
>> getting 24bit to work won't be trivial (if possible at all). Ben, could
>> you please send a patch to fix v5.5 by removing the S24 support
>> advertisement from the driver?
> 
> Why is that the best fix rather than just advertising the format
> implemented by the driver?
> 
> I really don't understand why this is all taking so long, this thread
> just seems to be going round in interminable circles long after it
> looked like the issue was understood.  I have to admit I've not read
> every single message in the thread but it's difficult to see why it
> doesn't seem to be making any progress.
>
Ben Dooks Jan. 30, 2020, 8:04 a.m. UTC | #58
On 28/01/2020 18:42, Jon Hunter wrote:
> 
> On 28/01/2020 13:19, Jon Hunter wrote:
> 
> ...
> 
>> I ran a test on Tegra124 Jetson-TK1 and 24-bit playback seems to work as
>> Ben has indicated. So I don't think it is broken.
> 
> Actually, it does not work on TK1. Pulseaudio was converting from
> S24_3LE to S16_LE. If I use plughw to do the conversion it sounds
> distorted indeed.
> 
> Ben what audio codec are you testing with?

I think it is the sgtl5000
Ben Dooks Jan. 30, 2020, 8:05 a.m. UTC | #59
On 29/01/2020 00:17, Dmitry Osipenko wrote:
> 28.01.2020 21:19, Jon Hunter пишет:
>>
>> On 28/01/2020 17:42, Dmitry Osipenko wrote:
>>> 28.01.2020 15:13, Mark Brown пишет:
>>>> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>>>>> 24.01.2020 19:50, Jon Hunter пишет:
>>>>
>>>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>
>>>>> It should solve the problem in my particular case, but I'm not sure that
>>>>> the solution is correct.
>>>>
>>>> If the format implemented by the driver is S24_3LE the driver should
>>>> advertise S24_3LE.
>>>
>>> It should be S24_LE, but seems we still don't know for sure.
>>
>> Why?
> /I think/ sound should be much more distorted if it was S24_3LE, but
> maybe I'm wrong.

S24_3LE is IICC the wrong thing and we can't support it on the tegra3
Ben Dooks Jan. 30, 2020, 8:06 a.m. UTC | #60
On 29/01/2020 10:49, Jon Hunter wrote:
> 
> On 28/01/2020 12:13, Mark Brown wrote:
>> I really don't understand why this is all taking so long, this thread
>> just seems to be going round in interminable circles long after it
>> looked like the issue was understood.  I have to admit I've not read
>> every single message in the thread but it's difficult to see why it
>> doesn't seem to be making any progress.
> 
> Sorry about that. On reviewing this with the audio team at NVIDIA, I was
> told we don't support S24_LE for I2S. The reason being that the crossbar
> between the DMA and I2S is not able to extract the correct 24-bits from
> the 32-bit sample to feed to the I2S interface. The Tegra documentation
> does show support for 24-bits, but not state explicit support for S24_LE.
> 
> Now Ben says that he has this working, but I am unable to reproduce
> this, so before just dropping the S24_LE support, I would like to
> understand how this is working for Ben in case there is something that
> we have overlooked here.
> 
> Jon
> 
Let's go back to S24_3LE isn't supportable, S24_LE is
Ben Dooks Jan. 30, 2020, 8:17 a.m. UTC | #61
On 29/01/2020 14:33, Jon Hunter wrote:
> 
> On 29/01/2020 10:49, Jon Hunter wrote:
>>
>> On 28/01/2020 12:13, Mark Brown wrote:
>>> I really don't understand why this is all taking so long, this thread
>>> just seems to be going round in interminable circles long after it
>>> looked like the issue was understood.  I have to admit I've not read
>>> every single message in the thread but it's difficult to see why it
>>> doesn't seem to be making any progress.
>>
>> Sorry about that. On reviewing this with the audio team at NVIDIA, I was
>> told we don't support S24_LE for I2S. The reason being that the crossbar
>> between the DMA and I2S is not able to extract the correct 24-bits from
>> the 32-bit sample to feed to the I2S interface. The Tegra documentation
>> does show support for 24-bits, but not state explicit support for S24_LE.
>>
>> Now Ben says that he has this working, but I am unable to reproduce
>> this, so before just dropping the S24_LE support, I would like to
>> understand how this is working for Ben in case there is something that
>> we have overlooked here.
> 
> Ah, I see that part of the problem is that patches 6 and 7 are yet to be
> applied and without these the audio is completely distorted because
> there is a mismatch in the data size between the APBIF and I2S
> controller. Applying these patches it is not distorted but now I am
> observing the clocking issue Ben reported and so the tone is not quite
> right.

I thought they had been applied? I probably dragged them back in when
putting in the support for the test channel on the colibri.

> Ben, I was able to workaround the clocking issue by making the I2S word
> clock 64 bits long and not 48.

Ok, that will work for I2S case, but maybe not TDM? I'd have to check.

> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index bbf81b5aa723..3c9b4779e61b 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -143,7 +143,7 @@ static int tegra30_i2s_hw_params(struct
> snd_pcm_substream *substream,
>          case SNDRV_PCM_FORMAT_S24_LE:
>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>                  audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> -               sample_size = 24;
> +               sample_size = 32;
>                  break;
>          case SNDRV_PCM_FORMAT_S32_LE:
>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> 
> For I2S I believe we only care about the edges of the word clock and so
> we make the overall period of the word clock 64 bit clocks then we no
> longer have an issue with the bit clock frequency. I assume that this
> should also be fine for TDM modes as well.
> 
> Can you let me know if this works for you?

I'll be back in the office next week to test.
Clemens Ladisch Jan. 30, 2020, 9:31 a.m. UTC | #62
Ben Dooks wrote:
> On 29/01/2020 00:17, Dmitry Osipenko wrote:
>> 28.01.2020 21:19, Jon Hunter пишет:
>>> On 28/01/2020 17:42, Dmitry Osipenko wrote:
>>>> 28.01.2020 15:13, Mark Brown пишет:
>>>>> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>>>>>> 24.01.2020 19:50, Jon Hunter пишет:
>>>>>
>>>>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>                  .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>>
>>>>>> It should solve the problem in my particular case, but I'm not sure that
>>>>>> the solution is correct.
>>>>>
>>>>> If the format implemented by the driver is S24_3LE the driver should
>>>>> advertise S24_3LE.
>>>>
>>>> It should be S24_LE, but seems we still don't know for sure.
>>>
>>> Why?
>> /I think/ sound should be much more distorted if it was S24_3LE, but
>> maybe I'm wrong.
>
> S24_3LE is IICC the wrong thing and we can't support it on the tegra3

There are three ways of arranging 24-bit samples in memory:

S24_3LE: 24-bit samples in 24-bit words without padding
S24_LE:  24-bit samples in 32-bit words, aligned to the LSB
S32_LE:  24-bit samples in 32-bit words, aligned to the MSB

It is very unlikely that your hardware implements S24_LE.

If a S32_LE driver wants to describe how many bits are actually used, it
should install a msbits constraint (snd_pcm_hw_constraint_msbits()).


Regards,
Clemens
Ben Dooks Jan. 30, 2020, 9:39 a.m. UTC | #63
On 30/01/2020 09:31, Clemens Ladisch wrote:
> Ben Dooks wrote:
>> On 29/01/2020 00:17, Dmitry Osipenko wrote:
>>> 28.01.2020 21:19, Jon Hunter пишет:
>>>> On 28/01/2020 17:42, Dmitry Osipenko wrote:
>>>>> 28.01.2020 15:13, Mark Brown пишет:
>>>>>> On Mon, Jan 27, 2020 at 10:20:25PM +0300, Dmitry Osipenko wrote:
>>>>>>> 24.01.2020 19:50, Jon Hunter пишет:
>>>>>>
>>>>>>>>                   .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>>                   .formats = SNDRV_PCM_FMTBIT_S32_LE |
>>>>>>>> -                          SNDRV_PCM_FMTBIT_S24_LE |
>>>>>>>> +                          SNDRV_PCM_FMTBIT_S24_3LE |
>>>>>>
>>>>>>> It should solve the problem in my particular case, but I'm not sure that
>>>>>>> the solution is correct.
>>>>>>
>>>>>> If the format implemented by the driver is S24_3LE the driver should
>>>>>> advertise S24_3LE.
>>>>>
>>>>> It should be S24_LE, but seems we still don't know for sure.
>>>>
>>>> Why?
>>> /I think/ sound should be much more distorted if it was S24_3LE, but
>>> maybe I'm wrong.
>>
>> S24_3LE is IICC the wrong thing and we can't support it on the tegra3
> 
> There are three ways of arranging 24-bit samples in memory:
> 
> S24_3LE: 24-bit samples in 24-bit words without padding
> S24_LE:  24-bit samples in 32-bit words, aligned to the LSB
> S32_LE:  24-bit samples in 32-bit words, aligned to the MSB
> 
> It is very unlikely that your hardware implements S24_LE.

Which is wrong, since this is exactly what the hardware implements.

The DMA fetches 32 bit words, and then the front end dispatches only
24 bits of these to the I2S/TDM

>
Jon Hunter Jan. 30, 2020, 12:05 p.m. UTC | #64
On 30/01/2020 08:17, Ben Dooks wrote:

...

> I'll be back in the office next week to test.

Any objections to reverting this patch now for v5.6 and pushing to
stable for v5.5, then getting this fixed properly for v5.7?

Jon
Ben Dooks Jan. 30, 2020, 12:07 p.m. UTC | #65
On 30/01/2020 12:05, Jon Hunter wrote:
> 
> On 30/01/2020 08:17, Ben Dooks wrote:
> 
> ...
> 
>> I'll be back in the office next week to test.
> 
> Any objections to reverting this patch now for v5.6 and pushing to
> stable for v5.5, then getting this fixed properly for v5.7?

No, as long as it does not drag on too much.
The other option is just to remove the announcement of these
capabilities.

I think I need to check exactly what got merged and then go and
work out a full series.

The original authors and testers left Codethink nearly 2 years ago now.
Jon Hunter Jan. 30, 2020, 1:09 p.m. UTC | #66
On 30/01/2020 12:07, Ben Dooks wrote:
> On 30/01/2020 12:05, Jon Hunter wrote:
>>
>> On 30/01/2020 08:17, Ben Dooks wrote:
>>
>> ...
>>
>>> I'll be back in the office next week to test.
>>
>> Any objections to reverting this patch now for v5.6 and pushing to
>> stable for v5.5, then getting this fixed properly for v5.7?
> 
> No, as long as it does not drag on too much.

I won't if you can address the comments previously posted for the other
patches :-)

> The other option is just to remove the announcement of these
> capabilities.

This patch is not that big and so we may as well revert.

> I think I need to check exactly what got merged and then go and
> work out a full series.

Looks like 3 of 7 patches were merged. So if we revert this, then there
are still 5 that are needed.

Cheers!
Jon
Mark Brown Jan. 30, 2020, 1:10 p.m. UTC | #67
On Thu, Jan 30, 2020 at 08:17:37AM +0000, Ben Dooks wrote:
> On 29/01/2020 14:33, Jon Hunter wrote:

> > controller. Applying these patches it is not distorted but now I am
> > observing the clocking issue Ben reported and so the tone is not quite
> > right.

> I thought they had been applied? I probably dragged them back in when
> putting in the support for the test channel on the colibri.

There were review comments from Jon on patch 6 that you never responded
to.
Clemens Ladisch Jan. 30, 2020, 2:58 p.m. UTC | #68
Ben Dooks wrote:
> On 30/01/2020 09:31, Clemens Ladisch wrote:
>> S24_LE:  24-bit samples in 32-bit words, aligned to the LSB
>> S32_LE:  24-bit samples in 32-bit words, aligned to the MSB
>>
>> It is very unlikely that your hardware implements S24_LE.
>
> Which is wrong, since this is exactly what the hardware implements.
>
> The DMA fetches 32 bit words, and then the front end dispatches only
> 24 bits of these to the I2S/TDM

Which 24 bits?  Is the padding in the first byte (S32_LE) or in the
last byte (S24_LE)?


Regards,
Clemens
Ben Dooks Jan. 31, 2020, 10:50 a.m. UTC | #69
On 2020-01-30 14:58, Clemens Ladisch wrote:
> Ben Dooks wrote:
>> On 30/01/2020 09:31, Clemens Ladisch wrote:
>>> S24_LE:  24-bit samples in 32-bit words, aligned to the LSB
>>> S32_LE:  24-bit samples in 32-bit words, aligned to the MSB
>>> 
>>> It is very unlikely that your hardware implements S24_LE.
>> 
>> Which is wrong, since this is exactly what the hardware implements.
>> 
>> The DMA fetches 32 bit words, and then the front end dispatches only
>> 24 bits of these to the I2S/TDM
> 
> Which 24 bits?  Is the padding in the first byte (S32_LE) or in the
> last byte (S24_LE)?

I think the low 24 bits are sent from the 32 bit word.
Clemens Ladisch Jan. 31, 2020, 11:03 a.m. UTC | #70
Ben Dooks wrote:
> On 2020-01-30 14:58, Clemens Ladisch wrote:
>> Ben Dooks wrote:
>>> On 30/01/2020 09:31, Clemens Ladisch wrote:
>>>> S24_LE:  24-bit samples in 32-bit words, aligned to the LSB
>>>> S32_LE:  24-bit samples in 32-bit words, aligned to the MSB
>>>>
>>>> It is very unlikely that your hardware implements S24_LE.
>>>
>>> Which is wrong, since this is exactly what the hardware implements.
>>>
>>> The DMA fetches 32 bit words, and then the front end dispatches only
>>> 24 bits of these to the I2S/TDM
>>
>> Which 24 bits?  Is the padding in the first byte (S32_LE) or in the
>> last byte (S24_LE)?
>
> I think the low 24 bits are sent from the 32 bit word.

This would indeed by S24_LE.  If you change the driver to say that it
supports _only_ S24_LE, is the data then played correctly?


Regards,
Clemens
Ben Dooks March 19, 2020, 3:32 p.m. UTC | #71
On 2020-01-30 13:10, Mark Brown wrote:
> On Thu, Jan 30, 2020 at 08:17:37AM +0000, Ben Dooks wrote:
>> On 29/01/2020 14:33, Jon Hunter wrote:
> 
>> > controller. Applying these patches it is not distorted but now I am
>> > observing the clocking issue Ben reported and so the tone is not quite
>> > right.
> 
>> I thought they had been applied? I probably dragged them back in when
>> putting in the support for the test channel on the colibri.
> 
> There were review comments from Jon on patch 6 that you never responded
> to.

Hmm, I may have accidentally deleted those.

I will look to see if I can re-form the series and re-send in the next
couple of weeks. I've got no access currently to the machine and having
to deal with working from home for the next month or so.
Dmitry Osipenko March 20, 2020, 2:18 p.m. UTC | #72
19.03.2020 18:32, Ben Dooks пишет:
...
> Hmm, I may have accidentally deleted those.
> 
> I will look to see if I can re-form the series and re-send in the next
> couple of weeks. I've got no access currently to the machine and having
> to deal with working from home for the next month or so.

Great, looking forward to the new version :)
diff mbox series

Patch

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index 73f0dddeaef3..063f34c882af 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -127,7 +127,7 @@  static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct device *dev = dai->dev;
 	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 	unsigned int mask, val, reg;
-	int ret, sample_size, srate, i2sclock, bitcnt;
+	int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
 	struct tegra30_ahub_cif_conf cif_conf;
 
 	if (params_channels(params) != 2)
@@ -137,8 +137,19 @@  static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
 		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
+		audio_bits = TEGRA30_AUDIOCIF_BITS_16;
 		sample_size = 16;
 		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
+		audio_bits = TEGRA30_AUDIOCIF_BITS_24;
+		sample_size = 24;
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
+		audio_bits = TEGRA30_AUDIOCIF_BITS_32;
+		sample_size = 32;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -170,8 +181,8 @@  static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	cif_conf.threshold = 0;
 	cif_conf.audio_channels = 2;
 	cif_conf.client_channels = 2;
-	cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
-	cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
+	cif_conf.audio_bits = audio_bits;
+	cif_conf.client_bits = audio_bits;
 	cif_conf.expand = 0;
 	cif_conf.stereo_conv = 0;
 	cif_conf.replicate = 0;
@@ -306,14 +317,18 @@  static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_96000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE |
+			   SNDRV_PCM_FMTBIT_S24_LE |
+			   SNDRV_PCM_FMTBIT_S16_LE,
 	},
 	.capture = {
 		.stream_name = "Capture",
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_96000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE |
+			   SNDRV_PCM_FMTBIT_S24_LE |
+			   SNDRV_PCM_FMTBIT_S16_LE,
 	},
 	.ops = &tegra30_i2s_dai_ops,
 	.symmetric_rates = 1,