ALSA: pcm: add support for 24kHz sample rate

[yongzhi1]

Bluetooth LC-3 codec uses 24kHz for standard quality, so add the new rate to the known-rates list for PCM offload support.

[marc-hb]

This breaks aplay in every test across the Zephyr/IPC4 board: "unable to install hw_params" or "invalid argument", "Unsupported audio format",...

https://sof-ci.01.org/linuxpr/PR4966/build2620/devicetest/index.html
https://sof-ci.01.org/linuxpr/PR4966/build2621/devicetest/index.html
https://sof-ci.01.org/linuxpr/PR4966/build2622/devicetest/index.html

The stable-v2.2 branch is fine:
https://sof-ci.01.org/linuxpr/PR4966/build2623/devicetest/index.html

[yongzhi1]

This breaks aplay in every test across the Zephyr/IPC4 board: "unable to install hw_params" or "invalid argument", "Unsupported audio format",...

https://sof-ci.01.org/linuxpr/PR4966/build2620/devicetest/index.html https://sof-ci.01.org/linuxpr/PR4966/build2621/devicetest/index.html https://sof-ci.01.org/linuxpr/PR4966/build2622/devicetest/index.html

The stable-v2.2 branch is fine: https://sof-ci.01.org/linuxpr/PR4966/build2623/devicetest/index.html

It fails fast :), thanks for the quick feedback, did not observe the failure on my board though, checking why it chose 44.1k now

[  227.061181] kernel: snd_sof:sof_ipc4_pcm_dai_link_fixup: sof-audio-pci-intel-lnl 0000:00:1f.3: Set NoCodec-0 to 32 bit format
[  227.061215] kernel: snd_sof_intel_hda_common:generic_calc_stream_format: sof-audio-pci-intel-lnl 0000:00:1f.3: format_val=0x41, rate=48000, ch=2, format=10
[  227.061224] kernel: snd_sof:sof_pcm_hw_params: sof-audio-pci-intel-lnl 0000:00:1f.3: pcm: hw params stream 0 dir 0
[  227.061232] kernel: snd_sof_intel_hda_common:hda_dsp_stream_hw_params: sof-audio-pci-intel-lnl 0000:00:1f.3: FW Poll Status: reg[0x1e0]=0x40000 successful
[  227.061265] kernel: snd_sof_intel_hda_common:hda_dsp_stream_hw_params: sof-audio-pci-intel-lnl 0000:00:1f.3: FW Poll Status: reg[0x1e0]=0x40000 successful
[  227.061275] kernel: snd_sof_intel_hda_common:hda_dsp_stream_setup_bdl: sof-audio-pci-intel-lnl 0000:00:1f.3: period_bytes:0x5624
[  227.061277] kernel: snd_sof_intel_hda_common:hda_dsp_stream_setup_bdl: sof-audio-pci-intel-lnl 0000:00:1f.3: periods:4
[  227.061296] kernel: snd_sof:sof_ipc4_prepare_copier_module: sof-audio-pci-intel-lnl 0000:00:1f.3: copier host-copier.0.playback, type 23
[  227.061302] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: sof_ipc4_init_input_audio_fmt: Unsupported audio format: 44100Hz, 16bit, 2 channels
[  227.061405] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: failed to prepare widget host-copier.0.playback
[  227.061429] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: Failed to prepare connected widgets
[  227.061449] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: error: failed widget list set up for pcm 0 dir 0
[  227.061472] kernel: sof-audio-pci-intel-lnl 0000:00:1f.3: ASoC: error at snd_soc_pcm_component_hw_params on 0000:00:1f.3: -22

[marc-hb]

checking why it chose 44.1k now

Maybe related to:

• sof-tplgreader: parse rates from topology file sof-test#1174 (comment)

Wild guess.

[yongzhi1]

I am not able to find a satisfying explanation, tried the same cmd as shown in the log https://sof-ci.01.org/linuxpr/PR4966/build2621/devicetest/index.html?model=MTLP_SDW_AIOC&testcase=check-playback-10sec

aplay   -Dhw:0,2 -r 48000 -c 2 -f S16_LE -d 10 /dev/zero -v -q

And snd_pcm_hw_params_choose() found 48K correctly, the aplay exit without error:

           aplay-7220    [006] .....  1167.603204: hw_interval_param: pcmC0D2p:0 000/023 RATE 0 1 [48000 48000] 0 1 [48000 48000]
           aplay-7220    [006] .....  1167.603204: hw_interval_param: pcmC0D2p:0 000/023 PERIOD_TIME 0 0 (85333 85334) 0 0 (85333 85334)
           aplay-7220    [006] .....  1167.603205: hw_interval_param: pcmC0D2p:0 000/023 BUFFER_SIZE 0 1 [16384 16384] 0 1 [16384 16384]

(use sof-dev kernel directly on my DUT failed to boot, so only tested on https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-6.1)

Any insight of the CI tests failure @ranj063 @ujfalusi?

[ujfalusi]

@yongzhi1, these defines cannot be change like that, if at all.

