Intel modules adapter implementation #5848

jxstelter · 2022-05-23T14:43:06Z

Intel module adapter is an extension to SOF module adapter component that allows to integrate
modules developed under IADK (Intel Audio Development Kit) Framework. IADK modules uses uniform
set of interfaces and are linked into separate library. These modules are loaded in runtime
through library_manager and then after registration into SOF component infrastructure are
interfaced through module adapter API.

There is variety of modules developed under IADK Framework by 3rd party vendors. The assumption
here is to integrate these modules with SOF infrastructure without modules code modifications.
Another assumption is that the 3rd party modules should be loaded in runtime without need
of rebuild the base firmware.
Therefore C++ function, structures and variables definition are here kept with original form from
IADK Framework. This provides binary compatibility for already developed 3rd party modules.

Since IADK modules uses ProcessingModuleInterface to control/data transfer and AdspSystemService
to use base FW services from internal module code, there is a communication shim layer defined
in intel_module_adapter directory.

Since ProcessingModuleInterface consists of virtual functions, there are C++ -> C wrappers
defined to access the interface calls from SOF code.

There are three entities in intel module adapter package:

System Agent - A mediator to allow the custom module to interact with the base SOF FW.
It calls IADK module entry point and provides all necessary information to
connect both sides of ProcessingModuleInterface and System Service.
System Service - exposes of SOF base FW services to the module.
Processing Module Adapter - SOF base FW side of ProcessingModuleInterface API

This code will be usable after this PR: #5796 will be integrated.

mwasko · 2022-05-24T09:58:57Z

@lgirdwood, @ranj063, @dbaluta, @cujomalainey - fyi, feedback welcome

mwasko · 2022-05-24T11:46:56Z

src/include/sof/audio/module_adapter/intel_module_adapter/module_initial_settings_concrete.h

this struct name should be changed to something that has no reference to specific ipc protocol version

The ipc4_base_module_cfg is type of structure passed during init instance IPC. It is used across other build in modules in SOF, like aria, up_down_mixer, volume and src. IADK modules works only with IPC4 protocol. Why not to use it here?
I can define BaseModuleCfg type directly, but it will exact match the ipc4_base_module_cfg definition.

lgirdwood

A lot of code to review here. Can we split into per feature/API PRs for easier review.
It would also be helpful to see how we can reduce any duplication (structs, macros, enums etc) and use some name spacing around some definitions (as we have some generic names that could collide with current code and Zephyr).

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

src/include/sof/audio/module_adapter/intel_module_adapter/adsp_stddef.h

src/include/sof/audio/module_adapter/intel_module_adapter/core/kernel/logger/log.h

src/include/sof/audio/module_adapter/intel_module_adapter/ipc/module_initial_settings.h

lyakh · 2022-05-25T07:15:42Z

A lot of code to review here. Can we split into per feature/API PRs for easier review.

+1 on that. Also maybe it's only me who doesn't understand this, but I'd think, that to integrate with IADK modules we need their ABI definition and native SOF adapter code. Whereas this PR seems to contain a lot more. Why is this all needed in SOF?

src/include/sof/audio/module_adapter/intel_module_adapter/adsp_error_code.h

src/audio/module_adapter/module/intel_modules.c

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

lyakh · 2022-05-25T07:30:08Z

...if all those headers are needed to interface to IADK modules, because those are C++ libraries and they're an exact copy of respective IADK headers, then at least we should mark that whole directory - don't even touch this - or maybe make them an external dependency?

dbaluta · 2022-05-25T07:31:36Z

...if all those headers are needed to interface to IADK modules, because those are C++ libraries and they're an exact copy of respective IADK headers, then at least we should mark that whole directory - don't even touch this - or maybe make them an external dependency?

@lyakh should we introduce the concept of 'SDK' similiar with what Zephyr does and have all this information outside of SOF?

lyakh · 2022-05-25T09:58:41Z

...if all those headers are needed to interface to IADK modules, because those are C++ libraries and they're an exact copy of respective IADK headers, then at least we should mark that whole directory - don't even touch this - or maybe make them an external dependency?

