zephyr/cmake: move math/*.c files to now common math/CMakeLists.txt #8548
Conversation
In the review of commit 1bd9e0d ("cmake/zephyr: decentralize src/ipc/"), Andy recommended "maybe work around this by adding yet another layer of indirection". I rejected the idea at the time because the level of duplication in the ipc/ directory was small. Then I looked at the bigger math/ subdirectory and I realized in thesofproject#8548 that such an indirection layer is actually required for bigger and/or more complex cases. So I added that layer of indirection in commit 330d73e ("cmake: a few new add_local_sources[_ifdef]() compatibility macros") Now that we have it, we might as well use it and perform small simplifications in ipc/cmake. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
requested review from
ShriramShastry,
andrula-song,
andyross,
dbaluta,
kv2019i,
lgirdwood and
softwarecki
|
SOFCI TEST |
|
Only 1 suspend/resume timeout in https://sof-ci.01.org/sofpr/PR8548/build585/devicetest/index.html https://sof-ci.01.org/sof-pr-viewer/#/build/PR8548/build13196824 all green New i915 GSC firmware needed in https://sof-ci.01.org/sofpr/PR8548/build584/devicetest/index.html, everything else green. |
| add_local_sources(sof numbers.c) | ||
|
|
||
| if(CONFIG_CORDIC_FIXED) | ||
| if(CONFIG_CORDIC_FIXED OR it_is_zephyr) |
There was a problem hiding this comment.
Btw, do we have a CONFIG_ZEPHYR we can use here instead of it_is_zephyr ?
There was a problem hiding this comment.
is_zephyr is only a check for CONFIG_ZEPHYR_SOF_MODULE. The indirection is for brevity, flexibility and "future-proofness".
There was a problem hiding this comment.
why not just use CONFIG_ZEPHYR_SOF_MODULE if they are equivalent.
There was a problem hiding this comment.
They are equivalent now but who knows in the future. I'm currently spreading this conditional across almost all sof/**/CMakeLists.txt files, so this very thin indirection protects us against any future change. If this CONFIG_ namve ever changes in Zephyr in the future we could even make the macro support both the old and new for a seamless transition.
There was a problem hiding this comment.
If they are equivalent now then pls use, otherwise this is really just unneeded indirection today.
There was a problem hiding this comment.
Tend to agree that the existing kconfigs aren't super well named for this purpose. They're artifacts of the way the upstream Zephyr module system works: ZEPHYR_SOF_MODULE is automatically generated, and 'config SOF' is added in the Kconfig for the module. FWIW I NAK'd the latter because in this context it means the opposite of what it says ("CONFIG_SOF" might be expected to mean "xtos/traditional SOF and not Zephyr", when it means the opposite).
My personal vote would be a SOF-internal API that contains "zephyr" in the name like this one. Whether it's a kconfig or a cmake macro doesn't seem all that important I guess? Would be easy to fix in a different patch too.
There was a problem hiding this comment.
CONFIG_CORDIC OR CONFIG_ZEPHYR_SOMETHING_MODULE_OPTION_OF_THE_DAY would also give the wrong impression that both are choices, see discussion below. The second one is obviously not a CONFIGuration choice.
There was a problem hiding this comment.
We have a lot of naming inconsistent in SOF Kconfig space today, moving forward for v2.9 we can fix this with a CONFIG_SOF_ prefix i.e. its a SOF application Kconfig. For maths stuff we are going to be moving for v2.9 to
CONFIG_SOF_MATH_X
For Zephyr stuff, we need something CONFIG_SOF_ZEPHYR_X the precedence being this is a SOF Kconfig and NOT a Zephyr Kconfig.
@marc-hb can you incrementally change the is_zephyr to use the Kconfig, this way is easier for everyone to follow.
There was a problem hiding this comment.
For Zephyr stuff, we need something CONFIG_SOF_ZEPHYR_X the precedence being this is a SOF Kconfig and NOT a Zephyr Kconfig.
@marc-hb can you incrementally change the is_zephyr to use the Kconfig, this way is easier for everyone to follow.
You lost me sorry.
There was a problem hiding this comment.
I just found that some places started using CONFIG_ZEPHYR_NATIVE_DRIVERS...
ShriramShastry
left a comment
ShriramShastry
left a comment
There was a problem hiding this comment.
I've added my review comments. I welcome you making changes.
| add_local_sources(sof numbers.c) | ||
|
|
||
| if(CONFIG_CORDIC_FIXED) | ||
| if(CONFIG_CORDIC_FIXED OR it_is_zephyr) |
There was a problem hiding this comment.
Using the same name for managing math functions under Zephyr is not a good idea.
There was a problem hiding this comment.
I'm not sure I understand @ShriramShastry. The purpose of this PR is to be very careful and NOT make any config "management" change. That can come later after this clean up. Currently, maths function are NOT managed in Zephyr. Did you not notice?
There was a problem hiding this comment.
OK, What I mean is that it_is_zephyr is used to enable and disable math functions. If the function is called in the code, you enable it_is_zephyr, otherwise you turn it off. What I'm not persuaded of is the use of the same flag (it_is_zephyr) across Zephyr code . If you have a solid justification (That's Zephyr) for using the same name, that's fine; otherwise, I advocate using a distinct naming policy for each math function.
There was a problem hiding this comment.
that it_is_zephyr is used to enable and disable math functions
No, that's not what it is. No one is going to "turn Zephyr on/off" to enable/disable some math functions. No one can decide whether they use Zephyr or not, it's not a choice. There is no such choice.
Before this PR:
- CORDIC is not optional in Zephyr. No such config "management" in Zephyr.
After this PR:
- CORDIC is not optional in Zephyr. No such config "management" in Zephyr.
The purpose of this PR is to carefully NOT change that.
If you want make to make CORDIC or other math code CONFIGurable in Zephyr then please submit a separate PR. This PR is a pure clean-up, it does not make any CONFIG change.
| add_local_sources_ifdef(CONFIG_MATH_EXP sof exp_fcn.c exp_fcn_hifi.c) | ||
|
|
||
| if(CONFIG_MATH_DECIBELS) | ||
| if(CONFIG_MATH_DECIBELS OR it_is_zephyr) |
There was a problem hiding this comment.
Using the same name for managing math functions under Zephyr is not a good idea.
There was a problem hiding this comment.
my question of the day to these changes: I understand that it's just carried over from the present global sof/zephyr/CMakeLists.txt, but now that we move to these local files, shouldn't we rather just select MATH_DECIBELS somewhere with Zephyr?
There was a problem hiding this comment.
Yes, I am thinking about it, and it makes more sense to me to have a select option with Zephyr.
There was a problem hiding this comment.
shouldn't we rather just select MATH_DECIBELS somewhere with Zephyr?
Maybe yes but please, please do not distract this PR even further :-(
In the review of commit 1bd9e0d ("cmake/zephyr: decentralize src/ipc/"), Andy recommended "maybe work around this by adding yet another layer of indirection". I rejected the idea at the time because the level of duplication in the ipc/ directory was small. Then I looked at the bigger math/ subdirectory and I realized in thesofproject#8548 that such an indirection layer is actually required for bigger and/or more complex cases. So I added that layer of indirection in commit 330d73e ("cmake: a few new add_local_sources[_ifdef]() compatibility macros") Now that we have it, we might as well use it and perform small simplifications in ipc/cmake. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
There seems to be some confusion happening in the review comments right now.
Clarification: this CMake PR is BUG-FOR-BUG compatible.
There are Cmake and CONFIG bugs in zephyr/CMakeLists.txt right now that this #8260 effort is exposing. The purpose of this PR is to carefully NOT fix any of these bugs. The commits in this PR and all other 8260 commits in general are way too big to hide small functional changes inside. You really don't want your git bisect to land on a giant CONFIG_ commit like these. So this PR is strictly "bug-preserving". Garbage in, exact same garbage out.
If this PR made you discover some existing CMake or CONFIG bug, then please submit a smaller PR either before this PR is merged (and I will rebase and adjust), or after this PR. But please stop asking for bug fixes here.
I edited the description too.
|
Demoting to draft until @andrula-song 's bug fix is reviewed: |
CMake decentralization per thesofproject#8260 The purpose of thesofproject#8260 is to divide and conquer the giant zephyr/CMakeLists.txt file while staying _bug-for-bug compatible_. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
force-pushed
the
cmake-math-move
branch
from
205e202 to
2c760aa
Compare
|
Rebased on top of #8551. More comments added, thanks for the feedback! |
In the review of commit 1bd9e0d ("cmake/zephyr: decentralize src/ipc/"), Andy recommended "maybe work around this by adding yet another layer of indirection". I rejected the idea at the time because the level of duplication in the ipc/ directory was small. Then I looked at the bigger math/ subdirectory and I realized in #8548 that such an indirection layer is actually required for bigger and/or more complex cases. So I added that layer of indirection in commit 330d73e ("cmake: a few new add_local_sources[_ifdef]() compatibility macros") Now that we have it, we might as well use it and perform small simplifications in ipc/cmake. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
I volunteered to do this on my "spare time" but this is proving too time consuming, it needs proper planning. Maybe another time? |
8 participants
CMake decentralization per #8260
EDIT: There seems to be some confusion happening in the review comments right now.
Clarification: this CMake PR is BUG-FOR-BUG compatible.
There are Cmake and CONFIG bugs in zephyr/CMakeLists.txt right now that this #8260 effort is exposing. The purpose of this PR is to carefully NOT fix any of these bugs. The commits in this PR and all other 8260 commits in general are way too big to hide small functional changes inside. You really don't want your git bisect to land on a giant CONFIG_ commit like these. So this PR is strictly "bug-preserving". Garbage in, exact same garbage out.
If this PR made you discover some existing CMake or CONFIG bug, then please submit a smaller PR either before this PR is merged (and I will rebase and adjust), or after this PR. But please stop asking for bug fixes here.