On 18/10/2019 16:48, Ben Dooks wrote:

From: Edward Cragg edward.cragg@codethink.co.uk

Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' driver.

Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: merge fix for power management] [ben.dooks@codethink.co.uk: add review change for fsync of 1 clock] Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk

---

sound/soc/tegra/tegra30_i2s.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..73f0dddeaef3 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,34 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; }

+static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,

• 	       unsigned int tx_mask, unsigned int rx_mask,
  

• 	       int slots, int slot_width)
  

+{

• struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
• unsigned int mask, val;
• dev_dbg(dai->dev, "%s: txmask=0x%08x rxmask=0x%08x slots=%d width=%d\n",

•  __func__, tx_mask, rx_mask, slots, slot_width);
  

• mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |

•       TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |
  

•       TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;
  

• val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |

•      (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |
  

•      ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
  

• pm_runtime_get_sync(dai->dev);
• regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
• /* set the fsync width to minimum of 1 clock width */
• regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL,

• 	   TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, 0x0);
  

• pm_runtime_put(dai->dev);
• return 0;

+}

static int tegra30_i2s_probe(struct snd_soc_dai *dai) { struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); @@ -268,6 +296,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = { .set_fmt = tegra30_i2s_set_fmt, .hw_params = tegra30_i2s_hw_params, .trigger = tegra30_i2s_trigger,

• .set_tdm_slot = tegra30_i2s_set_tdm,

};

static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {

Thanks!

Reviewed-by: Jon Hunter jonathanh@nvidia.com

Cheers Jon

The patch

ASoC: tegra: Allow 24bit and 32bit samples

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks, Mark

From f3ee99087c8ca0ecfdd549ef5a94f557c42d5428 Mon Sep 17 00:00:00 2001

From: Edward Cragg edward.cragg@codethink.co.uk Date: Fri, 18 Oct 2019 16:48:28 +0100 Subject: [PATCH] ASoC: tegra: Allow 24bit and 32bit samples

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 Reviewed-by: Jon Hunter jonathanh@nvidia.com Link: https://lore.kernel.org/r/20191018154833.7560-3-ben.dooks@codethink.co.uk Signed-off-by: Mark Brown broonie@kernel.org --- 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 e6d548fa980b..48a09c9d60be 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; @@ -277,14 +288,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,

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 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_h...

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_h...

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 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_h...

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.

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_h...

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.

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_h...

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.

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_h... >> >> >> > > 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.

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_h... >>>> >>>> >>>> >>>> >>> >>> 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

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

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?

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.

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.

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 Thu, Oct 24, 2019 at 05:12:21PM +0100, Jon Hunter wrote:

On 18/10/2019 16:48, Ben Dooks wrote:

...

@@ -90,10 +92,12 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, TEGRA30_I2S_CTRL_LRCK_MASK; switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_DSP_A:

• ch_val = TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE;
  

Sorry, I just saw the feedback on the previous iteration. I don't think we want to set the polarity but based upon the format that is passed ...

The polarity should be set based on the the inversion flags in the format, normally both DSP modes trigger on the rising edge of the LRCLK. The difference is if there's a delay before the first data bit or not.

The patch

ASoC: tegra: disable rx_fifo after disable stream

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks, Mark

From 8c05f6af7b7d713e327cd6df5a8889c32fc1c10f Mon Sep 17 00:00:00 2001

From: Ben Dooks ben.dooks@codethink.co.uk Date: Fri, 18 Oct 2019 16:48:30 +0100 Subject: [PATCH] ASoC: tegra: disable rx_fifo after disable stream

We see odd FIFO overruns with this, we assume the best thing to do is to disable the RX I2S frontend first, and then disable the FIFO that is using it.

This also fixes an issue where using multi-word frames (TDM) have partial samples stuck in the FIFO which then get read out when the next capture is started.

Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk Acked-by: Jon Hunter jonathanh@nvidia.com Link: https://lore.kernel.org/r/20191018154833.7560-5-ben.dooks@codethink.co.uk Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/tegra/tegra30_i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 8f8924060d9d..dbed3c5408e7 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -231,9 +231,9 @@ static void tegra30_i2s_start_capture(struct tegra30_i2s *i2s)

static void tegra30_i2s_stop_capture(struct tegra30_i2s *i2s) { - tegra30_ahub_disable_rx_fifo(i2s->capture_fifo_cif); regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, TEGRA30_I2S_CTRL_XFER_EN_RX, 0); + tegra30_ahub_disable_rx_fifo(i2s->capture_fifo_cif); }

static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,

On 18/10/2019 16:48, Ben Dooks wrote:

If the hw_params uses a different bit or channel count, then we need to change both the I2S unit's CIF configuration as well as the APBIF one.

To allow changing the APBIF, add a call to reconfigure the RX or TX FIFO without changing the DMA or allocation, and get the I2S driver to call it once the hw params have been calculate.

Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk

sound/soc/tegra/tegra30_ahub.c | 115 ++++++++++++++++++--------------- sound/soc/tegra/tegra30_ahub.h | 5 ++ sound/soc/tegra/tegra30_i2s.c | 2 + 3 files changed, 69 insertions(+), 53 deletions(-)

diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 952381260dc3..24bc03428b45 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -84,12 +84,40 @@ static int tegra30_ahub_runtime_resume(struct device *dev) return 0; }

+int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif,

• 	       struct tegra30_ahub_cif_conf *cif_conf)
  

+{

• int channel = rxcif - TEGRA30_AHUB_RXCIF_APBIF_RX0;
• u32 reg, val;
• pm_runtime_get_sync(ahub->dev);
• reg = TEGRA30_AHUB_CHANNEL_CTRL +

•      (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE);
  

• val = tegra30_apbif_read(reg);
• val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |

•  TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);
  

• val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |

•       TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |
  

•       TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;
  

• tegra30_apbif_write(reg, val);
• cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_RX;
• reg = TEGRA30_AHUB_CIF_RX_CTRL +

•      (channel * TEGRA30_AHUB_CIF_RX_CTRL_STRIDE);
  

• ahub->soc_data->set_audio_cif(ahub->regmap_apbif, reg, cif_conf);
• pm_runtime_put(ahub->dev);
• return 0;

+} +EXPORT_SYMBOL_GPL(tegra30_ahub_setup_rx_fifo);

int tegra30_ahub_allocate_rx_fifo(enum tegra30_ahub_rxcif *rxcif, char *dmachan, int dmachan_len, dma_addr_t *fiforeg) { int channel;

• u32 reg, val; struct tegra30_ahub_cif_conf cif_conf;

  channel = find_first_zero_bit(ahub->rx_usage,

@@ -104,37 +132,14 @@ int tegra30_ahub_allocate_rx_fifo(enum tegra30_ahub_rxcif *rxcif, *fiforeg = ahub->apbif_addr + TEGRA30_AHUB_CHANNEL_RXFIFO + (channel * TEGRA30_AHUB_CHANNEL_RXFIFO_STRIDE);

• pm_runtime_get_sync(ahub->dev);

• memset(&cif_conf, 0, sizeof(cif_conf));

• reg = TEGRA30_AHUB_CHANNEL_CTRL +

•      (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE);
  

• val = tegra30_apbif_read(reg);

• val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |

•  TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);
  

• val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |

•       TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |
  

•       TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;
  

• tegra30_apbif_write(reg, val);

• 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.expand = 0;

• cif_conf.stereo_conv = 0;

• cif_conf.replicate = 0;

• cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_RX;

• cif_conf.truncate = 0;

• cif_conf.mono_conv = 0;

• reg = TEGRA30_AHUB_CIF_RX_CTRL +

•      (channel * TEGRA30_AHUB_CIF_RX_CTRL_STRIDE);
  

• ahub->soc_data->set_audio_cif(ahub->regmap_apbif, reg, &cif_conf);

• pm_runtime_put(ahub->dev);

• return 0;

• return tegra30_ahub_setup_rx_fifo(*rxcif, &cif_conf);

I am not sure why it is necessary to call this here now because this is called everytime you call hw_params() right? It seems redundant to me.

Cheers Jon

On 18/10/2019 16:48, Ben Dooks wrote:

If the CIF is not configured as 16 or 8 bit, then the packing for 8/16 bits should not be enabled as the hardware only supports 8 or 16 bit packing.

Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk

sound/soc/tegra/tegra30_ahub.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 24bc03428b45..0768c6b6dc25 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -96,10 +96,17 @@ int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif, (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); val = tegra30_apbif_read(reg); val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |

•  TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);
  

• val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |

•       TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |
  

•       TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;
  

•  TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK |
  

•  TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN);
  

• val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT);

• if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16 ||

•    cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)
  

• val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN;
  

• if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16)

• val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;
  

• if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)

• val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_8_4;
  

• tegra30_apbif_write(reg, val);

  cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_RX;

@@ -203,10 +210,16 @@ int tegra30_ahub_setup_tx_fifo(enum tegra30_ahub_txcif txcif, (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); val = tegra30_apbif_read(reg); val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_MASK |

•  TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_MASK);
  

• val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_SHIFT) |

•       TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN |
  

•       TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_16;
  

•  TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_MASK |
  

•  TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN);
  

• val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_SHIFT);

• if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16 ||

•    cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)
  

• val |= TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN;
  

• if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16)

• val |= TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_16;
  

• if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)

• val |= TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_8_4;
  

  tegra30_apbif_write(reg, val);

  cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_TX;

Looks good to me.

Reviewed-by: Jon Hunter jonathanh@nvidia.com

Cheers Jon