Revert "topologies: switch all .m4 files to codec_provider and codec_consumer#5192
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I spent more time looking at the recursive diffoscope output (awesome tool, use it). What happens is very simple: Example: ├── sof-acp.conf
@@ -844,9 +844,9 @@
format "I2S"
mclk "codec_mclk_in"
mclk_freq "49152000"
- bclk "codec_consumer"
+ bclk "codec_slave"
bclk_freq "3072000"
- fsync "codec_consumer"
+ fsync "codec_slave"
fsync_freq "48000"
tdm_slots "2"
tdm_slot_width "32"
├── sof-acp.tplg
000015d0 00 00 00 00 00 00 00 78 00 00 00 00 00 00 00 01 |.......x........|
-000015e0 00 00 00 00 00 00 01 01 01 00 00 00 00 ee 02 00 |................|
+000015e0 00 00 00 00 00 00 00 00 01 00 00 00 00 ee 02 00 |................|
000015f0 e0 2e 00 80 bb 00 00 02 00 00 00 20 00 00 00 03 |........... ....|
00001600 00 00 00 03 00 00 00 00 00 00 00 00 00 00 00 00 |................|
00001610 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
All topologies show the same difference. |
|
Usual |
aiChaoSONG
left a comment
There was a problem hiding this comment.
The inclusive words codec_consumer and codec_provider are added after the alsa-lib 1.2.2 by Pierre. With #5162, the alsa-lib 1.2.2 cannot recognize the inclusive words, so the code goes to the ELSE part and will assign wrong value to bclk_provider(was bclk_master) and fsync(was fsync_master).
#5162 should be merged into the code once alsa-lib 1.2.2 becomes deprecated and no one uses it, but not now.
|
Thanks @aiChaoSONG ! So every alsatplg version before https://github.com/alsa-project/alsa-lib/commits/706192341d1d0bbb906 will silently corrupt the .tplg output, correct? If yes did you find (maybe with The alternative to this revert is to (loudly) fail the build and force developers to upgrade ALSA to the minimum version required. @ranj063 recently added such a version check for topology2, we could easily add another one. |
|
@marc-hb It's alsa-lib 1.2.5 |
So I think 1.2.5 is too recent and we should merge this revert. While Fedora is tracking ALSA very closely thanks to @perexg , other distributions are lagging behind The latest Ubuntu 21.10 is still on 1.2.4 (the next one will have at least 1.2.5) The latest Debian release is less than one month old and it still has 1.2.4 https://build.opensuse.org/request/show/899947 upgrade to 1.2.5 in June 2021, dunno which release that maps to. (1.2.5 was apparently tagged in May 2021) |
|
It not easier to check the alsatplg version before the call (in the build system) rather than to revert the whole change? Also, the code should be improved to fail when an unknown value is set. |
Considered above.
Which code? I don't think any distro would make any emergency 1.2.4.1 ALSA upgrade just for this. This rename is simply too early, that's all. When made this rename I realized the corresponding ALSA change was only 1 year old. However:
It's unfortunate the deprecation warning was added at the same time than the feature without any transition period. |
It looks like a bit bad workflow if you don't run your tests against the latest alsa-utils (alsatplg). The code is in upstream really long time enough for the verification and you can do the verification right now. Alternate way is to replace the clock strings in the configuration files back for the older alsatplg binaries (use sed in the build system or so).
The upstream code to avoid such situations in future. |
|
Note - I believe that the generated ,tplg binaries between alsatplg 1.2.2 and recent should be identical (so the verification can compare the .tlpg binaries only - no hw tests are required). It should be a quick job. |
|
@ranj063 can you confirm the topology2 fails for unknown values ? |
I think we're not really using topology2 anywhere yet.
Validation is using ALSA 1.2.6 now, this revert is for developers.
Yes it's "bad" - does it have to be "punished" with silent .tplg corruption and very obscure errors? Do we think we have too many SOF developers, is this how we want to welcome new ones? I was lucky to identify this issue in less than a couple hours only because I rebase every day and could bisect quickly. I also made the change in the first place. Having multiple library versions installed concurrently on Linux is a real pain, if the distro one "seems" to work it's not surprising people stick to it.
+1 and thanks for the other ideas. |
In this case, the developers should be informed to use the new ALSA release (and the build system should fail). It's quite easy to develop things using docker/podman, if they want to use the distro without regular upstream updates. Only my two cents. I won't comment this more. |
greg-intel
left a comment
There was a problem hiding this comment.
I approved the original, and am disappointed that this didn't work out.
The warnings are very noisy.
|
Good developer experience is entirely about removing as much friction as possible.
Compiling gcc toolchains is also a huge amount of friction but only once. Docker is relatively easy to use but it's not frictionless either. For instance: In fact in related #5202 I asked some (experienced) developers to test with https://thesofproject.github.io/latest/getting_started/build-guide/build-with-docker.html#build-firmware-binaries-with-docker and even when these instructions are very short, those who have never used docker yet were... not interested! Friction. https://www.google.com/search?q=friction+habits |
Actually, even Fedora 35 does not work out of the box. It's supposed to work: Yet the So What a poor experience. I found this suspicious bit https://github.com/alsa-project/alsa-utils/blob/40202a522aa86599bffaabf8097bf9bf1cef4f61/topology/topology.c#L59 "-o, --output=FILE set output file\n"
#if SND_LIB_VER(1, 2, 5) < SND_LIB_VERSION
"-D, --define=ARGS define variables (VAR1=VAL1[,VAR2=VAL2] ...)\n"
"-I, --inc-dir=DIR set include path\n"
#endif
"-s, --sort sort the identifiers in the normalized output\n"(funny how the default branch names at https://github.com/alsa-project and https://git.alsa-project.org/ are still "master") |
…consumer" This reverts commit f50b6fe. I discovered the hard way that this change causes alsatplg version 1.2.2 (the default version in the current LTS Ubuntu) to corrupt just a few bytes in .tplg output in an incredibly discrete and time-consuming way: no error message at build time and same file size. See example at thesofproject#5162. We could/should just require a minimum alsatplg version at the CMake level but at this moment we don't even know which minimum version is needed and we would also need to take some time to test a few alsatplg versions. If version 1.2.2 would just fail with an decent error message that can be searched and discussed then everything would be fine but silent corruption is really not OK. So users of recent versions will unfortunately have to live with the huge number of warnings for now. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
606b2ea to
9bed77f
Compare
|
https://sof-ci.01.org/sofpr/PR5192/build12056/devicetest/?model=APL_UP2_NOCODEC_ZEPHYR&testcase=multiple-pause-resume-5 looks like unrelated #5257, everything else passed. |
We'll use this new function to require ALSA 1.2.5 and finally rename to "codec_consumer" (see revert discussion thesofproject#5192) Signed-off-by: Marc Herbert <marc.herbert@intel.com>
We'll use this new function to require ALSA 1.2.5 and finally rename to "codec_consumer" (see revert discussion thesofproject#5192) Signed-off-by: Marc Herbert <marc.herbert@intel.com>
We'll use this new function to require ALSA 1.2.5 and finally rename to "codec_consumer" (see revert discussion #5192) Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Note another, unrelated and much more recent silent corruption with |
This reverts commit f50b6fe.
I discovered the hard way that this change causes alsatplg version
1.2.2 (the default version in the current LTS Ubuntu) to corrupt just
a few bytes in .tplg output in an incredibly discrete and
time-consuming way: no error message at build time and same file size.
See example at #5162.
We could/should just require a minimum alsatplg version at the CMake
level but at this moment we don't even know which minimum version is
needed and we would also need to take some time to test a few alsatplg
versions. If version 1.2.2 would just fail with an decent error
message that can be searched and discussed then everything would be
fine but silent corruption is really not OK.
So users of recent versions will unfortunately have to live with the
huge number of warnings for now.
Signed-off-by: Marc Herbert marc.herbert@intel.com