thesofproject / sof-test

BSD 3-Clause "New" or "Revised" License
12 stars 44 forks source link

bluetooth offload: sof_ipc4_init_input_audio_fmt: Unsupported audio format: 48000Hz, 16bit, 1 channels #1185

Open kv2019i opened 2 months ago

kv2019i commented 2 months ago

EDIT: this was https://github.com/thesofproject/sof/issues/9067, now transferred to sof-test

Describe the bug SOF commit c77a4feb2f1f15a3a29edf49b091b4b4a507be48 breaks BT offload tplgs like sof-nocodec-bt-mtl-lbm.tplg .

To Reproduce TPLG=/lib/firmware/intel/development/sof-nocodec-bt-mtl-lbm.tplg MODEL=MTLP_RVP_NOCODEC-bluetooth-offload SOF_TEST_INTERVAL=5 ~/sof-test/test-case/check-playback.sh -d 3 -l 1 -r 1 -F

Reproduction Rate 100%

Expected behavior Playback works.

Impact Playback fails.

Environment 1) SOF Commit: c77a4feb2f1f 2) Linux Commit: 4ede2a0ed114

2) Name of the topology file

Screenshots or console output

2024-04-17 11:13:38 UTC [REMOTE_COMMAND] sof-tplgreader.py /usr/lib/firmware/intel/development/sof-nocodec-bt-mtl-lbm.tplg -f 'type:playback & id:any & ~pcm:HDMI' -b ' pcm:HDA Digital' -s 0 -e
2024-04-17 11:13:38 UTC [REMOTE_INFO] ===== Testing: (Round: 1/1) (PCM: Port2 [hw:0,2]) (Loop: 1/1) =====
2024-04-17 11:13:38 UTC [REMOTE_COMMAND] aplay   -Dhw:0,2 -r 48000 -c 1 -f S16_LE -d 3 /dev/zero -v -q
aplay: set_params:1416: Unable to install hw params:
ACCESS:  RW_INTERLEAVED
FORMAT:  S16_LE
SUBFORMAT:  STD
SAMPLE_BITS: 16
FRAME_BITS: 16
CHANNELS: 1
RATE: 48000
PERIOD_TIME: 125000
PERIOD_SIZE: 6000
PERIOD_BYTES: 12000
PERIODS: 4
BUFFER_TIME: 500000
BUFFER_SIZE: 24000
BUFFER_BYTES: 48000
TICK_TIME: 0 

cc:

kv2019i commented 2 months ago

FYI @marc-hb @ranj063 @ujfalusi @plbossart

kv2019i commented 2 months ago

And FYI @fredoh9 Not sure if there's an issue with tplg-reader as well.

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

Is not expected to work, so the test case runs the wrong commeand, but also this fails

aplay -Dhw:0,2 -r 16000 -c 1 -f S16_LE -d 3 /dev/zero -v -q

ujfalusi commented 2 months ago

@kv2019i, what is in dmesg? What are not finding?

kv2019i commented 2 months ago

So the rate was limited to 48000 always- Fix in thesofproject/sof#9068.

There remains an issue with sof-test https://github.com/thesofproject/sof-test/pull/1174 . The change to prioritize 48000 ("intentionally put 48000 first to make first in the list") has the unwanted side-effect that our "check-playback-all-formats" test plan will now try to play with:

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

while before this tplgtool.py change, test was done with:

aplay -Dhw:0,2 -r 8000 -c 1 -f S16_LE -d 3 /dev/zero -v -q

Now both are actually within the capabilities advertized. The ALSA PCM capabilities do not allow us to describe the limitation that we don't support 48kHz 1ch 16bit on this PCM, so this will return an error if "hw:0,2" PCM access is used.

Not sure how to fix this. Any ideas @fredoh9 or @marc-hb ?

kv2019i commented 2 months ago

