sof_api: lmdk: Extraction of api headers for loadable modules #8365

softwarecki · 2023-10-20T14:46:20Z

I separated some of the headers into a separate modules directory. They will be shared between sof and native loadable modules.

lgirdwood

Fwiw, I think git has a cmd line hint to help it detect file moves, but GH may not interpret it.

lgirdwood · 2023-10-20T16:20:50Z

src/include/sof_api/ipc4/base-config.h

@@ -0,0 +1,240 @@
+/* SPDX-License-Identifier: BSD-3-Clause */


So I agree having an API directory for modules is good, but can we call it include/module/ ? This way all our modules would be including e.g.

#include <module/ipc4/base-config.h> #include <module/module.h>

and so on. i.e. we make it clear this API is for modules and make it more obvious when new APIs are added if they are intended for baseFW or are intened for client modules.

Idea behind it is to keep this files as an external header package and calling it sof_api doesn't leave place for mistake.

Modules include are limited only to include/sof_api directory, so its include will looks like:

#include <ipc4/base-config.h> #include <module/generic.h>

From the sof perspective it is:

#include <sof_api/ipc4/base-config.h> #include <sof_api/module/generic.h>

Idea behind it is to keep this files as an external header package and calling it sof_api doesn't leave place for mistake.

This is not going to work if headers are external to this repo, headers need to be local to this repo.

Modules include are limited only to include/sof_api directory, so its include will looks like:

I'm good with that. My issue is that sof_api name is quite ambiguous i.e. API for SOF what ? all of SOF or just part of SOF. In this case it needs to be clear that this API is only for modules only.

These headers will be kept in this repo for CI needs. If any changes will be required we will create headers release package for loadable modules and change api version number to keep on track changes. For that will have to create some kind of ABI script only for this cathalog.

Modules include are limited only to include/sof_api directory, so its include will looks like:

I'm good with that. My issue is that sof_api name is quite ambiguous i.e. API for SOF what ? all of SOF or just part of SOF. In this case it needs to be clear that this API is only for modules only.

sof_api because we are using module_api, component_api and that headers contains essential data structures for modules to be loaded. If naming is such a problem we can move them to lmdk/include

The direction of travel is component api is deprecated and will be removed, we will only have module api for modules so this programming surface must be obvious for module developers. In the future we will have the following include directories (not showing IADK atm)

src/include/module - all APIs used for modules

src/include/sof - base FW apis, not used by modules.

src/audio/<module_name>/ - local module private headers, used by module only.

@lgirdwood I will rename sof_api to module soon.

src/include/ipc4/base-config.h

dbaluta · 2023-10-23T11:53:43Z

@lgirdwood @softwarecki

Now that Zephyr supports loadable module is there a plan to use this feature with SOF?

https://docs.zephyrproject.org/3.5.0/services/llext/index.html

lgirdwood · 2023-10-23T14:28:53Z

@lgirdwood @softwarecki

Now that Zephyr supports loadable module is there a plan to use this feature with SOF?

https://docs.zephyrproject.org/3.5.0/services/llext/index.html

Yep, we are working on several fronts to provide

backwards compatible modules/libraries API used by Windows (this PR and others from @softwarecki and @pjdobrowolski) and to be enabled soon for MTL for SOF.
proof of concept with the new Zephyr module APIs to see how this can help and work on SOF DSP architectures (@lyakh and @teburd) to be reviewed post MTL.

We should be able to have both methods usuable above via Kconfig. Your welcome to beta test :)

dbaluta · 2023-10-24T07:24:24Z

We should be able to have both methods usuable above via Kconfig. Your welcome to beta test :)

Great. Thanks. Will test the new features with Cadence libraries.

lyakh · 2023-10-24T07:32:54Z

We should be able to have both methods usuable above via Kconfig. Your welcome to beta test :)

Great. Thanks. Will test the new features with Cadence libraries.