@lyakh should we introduce the concept of 'SDK' similiar with what Zephyr does and have all this information outside of SOF?

@dbaluta is that what Zephyr SDK is for? AFAIK usually you build with Zephyr by linking your application together with Zephyr so you do need complete Zephyr sources. So far that's the same as what we do with SOF, you have to link statically with SOF. To link your module with SOF, you create an interface and you can decide where to maintain that interface - with SOF by submitting it to the SOF mainline or with your project. Either way should work, right? But for cases like this one - at the very least I'd rather have a very clear indication somewhere: external API, verbatim copy from external project, don't touch. But actually maintaining it separately, maybe as yet another git submodule, would be even cleaner?

jxstelter · 2022-05-25T14:25:03Z

...if all those headers are needed to interface to IADK modules, because those are C++ libraries and they're an exact copy of respective IADK headers, then at least we should mark that whole directory - don't even touch this - or maybe make them an external dependency?

@lyakh should we introduce the concept of 'SDK' similiar with what Zephyr does and have all this information outside of SOF?

@dbaluta is that what Zephyr SDK is for? AFAIK usually you build with Zephyr by linking your application together with Zephyr so you do need complete Zephyr sources. So far that's the same as what we do with SOF, you have to link statically with SOF. To link your module with SOF, you create an interface and you can decide where to maintain that interface - with SOF by submitting it to the SOF mainline or with your project. Either way should work, right? But for cases like this one - at the very least I'd rather have a very clear indication somewhere: external API, verbatim copy from external project, don't touch. But actually maintaining it separately, maybe as yet another git submodule, would be even cleaner?

This is why I posted this draft PR for RFC. I hope that we can work out the best solution. Git submodule is also an option. But there was discussion about plan to enable this loadable library support and then add IADK module examples here too. Therefore we need these headers in original form to keep compatibility with IADK framework and be able to rebuild modules in SOF environment if anybody need to.
Another think is that IADK modules are not linked with SOF code statically. These are separate binaries loaded in runtime, so there is logic responsible to dynamically interface SOF base firmware with that loadable module. This explain higher complication level comparing statically linked modules.

lgirdwood · 2022-05-27T11:02:13Z

...if all those headers are needed to interface to IADK modules, because those are C++ libraries and they're an exact copy of respective IADK headers, then at least we should mark that whole directory - don't even touch this - or maybe make them an external dependency?

@lyakh should we introduce the concept of 'SDK' similiar with what Zephyr does and have all this information outside of SOF?

@dbaluta is that what Zephyr SDK is for? AFAIK usually you build with Zephyr by linking your application together with Zephyr so you do need complete Zephyr sources. So far that's the same as what we do with SOF, you have to link statically with SOF. To link your module with SOF, you create an interface and you can decide where to maintain that interface - with SOF by submitting it to the SOF mainline or with your project. Either way should work, right? But for cases like this one - at the very least I'd rather have a very clear indication somewhere: external API, verbatim copy from external project, don't touch. But actually maintaining it separately, maybe as yet another git submodule, would be even cleaner?

This is why I posted this draft PR for RFC. I hope that we can work out the best solution. Git submodule is also an option. But there was discussion about plan to enable this loadable library support and then add IADK module examples here too. Therefore we need these headers in original form to keep compatibility with IADK framework and be able to rebuild modules in SOF environment if anybody need to. Another think is that IADK modules are not linked with SOF code statically. These are separate binaries loaded in runtime, so there is logic responsible to dynamically interface SOF base firmware with that loadable module. This explain higher complication level comparing statically linked modules.

This API is a wrapper between core SOF and the Intel processing modules in the exact same way we wrap Waves, Cadence and DTS modules with the following differences.

Its C++ instead of C.
It also wraps and exposes services alongside the audio API.
It has an eclipse plugin GUI (the IADK) which is available to Intel module integrators.

The best solution is to treat it like the other module interfaces and upstream in main (and after all review comments are addressed). This then allows the Intel modules to be treated the same as the other modules.

At a high level this does need (and may already have - I may have missed due to PR size).