Following sof-test patch avoids the problem and makes the daily test plan pass (with thesofproject/sof#9068), but not really a fix right:

--- a/tools/tplgtool.py
+++ b/tools/tplgtool.py
@@ -612,8 +612,9 @@ class TplgFormatter:
     @staticmethod
     def _to_rate_string(rate):
         # Note: intentionally put 48000 first to make first in the list
-        bit_rates_dict = {7 : 48000,
+        bit_rates_dict = {
             1 : 8000, 3 : 16000, 4 : 22050, 5 : 32000, 6 : 44100,
+            7 : 48000,
             8 : 64000, 9 : 96000}
         return [ str(bit_rates_dict[bit]) for bit in 
ujfalusi commented 2 months ago

I wonder if the same issue present with other setups:

# git grep rate_m tools/topology/topology2/

tools/topology/topology2/cavs-benchmark-hda.conf:                       rate_min 8000
tools/topology/topology2/cavs-benchmark-hda.conf:                       rate_max 192000
tools/topology/topology2/cavs-benchmark-hda.conf:                       rate_min 8000
tools/topology/topology2/cavs-benchmark-hda.conf:                       rate_max 192000
tools/topology/topology2/cavs-mixin-mixout-hda.conf:                    rate_min $HDA_ANALOG_PLAYBACK_RATE
tools/topology/topology2/cavs-mixin-mixout-hda.conf:                    rate_max $HDA_ANALOG_PLAYBACK_RATE
tools/topology/topology2/cavs-mixin-mixout-hda.conf:                    rate_min $HDA_ANALOG_CAPTURE_RATE
tools/topology/topology2/cavs-mixin-mixout-hda.conf:                    rate_max $HDA_ANALOG_CAPTURE_RATE
tools/topology/topology2/cavs-nocodec.conf:                                     rate_min 8000
tools/topology/topology2/cavs-nocodec.conf:                                     rate_max 192000
tools/topology/topology2/cavs-nocodec.conf:                                     rate_min 8000
tools/topology/topology2/cavs-nocodec.conf:                                     rate_max 192000
tools/topology/topology2/cavs-sdw-src-gain-mixin.conf:                  rate_min 16000
tools/topology/topology2/cavs-sdw-src-gain-mixin.conf:                  rate_max 48000
tools/topology/topology2/cavs-src-mixin-mixout-hda.conf:                        rate_min 8000
tools/topology/topology2/cavs-src-mixin-mixout-hda.conf:                        rate_max 192000
tools/topology/topology2/include/common/pcm_caps.conf:#         rate_min                48000
tools/topology/topology2/include/common/pcm_caps.conf:#         rate_max                48000
tools/topology/topology2/include/common/pcm_caps.conf:  DefineAttribute."rate_min" {}
tools/topology/topology2/include/common/pcm_caps.conf:  DefineAttribute."rate_max" {}
tools/topology/topology2/platform/intel/bt-generic.conf:                        rate_min 8000
tools/topology/topology2/platform/intel/bt-generic.conf:                        rate_max 48000
tools/topology/topology2/platform/intel/bt-generic.conf:                        rate_min 8000
tools/topology/topology2/platform/intel/bt-generic.conf:                        rate_max 48000
tools/topology/topology2/platform/intel/dmic-generic.conf:                      rate_min $DMIC0_RATE
tools/topology/topology2/platform/intel/dmic-generic.conf:                      rate_max $DMIC0_RATE
tools/topology/topology2/platform/intel/dmic-wov.conf:                  rate_min        16000
tools/topology/topology2/platform/intel/dmic-wov.conf:                  rate_max        16000
tools/topology/topology2/platform/intel/dmic-wov.conf:                  rate_min        16000
tools/topology/topology2/platform/intel/dmic-wov.conf:                  rate_max        16000
tools/topology/topology2/platform/intel/dmic-wov.conf:                  rate_min     16000
tools/topology/topology2/platform/intel/dmic-wov.conf:                  rate_max     16000
tools/topology/topology2/platform/intel/dmic1-passthrough.conf:                        rate_min $DMIC1_RATE
tools/topology/topology2/platform/intel/dmic1-passthrough.conf:                        rate_max $DMIC1_RATE
tools/topology/topology2/platform/intel/hdmi-in-capture.conf:                   rate_min 48000
tools/topology/topology2/platform/intel/hdmi-in-capture.conf:                   rate_max 48000
tools/topology/topology2/platform/intel/hdmi-in-capture.conf:                   rate_min 48000
tools/topology/topology2/platform/intel/hdmi-in-capture.conf:                   rate_max 48000
marc-hb commented 2 months ago

As expected, all MTLP_RVP_NOCODEC-bluetooth-offload tests still failing in today's daily 40182

marc-hb commented 2 months ago

Daily tests 40387 are still all failing on MTLP_RVP_NOCODEC-bluetooth-offload even with today's commit 278ecc5b74ad "fix rate constraints".

kv2019i commented 2 months ago

@marc-hb wrote:

Daily tests 40387 are still all failing on MTLP_RVP_NOCODEC-bluetooth-offload even with today's commit 278ecc5 "fix rate constraints".

This is expected. The SOF tplg is now correct and FW behaves as expected.

The test fails as sof-test tries a combination that is not supported:

2024-04-26 14:12:31 UTC [REMOTE_INFO] ===== Testing: (Round: 1/1) (PCM: Port2 [hw:0,2]) (Loop: 1/1) =====
2024-04-26 14:12:31 UTC [REMOTE_COMMAND] aplay   -Dhw:0,2 -r 48000 -c 1 -f S16_LE -d 3 /dev/zero -v -q
aplay: set_params:1416: Unable to install hw params:

The PCM does not support opening 48kHz in mono/1ch mode, so this command is expected to fail. The problem is now how can sof-test no what to test. Current code assumes all combinations of supported channels and support sampling rates are supported, but this is not the case.

Earlier this worked with some luck as sof-test picked a combination to test that happened to work. In any case, this is a sof-test problem now.

marc-hb commented 2 months ago

In any case, this is a sof-test problem now.

I was about to transfer this issue to sof-test to avoid creating a new one (the context is spread in too many Github places already) but I'm not so sure now. Someone named @kv2019i wrote this:

https://github.com/thesofproject/sof/pull/9068#discussion_r1575898007

ack. his is just hitting the limits of what we can express in capabilities. the topology description of PCM cannot express the PCM does not support all combinations of supported channels and rates.

If the required information isn't available in the first place then sof-test cannot divine it and this is just not usable and not testable, is it? Let's leave sof-test aside for a moment and focus on real use cases (which sof-test should be as close to as possible). How does "real" software know which combinations are valid versus not? Some UCM magic maybe?

kv2019i commented 2 months ago

@marc-hb There are a possibilities. The alsa-lib C interface allows applications to discover such complex constraints. I.e. application can leave channel count as "open" or use the "_near" variants to ask a specific value but be open to adjust to hardware capabilities. sof-test does not use this interface, but rather it implements its own discover of capability and aplay/arecord are instructed to use specific settings via the "hw:x" layer and explicit setting of all key audio parameters. This makes sense as we want to specifically test the audio drivers and want to control the audio parameters used in the tests.

In the specific case of BT, the set of supported audio formats comes from the set of Bluetooth profiles we support. That is currently HFP and A2DP and there is a enumerated set of formats the Bluetooth chip and audio DSP support. So it's hardcoded in the user-space code for BT offload that these specific audio formats are used.

So a pragmatic solution would perhaps be to detect (in sof-test) we are testing Bluetooth offload, and ignore known bad audio format combinations.

marc-hb commented 2 months ago

(in sof-test) we are testing Bluetooth offload, and ignore known bad audio format combinations.

OK but how? In pseudo-code.

cc: @yongzhi1

ujfalusi commented 2 months ago

@kv2019i, in the kernel we can have rules (with snd_pcm_hw_rule_add()). With that you can say that for example mono is only allowed with 16KHz (not sure what the constraint is). Discovering or to be precise describing the constraint is another question, probably we could use the copier formats to scan through and 'rule out' combinations that is not listed.

kv2019i commented 2 months ago

@marc-hb I have no easy answer. Something along the lines of:

  if (Intel bluetooth offload)
        if (rate == 48000)
            channels = 2;
       else
            channels = 1;

Not sure how to implement the if condition check. This is a property of the tplg in the end. This is a clunky solution perhaps, but if the idea was to not write a completely new test for BT offload, we need to somehow figure out a way to decorate the generic test code so it can work with this specific type of tplg.

ujfalusi commented 2 months ago

right, a kernel side of hw_rule would not work for this for sure. If user space asks for not supported combination of channel and rate then ALSA will not allow it, but in a higher level, I guess we fail now with the copier format or blob lookup.

marc-hb commented 2 months ago

The alsa-lib C interface allows applications to discover such complex constraints. I.e. application can leave channel count as "open" or use the "_near" variants to ask a specific value but be open to adjust to hardware capabilities. sof-test does not use this interface, but rather it implements its own discover of capability and aplay/arecord are instructed to use specific settings via the "hw:x" layer and explicit setting of all key audio parameters. This makes sense as we want to specifically test the audio drivers and want to control the audio parameters used in the tests.

It makes sense for sof-test to explicitly set parameters but it does not make sense for sof-test to be ignorant and not use the same discovery APIs as other ALSA applications (trying all combinations). sof-test is really an ALSA test suite in disguise, so ALSA discovery APIs should be part of the sof-test coverage - and this would also provide the data that has been missing here.

In any case this looks like a major sof-test change.

marc-hb commented 2 months ago
  1. discover "real" capabilities by banging the ALSA APIs.

Trying to summarize a quick call with @ranj063 (thanks!), we have 4 more possible solutions:

  1. Make testing bluetooth a very special case in sof-test as already mentioned. It may sound like a "quick & dirty" solution but I'm actually not even sure about the "quick" part: need to check how each sof-test currently iterates on PCMs and capabilities. A lot of copy/paste/diverge there would not surprise me.

  2. Much more generic: try to leverage one of sof-test's topology parsers, find the host-copier and extract the exhaustive list of audio_formats (like these: https://github.com/thesofproject/sof/commit//b6d34fb4a837c). Switch to these to guide test iterations (again: probably not a small effort for the same reason)

  3. From @ranj063 , an apparently very long shot: add more PCM capabilities to have more than two per pipeline. That is: more than one in each direction. Unfortunately not off to a good start considering the constant size of struct snd_soc_tplg_stream_caps caps[2]; /* playback and capture for DAI */

  4. Also from @ranj063, the "overkill" solution (her name): also change Bluetooth topologies but this time add more, "narrower" and simpler pipelines (which gives more PCMs).

ranj063 commented 2 months ago

4. Also from @ranj063, the "overkill" solution (her name): also change Bluetooth topologies but this time add more, "narrower" and simpler pipelines (which gives more PCMs).

The more I think about it. this seems like the most logical solution. Advertising a combination of rates/channels that cannot possibly work doesn't seem like the right thing to do

kv2019i commented 2 months ago

@ranj063 wrote:

  1. Also from @ranj063, the "overkill" solution (her name): also change Bluetooth topologies but this time add more, "narrower" and simpler pipelines (which gives more PCMs).

The more I think about it. this seems like the most logical solution. Advertising a combination of rates/channels that cannot possibly work doesn't seem like the right thing to do

But this is not according to the design we have with CRAS. We identify a single PCM for offload and that single PCM is expected to support Bluetooth audio configurations. The number of configurations will increase later, so it will not scale to have distinct PCM for all possible combinations.

The valid PCM values are standardized in the Bluetooth standards, so I'm not sure why we need to solve the discovery problem in a more generic fashion than is required for product use.

ujfalusi commented 2 months ago

I tend to agree with @kv2019i, here the test should know what it is testing. BT offload is a special case, needing special treatment. No matter how much constraining, heuristics we put in kernel and/or in tplg it is not going to stop user space for asking for exactly a combination which is not supported, right?

There is always going to be a corner case and whack a mole is not fun in a long run.

But, if we really want to do this then we need topology change and kernel change to bring up the rules to aid discovery (which application can just not use and go hard for the non supported combination - again). Coding this in tplg and using the tplg itself in CI to magically handle this? Adding a mini SOF-UCM in tplg? We cannot expect applications to read topologies. Does it really worth the effort?