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.

Sign up for GitHub

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

Jump to bottom

mux: implement ipc4 support #6385

Merged
merged 3 commits into from
Oct 20, 2022
Merged

mux: implement ipc4 support #6385

merged 3 commits into from
Oct 20, 2022

Conversation

fkwasowi
Copy link
Contributor

@fkwasowi fkwasowi commented Oct 10, 2022 •
edited
Loading

mux module does not support ipc4

A change is needed to work properly: thesofproject/rimage#112

lgirdwood
lgirdwood reviewed Oct 10, 2022
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of complex looking code in here, can you provide more inline comments on each block to help reviewers. Thanks.

kv2019i
kv2019i requested changes Oct 10, 2022
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fkwasowi ! A few comments inline, please see.

src/audio/mux/mux.c Show resolved Hide resolved
src/audio/mux/mux.c Show resolved Hide resolved
src/audio/mux/mux.c Show resolved Hide resolved
src/audio/mux/mux.c Outdated Show resolved Hide resolved
src/audio/mux/mux.c Show resolved Hide resolved
src/audio/mux/mux.c Show resolved Hide resolved
src/audio/mux/mux.c Outdated Show resolved Hide resolved
src/audio/mux/mux.c Outdated Show resolved Hide resolved
src/audio/mux/mux.c Show resolved Hide resolved
src/audio/mux/mux.c Show resolved Hide resolved
@fkwasowi fkwasowi force-pushed the mux_module_ipc4_support branch 3 times, most recently from 020c9d1 to a472329 Compare October 10, 2022 13:06
@juimonen
Copy link

juimonen commented Oct 10, 2022 •
edited
Loading

@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?

aiChaoSONG
aiChaoSONG reviewed Oct 11, 2022
View reviewed changes
src/audio/mux/mux.c Outdated Show resolved Hide resolved
aiChaoSONG
aiChaoSONG reviewed Oct 11, 2022
View reviewed changes
src/audio/mux/mux.c Outdated Show resolved Hide resolved
@fkwasowi fkwasowi force-pushed the mux_module_ipc4_support branch 3 times, most recently from a523547 to 1daad89 Compare October 11, 2022 09:35
kv2019i
kv2019i requested changes Oct 11, 2022
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the comment in build_config() is very good. Only one very minor nit left, please see inline, otherwise seems good to go.

src/audio/mux/mux.c Outdated Show resolved Hide resolved
src/audio/mux/mux.c Outdated Show resolved Hide resolved
@fkwasowi
Copy link
Contributor Author

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

marc-hb
marc-hb previously requested changes Oct 18, 2022
View reviewed changes
Copy link
Collaborator

@marc-hb marc-hb left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TGL compiles just fine in https://sof-ci.01.org/sofpr/PR6385/build1949/build/tgl_zph.txt with xcc/install/builds/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8 and (hopefully) in your workspace too? What toolchain did you use?

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.

Whatever the root cause is, we can't merge code that does not compile for whatever reason.

@fkwasowi fkwasowi force-pushed the mux_module_ipc4_support branch 2 times, most recently from a51e0b7 to 00bdacb Compare October 19, 2022 09:57
@fkwasowi
Copy link
Contributor Author

fkwasowi commented Oct 19, 2022 •
edited
Loading

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

@kv2019i kv2019i mentioned this pull request Oct 19, 2022
Closed
@fkwasowi fkwasowi requested a review from marc-hb October 19, 2022 11:25
@marc-hb
Copy link
Collaborator

marc-hb commented Oct 19, 2022 •
edited
Loading

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.

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 20, 2022

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.

@marc-hb marc-hb dismissed their stale review October 20, 2022 01:16

Now compiles

@fkwasowi
west.yml: upgrade rimage to 3ee717e
f5212f8 
upgrade rimage to
3ee717e

3ee717e probe: mtl.toml invaliv probe type
1f4a36e mux: fix module type
dcfd11b mux: add mux cfg to list of modules
d957e03 rimage: make ace15 signing to support openssl3
a1b6e6d manifest: add fw_ver_micro to manifest
fb28357 config: mtl: add probe module

Signed-off-by: Kwasowiec, Fabiola <[email protected]>
@fkwasowi
mux: implement ipc4 support
59c54fa 
mux module does not support ipc4

Signed-off-by: "Kwasowiec, Fabiola" <[email protected]>
@fkwasowi
copier: set conversion function for all pins
f7c0b29 
conversion function and out_fmt
is set only for first pin

Signed-off-by: "Kwasowiec, Fabiola" <[email protected]>
@aborisovich
Copy link
Contributor

SOF CI

@fkwasowi
Copy link
Contributor Author

@kv2019i , You recently released a change with the same failing step (#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.

@mwasko mwasko merged commit 8083eb0 into thesofproject:main Oct 20, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Oct 20, 2022

I merged @kv2019i 's hard work in thesofproject/sof-test#971 so https://sof-ci.01.org/sofpr/PR6385/build2000/devicetest is all green. No need to ask for any exception now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh left review comments

@kv2019i kv2019i kv2019i approved these changes

@RanderWang RanderWang RanderWang approved these changes

@mwasko mwasko mwasko approved these changes

@akloniex akloniex Awaiting requested review from akloniex

@ranj063 ranj063 Awaiting requested review from ranj063

@juimonen juimonen Awaiting requested review from juimonen

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski

@lgirdwood lgirdwood Awaiting requested review from lgirdwood lgirdwood is a code owner

@aiChaoSONG aiChaoSONG Awaiting requested review from aiChaoSONG

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@aborisovich aborisovich Awaiting requested review from aborisovich

@bardliao bardliao Awaiting requested review from bardliao

@bhiregoudar bhiregoudar Awaiting requested review from bhiregoudar

@sunilkumardommati sunilkumardommati Awaiting requested review from sunilkumardommati

@marc-hb marc-hb Awaiting requested review from marc-hb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.