A Kconfig menu entry to enable the Intel module API (which may probably depend on Zephyr IIUC).
Some stub or example Intel modules (open source and upstream) that should be built and run as part of CI (agreed at TSC - we need to do for others too) with Kconfigs to build each Intel module.

lgirdwood

At a high level the audio API looks fine, but there are some opens on the logging and notification services and these should wrap the existing Zephyr APIs. We also have some generic macros and not much name spacing that may bring a lot of compilation issues that need to be addressed.

src/include/sof/audio/module_adapter/intel/core/kernel/logger/log.h

lgirdwood · 2022-05-27T11:07:33Z

src/include/sof/audio/module_adapter/intel/core/kernel/logger/log.h

Looks like we are redefining log priorities already in Zephyr.

This is a problem of library build in IADK environment. It was build in IADK defined log priorities and library generates such priorities. I need to convert those into Zephyr definitions in system service call.

Library calls this function:
void SystemServiceLogMessage(AdspLogPriority log_priority, uint32_t log_entry,
AdspLogHandle const *log_handle, uint32_t param1, uint32_t param2,
uint32_t param3, uint32_t param4);

Passing AdspLogPriority like in this define. In the SystemServiceLogMessage() routine body I can "translate" this into Zephyr codes and generate Zephyr kind log. Can not change this directly since library does not know Zephyr log priority definitions.

ok, I wonder if we can get away with converting some log levels using the compiler e.g.

#define SOME_IADK_LOG_LEVEL EQUIVALENT_ZEPHYR_LOG_LEVEL

My expectation is that all legacy modules will need recompiled for use with Zephyr RTOS

Also thinking we could have a high level macro to exclude some definitions that are used by IADK legacy code e.g.

#ifdef IADK_LEGACY_SUPPORT #include "iadk_rtos_definitions.h" #else #include "zephyr_rtos_definitions.h" #endif

The IADK build CC cmd line could define -DIADK_LEGACY_SUPPORT and pull in the correct header of the older modules that need a lot of the older macros etc. The newer stuff could use the Zephyr definitions.

Mapped to log levels already in SOF. There is separate development to add cAVS/ACE legacy logging format support. Loadable modules logging will be available after this work will be done.

lgirdwood · 2022-05-27T11:08:22Z

src/include/sof/audio/module_adapter/intel/core/kernel/logger/log.h

and we should use the Zephyr log descriptor for each of these log sources.

Same here. Library was build with those definitions. It will generate log entry with L_MODULE type. In the SystemServiceLogMessage() routine body we can generate Zephyr log descriptor but first we need to decode what was obtained from the library.

I think the Zephyr LOG classes are static objects, i.e. they need to be declared at build time. @andyross ? e.g. from zephyr_domain.c

LOG_MODULE_DECLARE(ll_schedule, CONFIG_SOF_LOG_LEVEL);

We maybe able to dynamically create them though ?

