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

audio: xtensa: add Kconfig for inline functions #8033

Merged
merged 4 commits into from
Aug 18, 2023
Merged

audio: xtensa: add Kconfig for inline functions #8033

merged 4 commits into from
Aug 18, 2023

Conversation

btian1
Copy link
Contributor

@btian1 btian1 commented Aug 14, 2023 •
edited
Loading

Enable inline function for cavs 2.5 and above platform, this is a follow up with issue:#5212

@btian1 btian1 mentioned this pull request Aug 14, 2023
Closed
lyakh
lyakh reviewed Aug 15, 2023
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

Nice! Any size / performance comparison data yet?

zephyr/Kconfig Outdated Show resolved Hide resolved
btian1
btian1 commented Aug 15, 2023
View reviewed changes
Copy link
Contributor Author

@btian1 btian1 left a comment

Choose a reason for hiding this comment

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

please check #5212 , I pasted data for the compare.

zephyr/Kconfig Outdated Show resolved Hide resolved
@btian1 btian1 marked this pull request as ready for review August 16, 2023 01:28
@lyakh
Copy link
Collaborator

lyakh commented Aug 16, 2023

please check #5212 , I pasted data for the compare.

interesting... So mixout performance improves by 10.5% with inlining, mixin by 3.5%, whereas gain becomes 3.4% slower (is 0x60000 vs. 0x60001 - playback vs. capture?) While size increased by about 0.2%. Looks like a clear win to me! :-)

iuliana-prodan
iuliana-prodan reviewed Aug 16, 2023
View reviewed changes
zephyr/Kconfig Outdated Show resolved Hide resolved
@lgirdwood
Copy link
Member

@btian1 re-running CI, not expecting a logic error here but could be timing.
https://sof-ci.01.org/sofpr/PR8033/build11621/devicetest/index.html?model=TGLU_RVP_NOCODEC_IPC4ZPH&testcase=check-capture-10sec

@lgirdwood
Copy link
Member

SOFCI TEST

kv2019i
kv2019i requested changes Aug 16, 2023
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.

Excellent data on the impact. I have some comments on the naming and documentation, see inline.

src/arch/xtensa/Kconfig Outdated Show resolved Hide resolved
src/arch/xtensa/CMakeLists.txt Show resolved Hide resolved
@lgirdwood
Copy link
Member

@btian1 re-running CI, not expecting a logic error here but could be timing. https://sof-ci.01.org/sofpr/PR8033/build11621/devicetest/index.html?model=TGLU_RVP_NOCODEC_IPC4ZPH&testcase=check-capture-10sec

ok, looks good now.

@btian1 btian1 mentioned this pull request Aug 17, 2023
Merged
@btian1
Copy link
Contributor Author

btian1 commented Aug 17, 2023

SOFCI TEST

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 17, 2023

interesting... So mixout performance improves by 10.5% with inlining, mixin by 3.5%, whereas gain becomes 3.4% slower (is 0x60000 vs. 0x60001 - playback vs. capture?) While size increased by about 0.2%. Looks like a clear win to me! :-)

Can you share the tests with us? How did you measured the impact of the patch at runtime.

We would like to check this on imx. Also, what toolchain have you used? GCC or XCC. Zephyr or XTOS?

kv2019i
kv2019i requested changes Aug 17, 2023
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.

Few more nits still, the descsription could be even more accurate/concrete and I'd like the Kconfig definition to be done only once in the tree.

src/arch/xtensa/CMakeLists.txt Show resolved Hide resolved
src/arch/xtensa/Kconfig Outdated Show resolved Hide resolved
zephyr/Kconfig Outdated Show resolved Hide resolved
btian1 added 4 commits August 18, 2023 10:21
@btian1
audio: add a config to sof and zephyr code
4a3d281 
this config will be used to add or remove -fno-inline-functions
into compiler build option.

Signed-off-by: Baofeng Tian <[email protected]>
@btian1
audio: sof: remove -fno-inline-functions option for cavs2.5 and ace
5c970df 
remove this build option for sof build cavs2.5 and ace platform
with enable COMPILER_INLINE_FUNCTION_OPTION.

Signed-off-by: Baofeng Tian <[email protected]>
@btian1
audio: zephyr: add -fno-inline-functions option to zephyr code
c12224d 
By enable/disable COMPILER_INLINE_FUNCTION_OPTION to control
add or remove -fno-inline-functions build option for sof zephyr
code.

Signed-off-by: Baofeng Tian <[email protected]>
@btian1
audio: dai: fix the error reported by CI after enable inline
7780e4a 
after enable inline build option, CI build is reporting not
initialize this structure, init it to remove the error.

Signed-off-by: Baofeng Tian <[email protected]>
@btian1
Copy link
Contributor Author

btian1 commented Aug 18, 2023

interesting... So mixout performance improves by 10.5% with inlining, mixin by 3.5%, whereas gain becomes 3.4% slower (is 0x60000 vs. 0x60001 - playback vs. capture?) While size increased by about 0.2%. Looks like a clear win to me! :-)

Can you share the tests with us? How did you measured the impact of the patch at runtime.

We would like to check this on imx. Also, what toolchain have you used? GCC or XCC. Zephyr or XTOS?

please add -o app/perf_overlay.conf into fw build, then you will get trace logs with component cycle performance.

@btian1
Copy link
Contributor Author

btian1 commented Aug 18, 2023

SOFCI TEST

kv2019i
kv2019i approved these changes Aug 18, 2023
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 @btian1, looks good now! Great to have this long-open issue closed.

@kv2019i kv2019i merged commit 897f516 into thesofproject:main Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgirdwood lgirdwood lgirdwood approved these changes

@lyakh lyakh lyakh approved these changes

@iuliana-prodan iuliana-prodan iuliana-prodan approved these changes

@kv2019i kv2019i kv2019i approved these changes

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

@aborisovich aborisovich Awaiting requested review from aborisovich

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

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

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

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

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

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

Successfully merging this pull request may close these issues.
6 participants
@btian1 @lyakh @lgirdwood @dbaluta @kv2019i @iuliana-prodan