here's the current state #8180 together with zephyrproject-rtos/zephyr#62433 but it's still WiP

src/include/sof/audio/module_adapter/module/generic.h

src/include/sof/audio/sink_api_implementation.h

src/include/sof_api/audio/sink_api.h

marcinszkudlinski · 2023-10-25T12:46:20Z

src/audio/module_adapter/iadk/system_agent.cpp

@@ -14,7 +14,7 @@
 #include <stdint.h>
 #include <rtos/string.h>
 #include <utilities/array.h>
-#include <adsp_error_code.h>


so now we have sof_api and iadk ?

Should not IADK go to sof_api as well?

what is the difference?

@cujomalainey IADK is an Intel C++ wrapper/extension to the base sof C module API that aligns with modules used on Windows today.

lmdk/cmake/ldscripts/text_linker_script.txt

cujomalainey · 2023-10-31T00:00:38Z

src/audio/module_adapter/iadk/system_agent.cpp

@@ -14,7 +14,7 @@
 #include <stdint.h>
 #include <rtos/string.h>
 #include <utilities/array.h>
-#include <adsp_error_code.h>


what is the difference?

src/include/module/audio/audio_stream.h

src/include/module/audio/format.h

src/include/module/ipc4/base-config.h

kv2019i

This is very hard to follow with massive amount of code moved around. I don't have any showstoppers though. I did add a few editorial comments. The copyright updates are really problematic, one should NEVER REMOVE copyright years from source files and this PR does this in a massive scale. But given we use git to track the history in SOF, this is probably not a blocker if everyone else is happy. I do worry about the work to maintain this series. Couldn't we merge parts of this in pieces to get progress. Having this kind of massive series long in review is taking lot of time both from you the authors as well as everybody reviewing.

src/include/ipc4/base-config.h

src/include/ipc4/up_down_mixer.h

src/include/module/iadk/adsp_error_code.h

src/include/sof/audio/module_adapter/module/generic.h

src/include/ipc/stream.h

src/include/sof/audio/audio_stream.h

src/include/sof/audio/module_adapter/module/module_interface.h

src/include/sof/audio/format.h

softwarecki · 2023-11-21T15:20:02Z

@kv2019i I restored copyright years.

From my perspective, it was just cutting parts of the files and pasting them into a new files. I thought that the review in this case would be a formality. In the future, in a similar situation, I will break it into a series of atomic PRs.

kv2019i

Thanks @softwarecki , good for me!

lgirdwood

I'm good with 99%, but we cant have conditional code in the structures as a marker for PRIVATE and public APIs, this will mean different ABI for different users. We should use a comment here whilst any cleanup is in progress.

lgirdwood · 2023-11-21T15:49:59Z

src/include/module/module/base.h

+	uint32_t period_bytes; /** pipeline period bytes */
+
+	/* Fields below can only be accessed by the SOF */
+#ifdef SOF_MODULE_API_PRIVATE


Is this new, wont it change the ABI if its defined for some users and not others ?

@lgirdwood This symbol is defined in the file included by sof. Loadable modules should not define it and nor refer to this part of the structure. This structures should be divided in a future so sof can keep their private fileds in a different structure. It will take a lot of work and will require a lot of changes. Would it be enough for now to add a comment that this is a temporary solution until these fields are separated into a new structure?

Yeah, the comment need to be very visible and explain the roadmap forwards. That way its really obvious to everyone working on modules.

kv2019i · 2023-11-21T16:29:53Z

@softwarecki wrote:

From my perspective, it was just cutting parts of the files and pasting them into a new files. I thought that the review in this case would be a formality.

