| Message ID | 20191018154833.7560-3-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/7] ASoC: tegra: add a TDM configuration callback | expand |
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
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.
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?
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.
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
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?
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.
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
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.
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
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
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?
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.
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.
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.
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.
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
[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.
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?
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.
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,
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.
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
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)
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
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?
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.
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.
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.
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...
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.
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
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.
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
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
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
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.
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. >
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)
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?
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.
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
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?
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.
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.
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
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.
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.
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.
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
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
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
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.
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
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
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!
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. >
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
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
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
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.
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
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 >
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
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.
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
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.
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
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.
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
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.
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 --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,