modules: Remove unused code and variables #8897
Conversation
softwarecki
requested review from
lyakh,
lgirdwood,
kv2019i,
marcinszkudlinski,
jxstelter and
pjdobrowolski
softwarecki
force-pushed
the
modules_cleanup
branch
2 times, most recently
from
7413498 to
62fbba6
Compare
softwarecki
requested review from
plbossart,
mmaka1,
lbetlej and
dbaluta
as code owners
|
@softwarecki what is |
We currently have three types of loadable modules...
|
There was a problem hiding this comment.
Seems ok. I'd appreciate if @pjdobrowolski , @lyakh and others working with the loadable modules could review this before we go ahead and merge.
| @@ -63,6 +63,7 @@ static int modules_init(struct processing_module *mod) | |||
| struct comp_dev *dev = mod->dev; | |||
| const struct ipc4_base_module_cfg *src_cfg = &md->cfg.base_cfg; | |||
| byte_array_t mod_cfg; | |||
| bool is_native_sof = false; | |||
There was a problem hiding this comment.
@softwarecki I don't think we have really explained "FDK" in SOF context, so following the git commit messages is a bit challenging. Correct Adrian me if I'm wrong, but in this context "FDK modules" is same as IADK modules which we have explained and documented in https://thesofproject.github.io/latest/architectures/firmware/intel/ace/iadk_modules.html .
So essentially, when you have native SOF loadable modules that don't depend on Intel IADK, the IADK specific code is not needed and code for native modules can be simplified like done in this PR.
I do find the language around "adapter"'s confusing. We have had adapters both between old SOF component.h interface and the new module interface. But we also have adapters between specialized interfaces (like Intel's IADK) and the module interface. This is not incorrect, but makes harder to follow changes like this PR.
There was a problem hiding this comment.
@softwarecki please explain: currently modules_init() performs the mapping of the memory for the module and the copying. How do you get rid of that? Do you work from the IMR directly? Let me mark this for "changes" until this is clarified
There was a problem hiding this comment.
@kv2019i Yes, i mean IADK modules. FDK modules was rebranded to IADK.
@lyakh The modules_init() function is still used, called by Module Adapter. After detecting that it is not an IADK module, the pointer to module_interface is overwritten and points to the interface provided by a loaded module. In this way, all other functions of the Processing Module Adapter are omitted.
There was a problem hiding this comment.
@softwarecki I think you're right, thanks!
There was a problem hiding this comment.
@softwarecki but then - doesn't that mean, that lib_manager_free_module() is never called?
There was a problem hiding this comment.
ok, I think it is a problem, that operations are completely overwritten and .free() isn't called any more. But it also looks like that was introduced earlier by a082752 . I'll block this PR for now, until we clarify or fix this.
There was a problem hiding this comment.
@lyakh: As you noticed, this PR does not introduce this change, so I see no reason to block it and thus postpone the next PR that will solve the described issue. I have already prepared the necessary changes and we are waiting for the current PRs to be merged.
There was a problem hiding this comment.
@softwarecki well, I don't see a fix among currently submitted PRs and I think the offending commit should be reverted as per #8923 instead of being further deepened. Let's revert that commit and then we can base further work on modifying the shim on top of a working state.
| @@ -63,6 +63,7 @@ static int modules_init(struct processing_module *mod) | |||
| struct comp_dev *dev = mod->dev; | |||
| const struct ipc4_base_module_cfg *src_cfg = &md->cfg.base_cfg; | |||
| byte_array_t mod_cfg; | |||
| bool is_native_sof = false; | |||
There was a problem hiding this comment.
@softwarecki please explain: currently modules_init() performs the mapping of the memory for the module and the copying. How do you get rid of that? Do you work from the IMR directly? Let me mark this for "changes" until this is clarified
softwarecki
force-pushed
the
modules_cleanup
branch
from
62fbba6 to
3ae92b4
Compare
| @@ -63,6 +63,7 @@ static int modules_init(struct processing_module *mod) | |||
| struct comp_dev *dev = mod->dev; | |||
| const struct ipc4_base_module_cfg *src_cfg = &md->cfg.base_cfg; | |||
| byte_array_t mod_cfg; | |||
| bool is_native_sof = false; | |||
There was a problem hiding this comment.
ok, I think it is a problem, that operations are completely overwritten and .free() isn't called any more. But it also looks like that was introduced earlier by a082752 . I'll block this PR for now, until we clarify or fix this.
softwarecki
force-pushed
the
modules_cleanup
branch
2 times, most recently
from
b3e5d75 to
a207823
Compare
lyakh
dismissed
their stale review
we understand the .free() problem rather well now and have a couple of potential solutions for it.
To ensure proper operation of native loadable modules it is necessary to bypass Processing Module Adapter used by IADK modules. This is currently done by overriding the pointer to module_interface used by the Module Adapter. Thanks to this, the Module Adapter directly calls functions provided by native module. As in this case the Processing Module Adapter functions are omitted, support for native libraries are removed from it as it is no longer needed. This leads to remove modules_process_raw and modules_process_audio_stream functions. Signed-off-by: Adrian Warecki <[email protected]>
The code allocating/freeing in_buff and out_buff buffers, which were not used, was removed from the processing module adapter. Signed-off-by: Adrian Warecki <[email protected]>
softwarecki
force-pushed
the
modules_cleanup
branch
from
a207823 to
745abdc
Compare
The value stored by the Processing Module Adapter in the module_adapter field of module_data structure has been moved to an unused private field. This change allowed the module_adapter field to be removed. Signed-off-by: Adrian Warecki <[email protected]>
The unused sys_service field has been removed from the processing_module structure. It was never initialized anywhere, and its value was passed as a parameter to the native_system_agent_start function which did not use it. Signed-off-by: Adrian Warecki <[email protected]>
The value assigned to the module_entry_point field in the module_data structure wasn't used anywhere. This field has been removed. Signed-off-by: Adrian Warecki <[email protected]>
softwarecki
force-pushed
the
modules_cleanup
branch
from
745abdc to
d5fe8be
Compare
Removed unnecessary functions from Processing Module Adapter. To ensure proper operation of native loadable modules it is necessary to bypass Processing Module Adapter used by FDK modules. This is currently done by overriding the pointer to module_interface used by the Module Adapter. Thanks to this, the Module Adapter directly calls functions provided by native module. As in this case the Processing Module Adapter functions are omitted, support for native libraries are removed from it as
it is no longer needed. This leads to remove is_native_sof variable and modules_process_raw, modules_process_audio_stream functions.
The code allocating/freeing in_buff and out_buff buffers, which were not used, was removed from the processing module adapter.
The value stored by the Processing Module Adapter in the module_adapter field of module_data structure has been moved to an unused private field. This change allowed the module_adapter field to be removed.
The unused sys_service field has been removed from the processing_module structure. It was never initialized anywhere, and its value was passed as a parameter to the native_system_agent_start function which did not use it.
The value assigned to the module_entry_point field in the module_data structure wasn't used anywhere. This field has been removed.