Skip to content

zephyr/cmake: move math/*.c files to now common math/CMakeLists.txt #8548

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

Closed
marc-hb wants to merge 1 commit into from
Closed

zephyr/cmake: move math/*.c files to now common math/CMakeLists.txt #8548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/math/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,40 @@ if(BUILD_LIBRARY)
return()
endif()

is_zephyr(it_is_zephyr)

add_local_sources(sof numbers.c)

if(CONFIG_CORDIC_FIXED)
# Up to now, trig.c has never been optional in Zephyr.
# Maybe it should be in the future.
if(CONFIG_CORDIC_FIXED OR it_is_zephyr)
Copy link
Member

@lgirdwood lgirdwood Dec 4, 2023

Choose a reason for hiding this comment

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

Btw, do we have a CONFIG_ZEPHYR we can use here instead of it_is_zephyr ?

Copy link
Collaborator Author

@marc-hb marc-hb Dec 4, 2023

Choose a reason for hiding this comment

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

is_zephyr is only a check for CONFIG_ZEPHYR_SOF_MODULE. The indirection is for brevity, flexibility and "future-proofness".

Copy link
Member

@lgirdwood lgirdwood Dec 4, 2023

Choose a reason for hiding this comment

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

why not just use CONFIG_ZEPHYR_SOF_MODULE if they are equivalent.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 4, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lgirdwood lgirdwood Dec 4, 2023

Choose a reason for hiding this comment

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

If they are equivalent now then pls use, otherwise this is really just unneeded indirection today.

Copy link
Contributor

@andyross andyross Dec 4, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lgirdwood lgirdwood Dec 8, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 12, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 19, 2023

Choose a reason for hiding this comment

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

I just found that some places started using CONFIG_ZEPHYR_NATIVE_DRIVERS...

Copy link
Contributor

@ShriramShastry ShriramShastry Dec 5, 2023

Choose a reason for hiding this comment

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

Using the same name for managing math functions under Zephyr is not a good idea.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ShriramShastry ShriramShastry Dec 5, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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(sof trig.c)
endif()

add_local_sources_ifdef(CONFIG_SQRT_FIXED sof sqrt_int16.c)

add_local_sources_ifdef(CONFIG_MATH_EXP sof exp_fcn.c exp_fcn_hifi.c)

if(CONFIG_MATH_DECIBELS)
# Up to now, decibels.c has never been optional in Zephyr.
# Maybe it should be in the future.
if(CONFIG_MATH_DECIBELS OR it_is_zephyr)
Copy link
Contributor

@ShriramShastry ShriramShastry Dec 5, 2023

Choose a reason for hiding this comment

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

Using the same name for managing math functions under Zephyr is not a good idea.

Copy link
Collaborator

@lyakh lyakh Dec 6, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ShriramShastry ShriramShastry Dec 6, 2023

Choose a reason for hiding this comment

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

Yes, I am thinking about it, and it makes more sense to me to have a select option with Zephyr.

Copy link
Collaborator Author

@marc-hb marc-hb Dec 6, 2023

Choose a reason for hiding this comment

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

shouldn't we rather just select MATH_DECIBELS somewhere with Zephyr?

Maybe yes but please, please do not distract this PR even further :-(

add_local_sources(sof decibels.c)
endif()

if(NOT it_is_zephyr) # So far none of these has ever been enabled in Zephyr.
add_local_sources_ifdef(CONFIG_NATURAL_LOGARITHM_FIXED sof log_e.c)

add_local_sources_ifdef(CONFIG_COMMON_LOGARITHM_FIXED sof log_10.c)

add_local_sources_ifdef(CONFIG_POWER_FIXED sof power.c)

add_local_sources_ifdef(CONFIG_BINARY_LOGARITHM_FIXED sof base2log.c)
endif()

add_local_sources_ifdef(CONFIG_MATH_FIR sof fir_generic.c fir_hifi2ep.c fir_hifi3.c)

if(CONFIG_MATH_FFT)
# So far this directory has never been enabled in Zephyr.
if(CONFIG_MATH_FFT AND NOT it_is_zephyr)
add_subdirectory(fft)
endif()

Expand All @@ -39,6 +48,7 @@ add_local_sources_ifdef(CONFIG_MATH_IIR_DF2T sof
add_local_sources_ifdef(CONFIG_MATH_IIR_DF1 sof
iir_df1_generic.c iir_df1_hifi3.c iir_df1.c)

if(NOT it_is_zephyr) # So far none of these has ever been enabled in Zephyr.
if(CONFIG_MATH_WINDOW)
add_local_sources(sof window.c)
endif()
Expand All @@ -54,3 +64,4 @@ endif()
if(CONFIG_MATH_DCT)
add_local_sources(sof dct.c)
endif()
endif() # not Zephyr
34 changes: 1 addition & 33 deletions zephyr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ set(SOF_SAMPLES_PATH "${SOF_SRC_PATH}/samples")
set(SOF_LIB_PATH "${SOF_SRC_PATH}/lib")
set(SOF_DRIVERS_PATH "${SOF_SRC_PATH}/drivers")
set(SOF_DEBUG_PATH "${SOF_SRC_PATH}/debug")
set(SOF_MATH_PATH "${SOF_SRC_PATH}/math")
set(SOF_TRACE_PATH "${SOF_SRC_PATH}/trace")

set(RIMAGE_TOP ${sof_top_dir}/tools/rimage)
Expand Down Expand Up @@ -146,6 +145,7 @@ endmacro()

add_subdirectory(../src/init/ init_unused_install/)
add_subdirectory(../src/ipc/ ipc_unused_install/)
add_subdirectory(../src/math/ math_unused_install/)



Expand Down Expand Up @@ -366,11 +366,6 @@ zephyr_include_directories(${SOF_PLATFORM_PATH}/${PLATFORM}/include)
# Commented files will be added/removed as integration dictates.
zephyr_library_sources(

# SOF math utilities
${SOF_MATH_PATH}/decibels.c
${SOF_MATH_PATH}/numbers.c
${SOF_MATH_PATH}/trig.c

# SOF library - parts to transition to Zephyr over time
${SOF_LIB_PATH}/clk.c
${SOF_LIB_PATH}/notifier.c
Expand Down Expand Up @@ -485,24 +480,6 @@ elseif(CONFIG_IPC_MAJOR_4)
)
endif()

zephyr_library_sources_ifdef(CONFIG_MATH_FIR
${SOF_MATH_PATH}/fir_generic.c
${SOF_MATH_PATH}/fir_hifi2ep.c
${SOF_MATH_PATH}/fir_hifi3.c
)

zephyr_library_sources_ifdef(CONFIG_MATH_IIR_DF1
${SOF_MATH_PATH}/iir_df1_generic.c
${SOF_MATH_PATH}/iir_df1_hifi3.c
${SOF_MATH_PATH}/iir_df1.c
)

zephyr_library_sources_ifdef(CONFIG_MATH_IIR_DF2T
${SOF_MATH_PATH}/iir_df2t_generic.c
${SOF_MATH_PATH}/iir_df2t_hifi3.c
${SOF_MATH_PATH}/iir_df2t.c
)

zephyr_library_sources_ifdef(CONFIG_COMP_ASRC
${SOF_AUDIO_PATH}/asrc/asrc.c
${SOF_AUDIO_PATH}/asrc/asrc_farrow_hifi3.c
Expand Down Expand Up @@ -785,15 +762,6 @@ zephyr_library_sources_ifdef(CONFIG_COMP_TDFB
${SOF_AUDIO_PATH}/tdfb/tdfb_hifi3.c
)

zephyr_library_sources_ifdef(CONFIG_SQRT_FIXED
${SOF_MATH_PATH}/sqrt_int16.c
)

zephyr_library_sources_ifdef(CONFIG_MATH_EXP
${SOF_MATH_PATH}/exp_fcn.c
${SOF_MATH_PATH}/exp_fcn_hifi.c
)

zephyr_library_sources_ifdef(CONFIG_COMP_UP_DOWN_MIXER
${SOF_AUDIO_PATH}/up_down_mixer/up_down_mixer.c
${SOF_AUDIO_PATH}/up_down_mixer/up_down_mixer_hifi3.c
Expand Down