sof
sof copied to clipboard
mux: implement ipc4 support
mux module does not support ipc4
A change is needed to work properly: https://github.com/thesofproject/rimage/pull/112
@fkwasowi is it so that copier or mixinmixout can't do the operations the mux/demux is doing? Anyway good to have this, but we need to get the "full story" how these parameters are coming from the user space via topology and kernel to firmware. As I understand current mux/demux is a "process component" with binary blob interface -> so it should be ipc agnostic? so why do we need a new interface for it?
@fkwasowi For the rimage dependency, please add a separate commit to this PR that updates sof/west.yml to point to the rimage repository where the dependent patch is merged. Please contact me offline if you need help. Other option is to do a separate PR to update the west.yml, and merge that first and then this one. Both are good to me.
@fkwasowi is it so that copier or mixinmixout can't do the operations the mux/demux is doing? Anyway good to have this, but we need to get the "full story" how these parameters are coming from the user space via topology and kernel to firmware. As I understand current mux/demux is a "process component" with binary blob interface -> so it should be ipc agnostic? so why do we need a new interface for it?
Modul copier and mixin/mixout at the moment do not have the ability to operate in the same way as Mux. It is possible that in the future the copier will have such functionality, but at the moment it does not. IPC3 and IPC4 significantly differ in config, so there was a need to redesign the module to support IPC4.
@fkwasowi For the rimage dependency, please add a separate commit to this PR that updates sof/west.yml to point to the rimage repository where the dependent patch is merged. Please contact me offline if you need help. Other option is to do a separate PR to update the west.yml, and merge that first and then this one. Both are good to me.
A separate PR has been created: https://github.com/thesofproject/rimage/pull/110, which is already merged
Thanks, the comment in build_config() is very good. Only one very minor nit left, please see inline, otherwise seems good to go.
Corrected
@fkwasowi wrote:
A separate PR has been created: thesofproject/rimage#110, which is already merged
The rimage changes are not automatically visible in SOF builds. Rather if a newer rimage (or Zephyr) is needed, one needs to submit a SOF PR to update sof/west.yml and change the rimage (or Zephyr) point to a more recent commit-id. All our CI infra uses west.yml to pick which versions to use. I.e. you need to add a separate commit to this PR to update rimage in west.yml, or otherwise the IPC4 tests will fail.
Starts to be ready to go. You do need to add an update to west.yml in a seprate commti (see #6396 for an example) and have that as a separate git commit in this PR. A couple of minor thing, including one bad naming I proposed (sorry about that).
Done (I have added a separate commit to west.yml)
SOFCI TEST
@fkwasowi The rimage updates are quite tricky to do. Let's do so that I'll do the rimage update in seperate PR https://github.com/thesofproject/sof/pull/6429 and you can then rebase your mux PR on top of that.
Issues that needed addressing:
- the rimage update must be first in a series (other git bisect will be broken)
- the commit that updates rimage commit id in west.yml, must also update the git submodule "rimage" (this is very confusing and is caused by some SOF targets not using west manifest)
- the commit-id you put in west.yml must be a commit-id of a merge commit (e9e074c3c3e8b7f97a3f07bc1eab2d2fdabcf9b6 is not a merged commit in rimage main branch, but rather points to prv-fkwasowi_modules_types_mtl )
Thanks for updating the rimage submodule as well, however there are still many serious rimage problems:
- In your last force-push https://github.com/thesofproject/sof/commits/7ac73be3a070986a, the commit message subject says "upgrade rimage to f1e5621067bb" but the commit message body says "upgrade rimage to e9e074c3c3e8b7f". At least one of them is wrong (or both)
- In fact both are wrong because one of them is in
west.yml
but you upgraded the submodule to a third, different rimage commit d957e0368b86 instead!? As instructed in the new comments I added towest.yml
,git status
andgit describe --dirty
should report the submodule / west mismatch. Didn't they? Always look at "git status" before pushing (even when a single git repo is involved)- The latest rimage commit is currently yet another rimage commit: 3ee717eebc6a , why not upgrade to the most recent rimage commit? Is there something wrong with it? Maybe because we all love dealing with rimage upgrades so much, save another rimage upgrade for later? :-D
- In your last force-push https://github.com/thesofproject/sof/commits/7ac73be3a070986a, your west.yml change is the LAST commit of the three. You're upgrading rimage because your other commits need that rimage upgrade, correct? If correct then your rimage upgrade commit must be first. It cannot be last because every commit must be usable for
git bisect
and other test reasons.- Wait, actually you upgraded the rimage git submodule and west.yml in... different commits! They must be kept in sync in every commit.
- You missed this log --oneline instruction below in the west.yml file. It's important because everyone regularly gets lost and confused with module updates - including you, see all the examples above.
# When upgrading projects here please run git log --oneline in the # project and if not too long then include the output in your commit # message. See examples of good commit messages at # https://github.com/thesofproject/sof/commits/4e1d3ba61abd3/rimage # (git log 4e1d3ba61abd3 -- rimage/)
Make sure you find a good git plugin in your editor (or a good standalone git graphical interface), one that lets you easily review all these changes before submitting. Never restrict yourself to the limited git command line, always use all the available git options; especially when ugprading a dependency.
I swapped the order of commits, changed the commit id to merge id, and merged two commits into one (update rimage submodules together with the change in west)-is it okay now?
Fine to me. @fkwasowi If you compare to https://github.com/thesofproject/sof/pull/6429 , the rimage history is missing from commit description, but this is not a mandatory part (and is missing from some earlier updates as well). I'm good with this version as well. We can merge this directly when Marc approves.
SOFCI TEST
Fine to me. @fkwasowi If you compare to #6429 , the rimage history is missing from commit description, but this is not a mandatory part (and is missing from some earlier updates as well). I'm good with this version as well. We can merge this directly when Marc approves.
Commit message changed
Fails to compile TGL in https://sof-ci.01.org/sof-pr-viewer/#/build/PR6385/build10613048
/localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-xcc -DBLD_COUNTERS=0 -DRELATIVE_FILE=\"src/audio/copier/copier.c\" -I/quickbuild/workspace1/24733/SOF_FW/src/platform/intel/cavs/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/xtos -I/quickbuild/workspace1/24733/SOF_FW/xtos/include -I/quickbuild/workspace1/24733/SOF_FW/src/platform/tigerlake/include -I/localdisk/tools/xtensa/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8/xtensa-elf/arch/include -I/localdisk/tools/xtensa/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8/xtensa-elf/include -I/quickbuild/workspace1/24733/SOF_FW/rimage/src/include -I/quickbuild/workspace1/24733/SOF_FW/src/include -I/quickbuild/workspace1/24733/SOF_FW/build_fw/generated/include -I/quickbuild/workspace1/24733/SOF_FW/third_party_include -nostdlib -fno-inline-functions -mlongcalls -O2 -g -Wall -Werror -Wl,-EL -Wmissing-prototypes -Wpointer-arith -mtext-section-literals -imacros/quickbuild/workspace1/24733/SOF_FW/build_fw/generated/include/autoconfig.h -MD -MT CMakeFiles/sof.dir/src/audio/copier/copier.c.o -MF CMakeFiles/sof.dir/src/audio/copier/copier.c.o.d -o CMakeFiles/sof.dir/src/audio/copier/copier.c.o -c /quickbuild/workspace1/24733/SOF_FW/src/audio/copier/copier.c /quickbuild/workspace1/24733/SOF_FW/src/audio/copier/copier.c: In function ‘copier_new’: /quickbuild/workspace1/24733/SOF_FW/src/audio/copier/copier.c:463: error: ‘for’ loop initial declaration used outside C99 mode /quickbuild/workspace1/24733/SOF_FW/src/audio/copier/copier.c: In function ‘copier_prepare’: /quickbuild/workspace1/24733/SOF_FW/src/audio/copier/copier.c:622: error: ‘for’ loop initial declaration used outside C99 mode CMakeFiles/sof.dir/build.make:1037: recipe for target 'CMakeFiles/sof.dir/src/audio/copier/copier.c.o' failed make[3]: *** [CMakeFiles/sof.dir/src/audio/copier/copier.c.o] Error 2
Fixed
So this looks a CI misconfiguration problem. CI should never compile anything with "secret flags" or secret options / configurations that developers cannot reproduce easily, all the entire build configurations MUST be available in version control. No CI "secret sauce" ever.
Fixed
What was fixed, is the CI secret sauce gone? It looks like you just changed the code to avoid the issue.
Fails to compile TGL in https://sof-ci.01.org/sof-pr-viewer/#/build/PR6385/build10613048
Apologies, this was not a Zephyr build. Thanks @mwasko for the tip.
SOF CI
@kv2019i , You recently released a change with the same failing step (https://github.com/thesofproject/sof/pull/6383), "Known fail in IPC4 test set, proceeding with merge."-could I also ask for such a merge? Because I do not know how to pass the step sof-ci. I have the same problem as Tomasz Lissowski had.
I merged @kv2019i 's hard work in https://github.com/thesofproject/sof-test/pull/971 so https://sof-ci.01.org/sofpr/PR6385/build2000/devicetest is all green. No need to ask for any exception now