-
Notifications
You must be signed in to change notification settings - Fork 349
fix and re-enable Zephyr builds for apl with xcc #4852
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this Kconfig defaults for each platform ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood in principle we can, sure. But just a couple of day ago I had to remind myself that select statements in Kconfig aren't recursive. I was trying to reconfigure the kernel, changed an option, its directly selected dependencies got switched, but not the next level up. So, maybe it would be good to limit the use of select at least where it's easy to avoid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining that this is for memory usage reasons (and not for avoiding a bug or some other reason)
I find this more readable because it's closer to plain English:
if defined __XCC__ && ! (CONFIG_ZEPHYR_SOF_MODULE && CONFIG_APOLLOLAKE)as in: "do not do this for Zephyr on APL"
Don't ask me about Kconfig but there seems to be some good documentation at https://docs.zephyrproject.org/latest/guides/build/kconfig/tips.html#alternatives-to-select
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-hb ack, agree, updated
marc-hb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://sof-ci.01.org/sofpr/PR4852/build10649/build/apl_zph.txt build successful with XCC
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. If @singalsu can check the algo-version check. If there's a cleaner way, let'd do that, but if not, let's go with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singalsu Can you review this? Is there a cleaner way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no point in leaving out the the optimizations, the intrinsics code is ball park of 10x speed with same configuration. Instead Zephyr build should choose the tiny coefficients set from kconfig (CONFIG_COMP_SRC_TINY) but keep optimizations. @lyakh Do you have ideas how to accomplish it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@singalsu it's easy to enable CONFIG_COMP_SRC_TINY for Zephyr on APL, but then it would be used for both gcc and xcc. Don't think we can check for compiler in Kconfig. Or we can do something like
#if defined __XCC__
+#if CONFIG_APOLLOLAKE && CONFIG_ZEPHYR_SOF_MODULE
+#define SRC_SHORT 1
+#endif
What's preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably override the CONFIG_COMP_SRC_TINY default value in xtensa-build-zephyr.sh. We have the platform and compiler information there. For similar examples search for "config" in https://github.com/thesofproject/sof/blob/main/scripts/xtensa-build-all.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-hb I'd rather not do that. It should work without using the script too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many other ways to ran out of memory (or worse) with Kconfig but fair enough: so much better when the defaults work.
but then it would be used for both gcc and xcc
How much of a problem would this be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marc-hb 0, it was my mistake: with gcc the SRC_SHORT option is already used
singalsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another way
Building SOF with Zephyr for Apollolake with XCC fails because of insufficient RAM. Reduce the heap size to fix the problem. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
as discussed with @singalsu I've moved src configuration to a Zephyr PR zephyrproject-rtos/zephyr#39351 |
singalsu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
|
https://sof-ci.01.org/sofpr/PR4852/build10688/build/apl_zph_error.txt still failing. Some iteration on zephyrproject-rtos/zephyr#39351 needed. Once it is merged, we can merge this. |
Zephyr XCC builds for Apollolake can be re-enabled now after the RAM footprint has been reduced. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
@lgirdwood yes, there were some problems, but it's working fine now |
|
Still failing in https://sof-ci.01.org/sofpr/PR4852/build10697/build/ This was with Zephyr commit 2f359aeacfab and SOF commit: EDIT: Zephyr commit 2f359aeacfab was 2 commits short of @lyakh's |
|
SOFCI TEST |
|
Successful XCC + APL build at https://sof-ci.01.org/sofpr/PR4852/build10705/build/apl_zph.txt with newer Zephyr a8c4abdcf055 |
|
@lgirdwood This is now ready! |
This should fix #4645, but additional Zephyr fixes are needed to fix the run-time too