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

lmdk: fix building system-service modules #8812

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Jan 29, 2024

System-service modules cannot include Zephyr headers and don't use exported symbols, they link sink_api.c and source_api.c into their images. Disable symbol exporting for them.

softwarecki
softwarecki previously approved these changes Jan 29, 2024
@softwarecki softwarecki dismissed their stale review January 29, 2024 13:59

will try it first

Copy link
Collaborator

@softwarecki softwarecki left a comment

Choose a reason for hiding this comment

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

This PR doesn't solve the problem. The __SOF_MODULE_SERVICE_BUILD__ symbol is defined only in one example module. We need more universal solution.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 29, 2024

This PR doesn't solve the problem. The __SOF_MODULE_SERVICE_BUILD__ symbol is defined only in one example module. We need more universal solution.

@softwarecki you don't use it when building LMDK modules? Ok, I thought you did. Is there anything that you do define for all system-service modules?

@softwarecki
Copy link
Collaborator

Ok, I thought you did. Is there anything that you do define for all system-service modules?

We don't define it in our examples of lmdk modules. Please check files in lmdk/cmake or use some definition used by llmodules.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 30, 2024

or use "some" definition used by llmodules.

That sounds very brittle (quotes mine)

Can you confirm MAJOR_IADSP_API_VERSION is here to stay for a while?

@lgirdwood
Copy link
Member

@softwarecki btw, why does system services build and link to a base FW file (e.fg the source/sink API) ? Shouldn't these be unresolved in the library and then be resolved when the library is loaded ?

@softwarecki
Copy link
Collaborator

@marc-hb MAJOR_IADSP_API_VERSION is currently unused and I planned to remove it in the future as I did in #8546 .

@lgirdwood I moved the common files used for building basefw and lmdk modules to the directory modules. These are simple wrappers that invoke pointers functions provided by the sink/source API.

@lyakh Can we reverse the condition and check for the existence of a definition associated with llext or move these exports to another file?

Copy link
Collaborator

@softwarecki softwarecki left a comment

Choose a reason for hiding this comment

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

MAJOR_IADSP_API_VERSION is deprecated

System-service modules cannot include Zephyr headers and don't use
exported symbols, they link sink_api.c and source_api.c into their
images. Disable symbol exporting for them.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Copy link
Collaborator

@softwarecki softwarecki left a comment

Choose a reason for hiding this comment

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

This is a very surprising solution, but it works so I accept it.

@lyakh
Copy link
Collaborator Author

lyakh commented Feb 5, 2024

I think we can safely state, that this PR cannot be a reason for all the CI failures:

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 5, 2024

This is a very surprising solution, but it works so I accept it.

Moreover it's short, well documented and self-contained in only 2 places to change if needed in the future. So very low technical debt if any!

@kv2019i kv2019i merged commit 45dc968 into thesofproject:main Feb 6, 2024
38 of 44 checks passed
@lyakh lyakh deleted the lmdk branch February 6, 2024 07:55
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.

6 participants