[ujfalusi]

You cannot do this.
Bit 7 was 48KHz before this patch, now it is 44.1KHz.
This will have catastrophic consequences with anything which uses audio.

[ujfalusi]

It should be possible to do this actually, SNDRV_PCM_RATE_ is kernel internal define

[charleskeepax]

This has been discussed a reasonable number of times upstream before, for example:

https://mailman.alsa-project.org/pipermail/alsa-devel/2013-July/064453.html

Historically, there has been some push back on including 24kHz, and the suggestion has been to use RATE_KNOT and constraints instead. That said 384k also got some push back:

https://mailman.alsa-project.org/pipermail/alsa-devel/2016-January/103502.html

And then was finally accepted in commit 4cc4531 ("ALSA: pcm: add support for 352.8KHz and 384KHz sample rate"). But definitely you want to send a change like this upstream first before doing much that depends on it.

[yongzhi1]

Thanks @ujfalusi @charleskeepax and @marc-hb for your inputs.

For the CI error, I tried sof-dev branch on Ubuntu, still am not able to repro, so I think it's not caused by this PR.
From https://github.com/thesofproject/linux/blob/topic/sof-dev/include/sound/pcm.h#L107, it hinted that changes are allowable, but we need a strong reason to justify, so let me sync offline with you experts.

/* If you change this don't forget to change rates[] table in pcm_native.c */

[marc-hb]

For the CI error, I tried sof-dev branch on Ubuntu, still am not able to repro, so I think it's not caused by this PR.

Only this PR is causing such "unsupported audio formats" all across the board. No other PR or daily test has any remotely similar failures. So, this PR is definitely triggering these failures. That does not mean this PR is incorrect but it is for sure the trigger.

What I find especially interesting is: this PR does not fail with the stable-v2.2 FW branch, which happens to use topology v1. All other branches fail and they use topology v2. Maybe not a coincidence?

sof-test depends heavily on ALSA libraries and topology parsing, so there is probably some dependency or even hardcoding there.

[ujfalusi]

@marc-hb, my guess is that tplg1 sets only rate_min and rate_max, they are just unsigned values (48000, 48000 for example) while with tplg2 (since thesofproject/sof@c77a4fe) we set the rates and these are parsed to be bits in the final caps->rates from source to tplg binary. The tplg binary is parsed by kernel and the rates, rate_min/max is copied as it is to the PCM's capabilities.
So a misaligned bit will cause some issue:
Before this PR, the tplg2's 48000 would end up as BIT(7) in rates and the kernel is aligned that 48000 is BIT(7).
With this PR, the tplg2's 48000 remain BIT(7) in rates while the kernel thinks that BIT(7) is 44100 (48000 is BIT(8)).

Re-defining BITs is not going to be an easy task between kernel, user space, topology compiler and applications. All need to be have the same understanding of the bits.

Update: alsa-lib references:
https://github.com/alsa-project/alsa-lib/blob/master/src/topology/tplg_local.h#L43-L61
https://github.com/alsa-project/alsa-lib/blob/master/src/topology/pcm.c#L24-L40
https://github.com/alsa-project/alsa-lib/blob/master/src/topology/pcm.c#L355-L376

[yongzhi1]

Hi, @marc-hb @ujfalusi , thanks, now I was reminded that I have updated alsa-lib with alsa-project/alsa-lib#396 on my build machine to test 24K, that's why I did not see the CI error. There is an implicit dependency between snd_pcm_rates defined in tplg_local.h and SNDRV_PCM_RATE defined kernel side include/sound/pcm.h.

[marc-hb]

I have updated alsa-lib with alsa-project/alsa-lib#396 on my build machine to test 24K, that's why I did not see the CI error.

This is exactly for this type of reasons that validation exists :-)

Re-defining BITs is not going to be an easy task between kernel, user space, topology compiler and applications. All need to be have the same understanding of the bits.

There is an implicit dependency between snd_pcm_rates defined in tplg_local.h and SNDRV_PCM_RATE defined kernel side include/sound/pcm.h.

So this is the "implicit ABI" that we were all suspecting the whole time, thanks!

"Implicit ABI" = really time-consuming so very bad. A very cheap, low-tech fix of incredible value is... adding a comment in the source warning about the other places where these bits are duplicated. Such a comment is basic copy/paste/diverge hygiene in general but it's obviously much more important when it's an ABI across different components versioned separately.

Very sad to see zero comment in any of these places.

[plbossart]

@yongzhi1 I think it's safe to say this PR is not aligned with the directions provided before by maintainers. There's no line of sight to extend the enums for frequencies, so my recommendation is to move on and use the RATE_CONTINUOUS and constraints.

[yongzhi1]

@yongzhi1 I think it's safe to say this PR is not aligned with the directions provided before by maintainers. There's no line of sight to extend the enums for frequencies, so my recommendation is to move on and use the RATE_CONTINUOUS and constraints.

Ack, let me close this and experiment with the RATE_CONTINUOUS and constraints. Thanks for the code review!!