My understanding is no, actually. The log modules are fixed (they go with C translation units and are implemented with linker section names, there's no runtime data structure). Log backends and filtering state can be changed at runtime, but the set of modules is assumed to be static. (I think -- not really an expert on logging).

If there's need for having loadable modules integrate with log module identifiers, there may need to be some infrastructure work done. @dcpleung or @nordic-krch might have ideas.

There is an option to have a dynamic instance but it was intended to be used for instances of the module. It allows to have dynamic filtering which is instance specific also module name that is used in prefix comes from instance but what is complied into the binary is decided based on log level set by LOG_MODULE_DECLARE or LOG_MODULE_REGISTER.

You can check the sample:

instance logging setup: https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/subsys/logging/logger/src/sample_instance.h

usage: https://github.com/zephyrproject-rtos/zephyr/blob/092406b08d41a4596925fc2a2eeaf09c5d12b769/samples/subsys/logging/logger/src/sample_instance.c#L24

src/include/sof/audio/module_adapter/intel/core/kernel/logger/log.h

src/include/sof/audio/module_adapter/intel/q_format.h

src/include/sof/audio/module_adapter/intel/syntax_sugar.h

src/include/sof/audio/module_adapter/intel/system_service.h

src/include/sof/audio/module_adapter/intel/xtensa_overlays.h

jxstelter · 2022-05-27T13:11:27Z

At a high level the audio API looks fine, but there are some opens on the logging and notification services and these should wrap the existing Zephyr APIs. We also have some generic macros and not much name spacing that may bring a lot of compilation issues that need to be addressed.

The general problem here is that library is build with IADK like definitions - not in SOF environment. Therefore (not only for logging, but this is good example) library calls SOF callbacks called here SystemService API with parameters defined as it knows them. In SystemService routines it is "translated" into SOF/Zephyr specific definitions and called appropriate API to do the job.
Replacing IADK defines with SOF/Zephyr specific will require IADK Library rebuild in SOF environment to generate new binary.
Our target was to use directly libraries build under IADK framework.

lgirdwood · 2022-05-30T12:28:07Z

The general problem here is that library is build with IADK like definitions - not in SOF environment. Therefore (not only for logging, but this is good example) library calls SOF callbacks called here SystemService API with parameters defined as it knows them. In SystemService routines it is "translated" into SOF/Zephyr specific definitions and called appropriate API to do the job.
Replacing IADK defines with SOF/Zephyr specific will require IADK Library rebuild in SOF environment to generate new binary.
Our target was to use directly libraries build under IADK framework.

I think IADK has to move with the times. We can do this both gradually and incrementally though. The pre-processor and abstraction will help us here, and allow a more manageable transition over time to a native SOF/Zephyr target.

Fwiw, I dont see any blockers in the audio API, logger API (which can wrap Zephyr), messaging API (which can wrap Zephyr) but the difficult part seems to be the rtos level macros/APIs (and these may need a more individual feature by feature integration plan)

jxstelter · 2022-05-30T12:42:25Z

Significant part of the logger API and messaging API code is needed really only for IADK modules building and not to build the intel adapter part. I am now working to remove from that PR the part not needed for adapter itself. This way the PR will be smaller and reduced to minimal set of IADK origin definitions. The rest could be added when we decide to support of IADK modules building under SOF build system.

src/include/sof/audio/module_adapter/intel/adsp_stddef.h

src/include/sof/audio/module_adapter/intel/fixed_array.h

src/include/sof/audio/module_adapter/intel/processing_module_adapter.h

ranj063 · 2022-05-31T16:51:01Z

src/include/sof/audio/module_adapter/intel/processing_module_interface.h

what is the relation between ProcessingModuleInterface and class Moduleadapter?

Actually the class ModuleAdapter create wrappers from C++ to C while ProcessingModuleInterface is an actual definition of interface impemented by external module. I think ModuleAdapter class is wrong name here. I am going to change it to something more accurate.

src/audio/module_adapter/module/intel_modules.c

src/audio/module_adapter/module_adapter.c

mmaka1 · 2022-06-01T09:48:11Z

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

It seems that selection of the part extracted to 'common.h' was driven by the internal source code factoring of the IADK adapter. There might be a better way to provide compilable header for C and C++ units, where all definitions required by a module_interface implementor are included and nothing more. For example generic.h could be renamed to module_interface.h (as this is the main thing defined here), keep module interface definition, buffers definition etc. and all things that should not be visible to the implementor could be moved from this header to module_adapter.c (like component definitions since this is one of the parts that module_adapter is trying to hide).

I changed this according to your request. Please review.

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

src/include/sof/audio/module_adapter/intel/processing_module_adapter.h

src/audio/module_adapter/module/intel_modules.c

lgirdwood

Looks good, just a few questions from me. Is there any order to upstream ? i.e. does this need to go before memory manager ?

lgirdwood · 2022-07-06T12:25:44Z

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

@jxstelter which parts of the adapter are generic and which parts are to support the Intel modules ? The reason I ask is that we have some generic file and directory naming and some Intel specific KConfigs mixed today.

IIRC @mmaka1 and @mwasko had made some similar observations e.g. that we would have the intel specific files go under a module_adapter/intel/ and generic files under module_adapter/.

Generic files are under module_adapter/ directory. There is one iadk_modules.c file which provides interfacse identical to this exposed by cadence.c, waves.c etc. in that place.
Directory module_adapter/iadk contains all intel specific files needed to connect with IADK modules. Initially this directory was module_adapter/intel, but during review was changed to module_adapter/iadk.

ok, so we cant include any iadk (intel) headers in generic code. The library manager PR is doing this but we can fix by keeping generic definitions in generic.h and include generic.h in the iadk headers.

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

src/include/sof/audio/module_adapter/iadk/iadk_module_adapter.h

src/audio/module_adapter/iadk/iadk_module_adapter.cpp

jxstelter · 2022-07-08T12:15:29Z

Looks good, just a few questions from me. Is there any order to upstream ? i.e. does this need to go before memory manager ?

Memory manager should be in upstream as well as library manager to have this code usable. Could be merged but will be not used until the other components merge.

src/include/sof/audio/module_adapter/iadk/adsp_error_code.h

cujomalainey · 2022-07-12T17:42:08Z

src/include/sof/audio/module_adapter/iadk/fixed_array.h

why is this intel specific?

This is part of IADK module interface. IADK module starting routine gets parameters with specific class. This adapter construct parameters of specific type to pas them to IADK callbacks.
See this IADK module example code for reference:
https://github.com/thesofproject/converged-sof-modules

@cujomalainey the iadk interface will look like the Cadence/Waves/Realtek/IGO interfaces at a high level and provide a similar wrapping between the core processing logic and the processing logic used by the module. The iadk interface also wraps some extra APIs around logging, probes and helper code for convenience.

cujomalainey · 2022-07-12T17:42:49Z

src/include/sof/audio/module_adapter/iadk/fixed_array.h

shouldnt this be a c struct?

Why? It should be used only in C++ code - the interface layer to IADK module API for parameter conversion.

@cujomalainey so this part is a convenience helper API not exposed by the C module API. The C module API uses regular C arrays here. The intention is to have the C++ module API wrap the C module API (same methods, flows, states etc) and provide further high level C++ type convenience where needed.

cujomalainey · 2022-07-12T17:44:15Z

src/include/sof/audio/module_adapter/iadk/iadk_module_adapter.h

is this transitional?

sorry, not following the question. The intention is to have a low level generic C module API used by core (and other modules) and also a high level C++ API (for existing Intel modules in C++). The same mechanism would be used to add a rust API (i.e. build it on top of the core C API).

src/audio/module_adapter/intel_module_adapter/module_initial_settings_concrete.cpp

src/audio/module_adapter/intel_module_adapter/system_service.c

src/audio/module_adapter/module/intel_modules.c

lgirdwood

@cujomalainey any opens for you ?
@jxstelter only a few comments that need updating for me. I want to make sure we have a good separation between the generic core code and the Intel IADK API. The generic code should not depend on the IADK.

lgirdwood · 2022-07-19T14:10:40Z

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

ok, so we cant include any iadk (intel) headers in generic code. The library manager PR is doing this but we can fix by keeping generic definitions in generic.h and include generic.h in the iadk headers.

lgirdwood · 2022-07-19T14:17:13Z

src/include/sof/audio/module_adapter/iadk/fixed_array.h

@cujomalainey the iadk interface will look like the Cadence/Waves/Realtek/IGO interfaces at a high level and provide a similar wrapping between the core processing logic and the processing logic used by the module. The iadk interface also wraps some extra APIs around logging, probes and helper code for convenience.

lgirdwood · 2022-07-19T14:21:59Z

src/include/sof/audio/module_adapter/iadk/fixed_array.h

@cujomalainey so this part is a convenience helper API not exposed by the C module API. The C module API uses regular C arrays here. The intention is to have the C++ module API wrap the C module API (same methods, flows, states etc) and provide further high level C++ type convenience where needed.

lgirdwood · 2022-07-19T14:25:25Z

src/include/sof/audio/module_adapter/iadk/iadk_module_adapter.h

sorry, not following the question. The intention is to have a low level generic C module API used by core (and other modules) and also a high level C++ API (for existing Intel modules in C++). The same mechanism would be used to add a rust API (i.e. build it on top of the core C API).

jxstelter · 2022-07-20T13:12:13Z

@jxstelter only a few comments that need updating for me. I want to make sure we have a good separation between the generic core code and the Intel IADK API. The generic code should not depend on the IADK.

@lgirdwood Changed to separate IADK from Library manager. Please review.

lgirdwood

Just one open from me, @cujomalainey anything from you ?

lgirdwood · 2022-07-20T14:28:14Z

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

Its better to have the include "iadk_modules.h" in the Intel module client code as this is still a dependency on generic.h i.e. it should be in the C/C++ Intel code that uses the IADK module API.

e.g. in the Intel client code

#include <module/generic.h> #include <module/iadk_modules.h>

There is one problem. Library manager creates new comp_driver object after module load and needs to assign its ops at this point. It will load IADK or SOF loadable modules but at some point it needs to select correct set of ops callbacks. See this: https://github.com/thesofproject/sof/blob/96618de55e0417f976761d69eb020359d7912216/src/library_manager/lib_manager.c#L369

We could add some function call to generic.h, that will do it inside generic code. But still generic will need to include iadk_modules.h functions. Don't know yet how to separate these two better.

ok, so I had another look at iadk_modules.h and I could not see anything that is not generic. i.e.

struct comp_dev *iadk_modules_shim_new(const struct comp_driver *drv, struct comp_ipc_config *config, void *spec); <<<<<< this func could be renamed module_shim_new() #define DECLARE_DYNAMIC_MODULE_ADAPTER(comp_dynamic_module, mtype, uuid, tr) \ do { \ (comp_dynamic_module)->type = mtype; \ (comp_dynamic_module)->uid = SOF_RT_UUID(uuid); \ (comp_dynamic_module)->tctx = &(tr); \ (comp_dynamic_module)->ops.create = *iadk_modules_shim_new; \ (comp_dynamic_module)->ops.prepare = module_adapter_prepare; \ (comp_dynamic_module)->ops.params = module_adapter_params; \ (comp_dynamic_module)->ops.copy = module_adapter_copy; \ (comp_dynamic_module)->ops.cmd = module_adapter_cmd; \ (comp_dynamic_module)->ops.trigger = module_adapter_trigger; \ (comp_dynamic_module)->ops.reset = module_adapter_reset; \ (comp_dynamic_module)->ops.free = module_adapter_free; \ } while (0)

This could all be copied into generic.h and with the func name change and would provide a generic method with no dependency on IADK APIs ? The module_shim_new() would need to be in a generic file too.

I understand your point, but even module_shim_new() will be in generic it has to refer to struct module_interface iadk_interface() which is iadk_modules.c specific. To be honest I don't know yet how to avoid this dependency. For built in modules you can create and initialize component driver at built time. For loadable You will got UUID at loading time so it has to be called at IPC service context.

lgirdwood

@jxstelter @mwasko as discussed, lets address the generic example in part 2 of this feature so that we have a method to determine if libraries are generic or derived from IADK API. Merging now so we can unblock folks forking on the kernel part of this. We should have both the generic and iadk module loading working for the v2.3 release.

lgirdwood · 2022-07-22T20:11:41Z

The license server fails us again.

[ 87%] Building C object CMakeFiles/sof.dir/src/drivers/intel/cavs/idc.c.o
License checkout failed: Cannot connect to license server system.
 The license server manager (lmgrd) has not been started yet,
 the wrong port@host or license file is being used, or the
 port or hostname in the license file has been changed.
Feature:       XTENSA_XCC_TIE
Server name:   fmylic7005.fm.intel.com
License path:  84300@xtensa01p.elic.intel.com:/srv/home/jenkins/xcc/install/tools/RG-2017.8-linux/XtensaTools/Tools/lic/license.dat:
FLEXnet Licensing error:-15,570.  System Error: 115 "Operation now in progress"
For further information, refer to the FLEXnet Licensing documentation,
available at "www.macrovision.com".
make[3]: *** [src/platform/cannonlake/CMakeFiles/boot_module.dir/build.make:76: src/platform/cannonlake/CMakeFiles/boot_module.dir/boot_module.c.o] Error 2
make[2]: *** [CMakeFiles/Makefile2:2230: src/platform/cannonlake/CMakeFiles/boot_module.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....
[ 89%] Building C object CMakeFiles/sof.dir/src/drivers/intel/hda/hda-dma.c.o

rerun the CI.

lgirdwood · 2022-07-22T20:11:48Z

SOFCI TEST

These defines need to be included in C++ wrappers for ProcesingModuleInterface defined in Intel IADK based 3rd party modules. Move it to separate file to not include whole sof stuff there. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

Add header files with definitions needed to build wrappers for run time integration of 3rd party modules developed with Intel IADK framework. Note: file naming and localization is kept from IADK Framework to be able to integrate or also rebuild and integrate original 3rd party modules without their code modifications. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

IADK based modules uses two interfaces to communicate with base firmware: - ProcessingModuleInterface; - SystemServices. Implementation of both with C++->C wrappers are added in these files. Additionally system_agent component is responsible for integration of both communications path after module loading. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

Add files with definition of module_adapter component for intel IADK modules. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

Added glue logic to integrate with module adapter the loadable modules developed under Intel IADK Framework. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

Do not unconditionally define assert() to nothing in some obscure and unrelated array.h file. This looks like it was a temporary hack that was missed in: Fixes commit 6f366e2 ("Add definition of interfaces of Intel IADK modules."), part of PR thesofproject#5848 too massive to be properly reviewed. (docs.zephyrproject.org/latest/contribute/contributor_expectations.html) In addition to enabling asserts when requested by a debug build (!), fix the following messenger warning: ``` In file included from src/include/sof/common.h:30, from src/include/sof/audio/sink_api.h:10, from src/include/sof/audio/module_adapter/module/module_interface.h:17, from src/include/sof/audio/module_adapter/iadk/iadk_module_adapter.h:19, from src/audio/module_adapter/iadk/system_agent.cpp:22: zephyr/include/rtos/panic.h:19: error: "assert" redefined [-Werror] 19 | #define assert(x) __ASSERT_NO_MSG(x) | In file included from src/audio/module_adapter/iadk/system_agent.cpp:16: src/include/sof/audio/module_adapter/iadk/utilities/array.h:60: note: this is the location of the previous definition 60 | #define assert(cond) ``` This warning appeared only later in messenger commit 002d4a3 ("Pipeline2.0: change module API to use various data") because it started to include audio/sink_api.h in module_interface.h. This led to the two definitions finally clashing with each other. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2022-08-16T11:16:32Z

src/audio/module_adapter/module/iadk_modules.c

+/**
+ * \brief iadk_modules_process.
+ * \param[in] mod - processing module pointer.
+ *


This doxygen is missing a lot of parameters, which generates warnings. Disabled for now in PR #6143.

marc-hb · 2023-06-21T20:40:41Z

src/include/sof/audio/module_adapter/iadk/utilities/array.h

+
+#ifdef __cplusplus
+/* clang-format off */
+#define assert(cond)


Fix submitted in

iadk/array.h: do not redefine assert() to nothing in an unrelated .h #7846

Do not unconditionally define assert() to nothing in some obscure and unrelated array.h file. This looks like it was a temporary hack that was missed in: Fixes commit 6f366e2 ("Add definition of interfaces of Intel IADK modules."), part of PR #5848 too massive to be properly reviewed. (docs.zephyrproject.org/latest/contribute/contributor_expectations.html) In addition to enabling asserts when requested by a debug build (!), fix the following messenger warning: ``` In file included from src/include/sof/common.h:30, from src/include/sof/audio/sink_api.h:10, from src/include/sof/audio/module_adapter/module/module_interface.h:17, from src/include/sof/audio/module_adapter/iadk/iadk_module_adapter.h:19, from src/audio/module_adapter/iadk/system_agent.cpp:22: zephyr/include/rtos/panic.h:19: error: "assert" redefined [-Werror] 19 | #define assert(x) __ASSERT_NO_MSG(x) | In file included from src/audio/module_adapter/iadk/system_agent.cpp:16: src/include/sof/audio/module_adapter/iadk/utilities/array.h:60: note: this is the location of the previous definition 60 | #define assert(cond) ``` This warning appeared only later in messenger commit 002d4a3 ("Pipeline2.0: change module API to use various data") because it started to include audio/sink_api.h in module_interface.h. This led to the two definitions finally clashing with each other. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

jxstelter requested review from mwasko and ranj063 May 23, 2022 14:43

mwasko reviewed May 24, 2022

View reviewed changes

lgirdwood added the P1 Blocker bugs or important features label May 24, 2022

lgirdwood reviewed May 24, 2022

View reviewed changes

dbaluta reviewed May 25, 2022

View reviewed changes

src/include/sof/audio/module_adapter/intel_module_adapter/adsp_error_code.h Outdated Show resolved Hide resolved

dbaluta reviewed May 25, 2022

View reviewed changes

src/audio/module_adapter/module/intel_modules.c Outdated Show resolved Hide resolved

dbaluta reviewed May 25, 2022

View reviewed changes

src/include/sof/audio/module_adapter/module/generic.h Outdated Show resolved Hide resolved

jxstelter force-pushed the intel_modules_adapter branch from 1fcc6cf to d7646eb Compare May 26, 2022 15:13

lgirdwood reviewed May 27, 2022

View reviewed changes

jxstelter force-pushed the intel_modules_adapter branch 2 times, most recently from fee5021 to 1aa3c96 Compare May 31, 2022 15:58

ranj063 reviewed May 31, 2022

View reviewed changes

src/audio/module_adapter/module_adapter.c Outdated Show resolved Hide resolved

mmaka1 requested changes Jun 1, 2022

View reviewed changes

jxstelter force-pushed the intel_modules_adapter branch 3 times, most recently from 12e22d6 to 00bb19d Compare June 7, 2022 10:24

jxstelter force-pushed the intel_modules_adapter branch 2 times, most recently from 70f4f83 to c3ac055 Compare June 24, 2022 09:08

jxstelter requested review from lgirdwood, mwasko and ranj063 July 5, 2022 09:26

jxstelter force-pushed the intel_modules_adapter branch from 7ea7b86 to 0d187a6 Compare July 5, 2022 15:56

lgirdwood reviewed Jul 6, 2022

View reviewed changes

jxstelter force-pushed the intel_modules_adapter branch from 0d187a6 to c168a1b Compare July 8, 2022 11:31

cujomalainey reviewed Jul 12, 2022

View reviewed changes

mwasko approved these changes Jul 19, 2022

View reviewed changes

lgirdwood reviewed Jul 19, 2022

View reviewed changes

jxstelter force-pushed the intel_modules_adapter branch from c168a1b to 27132bb Compare July 20, 2022 10:51

lgirdwood reviewed Jul 20, 2022

View reviewed changes

lgirdwood approved these changes Jul 22, 2022

View reviewed changes

jxstelter added 5 commits July 25, 2022 10:44

Add module_adapter component for intel modules.

c9c95b1

Add files with definition of module_adapter component for intel IADK modules. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

Connect intel_module_adapter with generic module adapter.

3295eda

Added glue logic to integrate with module adapter the loadable modules developed under Intel IADK Framework. Signed-off-by: Jaroslaw Stelter <Jaroslaw.Stelter@intel.com>

jxstelter force-pushed the intel_modules_adapter branch from 27132bb to 3295eda Compare July 25, 2022 08:45

lgirdwood approved these changes Jul 25, 2022

View reviewed changes

lgirdwood merged commit f9741cb into thesofproject:main Jul 25, 2022

marc-hb mentioned this pull request Jun 21, 2023

iadk/array.h: do not redefine assert() to nothing in an unrelated .h #7846

Merged

marc-hb reviewed Jun 21, 2023

View reviewed changes

Intel modules adapter implementation #5848

Intel modules adapter implementation #5848

Uh oh!

Conversation

jxstelter commented May 23, 2022

Uh oh!

mwasko commented May 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh commented May 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh commented May 25, 2022

Uh oh!

dbaluta commented May 25, 2022

Uh oh!

lyakh commented May 25, 2022

Uh oh!

jxstelter commented May 25, 2022

Uh oh!

lgirdwood commented May 27, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jxstelter commented May 27, 2022

Uh oh!

lgirdwood commented May 30, 2022

Uh oh!

jxstelter commented May 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!