Note for future: Yes, this generally applies and this is how I would treat a PR like you describe. But for this to work well, the PR needs to be doing just this. If you mix monotonic changes (like renames or moves) with small functional changes, then either you need to explain that very well in the cover letter ("one functional commit and rest are just renames"), or otherwise you will freak out the reviewers when they browse through the code and note changes that go beyond the simple. I mean this PR has also not so trivial changes, like the changes to header inclusion dependencies and changes to lmdk build rules. So when title says "api extraction", but I see commits that go beyond this, then this is no longer a trivial PR. This is ok as well, but just requires more time to review each iteration.

marc-hb

In the future, in a similar situation, I will break it into a series of atomic PRs.

I don't see why it's too late. You never have to submit all your local commits all at once. I git push myfork HEAD~5:refs/heads/notEverythingAllAtOnce all the time, keeping the last 5 commits (as an example) for later. The later commits provide more, local test coverage for the submitted commits.

You can still do this now.

Sometimes I push a second invisible/draft PR for even more test coverage (this is what I did in #8495)

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

EDIT: smaller PRs also provide more test coverage of individual commit and make it much easier to catch, understand and deal with regressions.

EDIT: smaller PRs also reduce conflicts and make their resolution easier:

marc-hb · 2023-11-22T23:54:15Z

src/module/audio/CMakeLists.txt

+		sink_api.c
+	)
+else()  ### Not Zephyr ###
+	add_local_sources(sof


I can simplify this after #8494 is merged. Good enough for now.

stale

Removed unused includes from a header files. Signed-off-by: Pawel Dobrowolski <pawelx.dobrowolski@intel.com>

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

Extracted struct sof_audio_stream_params and enum sof_ipc_frame definitions get_sample_bytes, get_sample_bitdepth and get_frame_bytes functions to a new files in the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

Use of a separated module headers when building native loadable libraries. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

Added building a static library containing common functions for modules provided by sof. Native loadable modules are statically linked to it. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

softwarecki · 2023-11-24T14:22:23Z

@lgirdwood @marc-hb @lyakh
I did a rebase to resolve the conflicts and expanded the description in the comments regarding the added #ifdefs.

softwarecki requested review from lyakh, lgirdwood, kv2019i and pjdobrowolski October 20, 2023 14:46

softwarecki requested review from abonislawski, RanderWang, marcinszkudlinski, pblaszko, plbossart, mmaka1, lbetlej and dbaluta as code owners October 20, 2023 14:46

softwarecki marked this pull request as draft October 20, 2023 14:49

softwarecki force-pushed the sof_api branch from 85c555e to 0168efe Compare October 20, 2023 14:56

lgirdwood reviewed Oct 20, 2023

View reviewed changes

softwarecki force-pushed the sof_api branch 2 times, most recently from f3d4891 to 60b7191 Compare October 23, 2023 11:48

softwarecki marked this pull request as ready for review October 23, 2023 12:14

softwarecki requested a review from a team as a code owner October 23, 2023 12:14

marcinszkudlinski requested changes Oct 25, 2023

View reviewed changes

pjdobrowolski approved these changes Oct 27, 2023

View reviewed changes

pjdobrowolski reviewed Oct 27, 2023

View reviewed changes

lmdk/cmake/ldscripts/text_linker_script.txt Outdated Show resolved Hide resolved

softwarecki force-pushed the sof_api branch 2 times, most recently from 53082f4 to 9cb0a2e Compare October 30, 2023 15:30

cujomalainey reviewed Oct 31, 2023

View reviewed changes

softwarecki added the loadable modules label Oct 31, 2023

softwarecki force-pushed the sof_api branch from e21e8d3 to 9bf4a37 Compare November 20, 2023 16:46

softwarecki requested review from lyakh, lgirdwood and marc-hb November 20, 2023 16:47

marcinszkudlinski approved these changes Nov 21, 2023

View reviewed changes

kv2019i reviewed Nov 21, 2023

View reviewed changes

softwarecki force-pushed the sof_api branch 2 times, most recently from a6254d2 to cab4bec Compare November 21, 2023 15:12

kv2019i approved these changes Nov 21, 2023

View reviewed changes

lgirdwood reviewed Nov 21, 2023

View reviewed changes

marc-hb reviewed Nov 23, 2023

View reviewed changes

pjdobrowolski and others added 3 commits November 24, 2023 14:11

audio: Header files cleanup

ed9727a

Removed unused includes from a header files. Signed-off-by: Pawel Dobrowolski <pawelx.dobrowolski@intel.com>

module: Move ip4/base-config headers to module

c67f879

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

module: Move adsp_error_code header to module

f52c249

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

softwarecki force-pushed the sof_api branch from cab4bec to 8ab539d Compare November 24, 2023 13:22

softwarecki added 7 commits November 24, 2023 15:13

module: module_adapter: Move module_interface header to module

ec879d6

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

module: module_adapter: Move generic header file to module

164935b

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

module: module_adapter: Move api_ver header file to module

06de180

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

module: sink: source: Move sink/source api headers to module

1df67fb

Moved header files to the module directory to separate an shared interface used by sof and native loadable modules. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

lmdk: module: Use extracted module headers in lmdk

7e9d31f

Use of a separated module headers when building native loadable libraries. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

lmdk: Build module common functions

be30d2b

Added building a static library containing common functions for modules provided by sof. Native loadable modules are statically linked to it. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

softwarecki force-pushed the sof_api branch from 8ab539d to be30d2b Compare November 24, 2023 14:14

lgirdwood approved these changes Nov 24, 2023

View reviewed changes

lgirdwood merged commit 4d4421a into thesofproject:main Nov 24, 2023
39 of 43 checks passed

marcinszkudlinski mentioned this pull request Nov 30, 2023

Add Loadable smart amp support #8554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sof_api: lmdk: Extraction of api headers for loadable modules #8365

sof_api: lmdk: Extraction of api headers for loadable modules #8365

softwarecki commented Oct 20, 2023 •

edited

Loading

lgirdwood left a comment

lgirdwood Oct 20, 2023

pjdobrowolski Oct 23, 2023

softwarecki Oct 23, 2023 •

edited

Loading

lgirdwood Oct 23, 2023

lgirdwood Oct 23, 2023 •

edited

Loading

pjdobrowolski Oct 24, 2023

pjdobrowolski Oct 24, 2023

lgirdwood Oct 25, 2023

softwarecki Oct 27, 2023

dbaluta commented Oct 23, 2023

lgirdwood commented Oct 23, 2023

dbaluta commented Oct 24, 2023

lyakh commented Oct 24, 2023

marcinszkudlinski Oct 25, 2023

cujomalainey Oct 31, 2023

lgirdwood Nov 10, 2023

cujomalainey Oct 31, 2023

kv2019i left a comment

softwarecki commented Nov 21, 2023

kv2019i left a comment

lgirdwood left a comment

lgirdwood Nov 21, 2023

softwarecki Nov 22, 2023

lgirdwood Nov 24, 2023

kv2019i commented Nov 21, 2023

marc-hb left a comment •

edited

Loading

marc-hb Nov 22, 2023

softwarecki commented Nov 24, 2023

		@@ -0,0 +1,240 @@
		/* SPDX-License-Identifier: BSD-3-Clause */

sof_api: lmdk: Extraction of api headers for loadable modules #8365

sof_api: lmdk: Extraction of api headers for loadable modules #8365

Conversation

softwarecki commented Oct 20, 2023 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarecki Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta commented Oct 23, 2023

lgirdwood commented Oct 23, 2023

dbaluta commented Oct 24, 2023

lyakh commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

softwarecki commented Nov 21, 2023

kv2019i left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Nov 21, 2023

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarecki commented Nov 24, 2023

softwarecki commented Oct 20, 2023 •

edited

Loading

softwarecki Oct 23, 2023 •

edited

Loading

lgirdwood Oct 23, 2023 •

edited

Loading

marc-hb left a comment •

edited

Loading