Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kv2019i opened this issue Apr 22, 2024 · 20 comments
Assignees
Labels
P1 Blocker bugs or important features type:test coverage gap This requires a new test case, not just fixing one

Comments

@kv2019i
Copy link
Contributor

kv2019i commented Apr 22, 2024

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

  3. Name of the topology file

    • Topology: sof-nocodec-bt-lbm.tplg
  4. Name of the platform(s) on which the bug is observed.

    • Platform: Intel MTL

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 kv2019i self-assigned this Apr 22, 2024
@kv2019i
Copy link
Contributor Author

kv2019i commented Apr 22, 2024

@kv2019i
Copy link
Contributor Author

kv2019i commented Apr 22, 2024

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
Copy link
Contributor

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

kv2019i referenced this issue in kv2019i/sof Apr 22, 2024
Commit c77a4fe ("topology2: pcm_caps: Remove defaults for
rate_min/rate_max") changed how rate constraints are described in
topology. After this change, the rate_min/max was ignored by SOF Linux
driver and the rate was incorrectly limited to 48000Hz for these
topologies.

Fix this issue by enumerating the supported sampling rates with "rates".

Link: https://github.com/thesofproject/sof/issues/9067
Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i
Copy link
Contributor Author

kv2019i commented Apr 22, 2024

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

There remains an issue with sof-test #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
Copy link
Contributor Author

kv2019i commented Apr 22, 2024

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
Copy link
Contributor

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

@kv2019i kv2019i added the P1 Blocker bugs or important features label Apr 23, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 23, 2024

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

lgirdwood referenced this issue in thesofproject/sof Apr 26, 2024
Commit c77a4fe ("topology2: pcm_caps: Remove defaults for
rate_min/rate_max") changed how rate constraints are described in
topology. After this change, the rate_min/max was ignored by SOF Linux
driver and the rate was incorrectly limited to 48000Hz for these
topologies.

Fix this issue by enumerating the supported sampling rates with "rates".

Link: https://github.com/thesofproject/sof/issues/9067
Signed-off-by: Kai Vehmanen <[email protected]>
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 26, 2024

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

@marc-hb marc-hb changed the title [BUG] regression with Intel bt-offload tplgs [BUG] regression with Intel bluetooth bt-offload tplgs Apr 26, 2024
@kv2019i
Copy link
Contributor Author

kv2019i commented Apr 29, 2024

@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
Copy link
Collaborator

marc-hb commented Apr 29, 2024

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:

thesofproject/sof#9068 (comment)

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
Copy link
Contributor Author

kv2019i commented Apr 30, 2024

@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 marc-hb transferred this issue from thesofproject/sof Apr 30, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 30, 2024

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

OK but how? In pseudo-code.

cc: @yongzhi1

@marc-hb marc-hb changed the title [BUG] regression with Intel bluetooth bt-offload tplgs bluetooth offload: Unsupported audio format: 48000Hz, 16bit, 1 channels Apr 30, 2024
@marc-hb marc-hb changed the title bluetooth offload: Unsupported audio format: 48000Hz, 16bit, 1 channels bluetooth offload: sof_ipc4_init_input_audio_fmt: Unsupported audio format: 48000Hz, 16bit, 1 channels Apr 30, 2024
@ujfalusi
Copy link
Contributor

ujfalusi commented May 2, 2024

@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
Copy link
Contributor Author

kv2019i commented May 2, 2024

@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
Copy link
Contributor

ujfalusi commented May 2, 2024

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
Copy link
Collaborator

marc-hb commented May 2, 2024

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
Copy link
Collaborator

marc-hb commented May 3, 2024

  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
Copy link
Contributor

ranj063 commented May 3, 2024

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
Copy link
Contributor Author

kv2019i commented May 6, 2024

@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
Copy link
Contributor

ujfalusi commented May 6, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Blocker bugs or important features type:test coverage gap This requires a new test case, not just fixing one
Projects
None yet
Development

No branches or pull requests

4 participants