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.

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

smart-amp-test: remove obsolete build options #9773

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Jan 13, 2025

smart-amp-test was one of the first modules, used as an example for loadable module development. Currently it can be built in 4 ways:

  • monolithic
  • as an LMDK native module
  • as legacy LLEXT
  • as current LLEXT

This commit removes legacy options and only preserves the first and the last possibilities.

smart-amp-test was one of the first modules, used as an example for
loadable module development. Currently it can be built in 4 ways:
- monolithic
- as an LMDK native module
- as legacy LLEXT
- as current LLEXT
This commit removes legacy options and only preserves the first and
the last options.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lgirdwood
Copy link
Member

@abonislawski whats happening with LMDK these days now that we can do llext ? Can we support LMDK APIs and loading via llext ?

@abonislawski
Copy link
Member

@lgirdwood we are only keeping one LMDK module (downmixer) in CI just in case if something change in the future roadmap. But in general it is frozen and we can remove smart-amp

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 14, 2025

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.

Mostly ok, but a few suprise code removals that I can't link to smart-amp-test, please see inline

@@ -15,7 +15,7 @@ SECTIONS {
_data_end = ABSOLUTE(.);
} >HPSRAM_seg : data_phdr

.rodata : ALIGN(4096) {
.rodata : {
Copy link
Collaborator

Choose a reason for hiding this comment

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

? Thhis seems very odd in a PR to remove smart-ampt-test

#include <sof/compiler_attributes.h>
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

This too, is this removing more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kv2019i no, all those changes were brought in with the PR, that enabled modular smart-amp-test 694aa9e

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lyakh But smart-ampt-test was an example, wouldn't these changed be needed by other LMDK modules? OTOH, the CI tests seem to pass (there's at least the mixer module), so maybe these were needed just for smart-amp-test and not LMDK support in general. Just not so easy to make this conclusion from the diff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kv2019i that's also my understanding, that since these changes were introduced with the smart-amp-test and reverting them doesn't break QB tests, don't think they were needed for other LMDK examples, i.e. for the down-mixer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants