You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
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.
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.
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:
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?
@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.
@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.
@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.
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.
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.
discover "real" capabilities by banging the ALSA APIs.
Trying to summarize a quick call with @ranj063 (thanks!), we have 4 more possible solutions:
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.
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)
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 */
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).
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
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.
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?
Activity
kv2019i commentedon Apr 22, 2024
FYI @marc-hb @ranj063 @ujfalusi @plbossart
kv2019i commentedon 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 commentedon Apr 22, 2024
@kv2019i, what is in dmesg? What are not finding?
topology2: intel: bt-generic.conf: fix rate constraints
kv2019i commentedon 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 commentedon 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:
ujfalusi commentedon Apr 22, 2024
I wonder if the same issue present with other setups:
marc-hb commentedon Apr 23, 2024
As expected, all MTLP_RVP_NOCODEC-bluetooth-offload tests still failing in today's daily 40182
topology2: intel: bt-generic.conf: fix rate constraints
marc-hb commentedon 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".
[-][BUG] regression with Intel bt-offload tplgs[/-][+][BUG] regression with Intel bluetooth bt-offload tplgs[/+]kv2019i commentedon Apr 29, 2024
@marc-hb wrote:
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:
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 commentedon Apr 29, 2024
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)
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 commentedon 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 commentedon Apr 30, 2024
OK but how? In pseudo-code.
cc: @yongzhi1
[-][BUG] regression with Intel bluetooth bt-offload tplgs[/-][+]bluetooth offload: Unsupported audio format: 48000Hz, 16bit, 1 channels[/+][-]bluetooth offload: Unsupported audio format: 48000Hz, 16bit, 1 channels[/-][+]bluetooth offload: sof_ipc4_init_input_audio_fmt: Unsupported audio format: 48000Hz, 16bit, 1 channels[/+]ujfalusi commentedon 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 commentedon May 2, 2024
@marc-hb I have no easy answer. Something along the lines of:
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 commentedon 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 commentedon May 2, 2024
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 commentedon May 3, 2024
Trying to summarize a quick call with @ranj063 (thanks!), we have 4 more possible solutions:
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.
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)
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 */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 commentedon May 3, 2024
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 commentedon May 6, 2024
@ranj063 wrote:
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 commentedon 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?