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

config: enable RTC_AEC module for LNL and PTL platforms #9730

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tmleman
Copy link
Contributor

@tmleman tmleman commented Dec 17, 2024

This patch enables the Google Real Time Communication Audio Processing (RTC_AEC) module for the LNL (Lunar Lake) and PTL (Panther Lake) platforms. The RTC_AEC module is essential for performing echo-cancelling and other real-time audio processing tasks.

Changes include:

  • Enabling CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING and CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK in the intel_adsp_ace20_lnl.conf configuration file.
  • Enabling CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING and CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK in the intel_adsp_ace30_ptl.conf configuration file.

This change ensures that the RTC_AEC module is included in the firmware builds for these platforms, allowing for proper testing and functionality of real-time audio processing features.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@lyakh could we make all the mocks into modules and then load then into CI via UUID ?

@lyakh
Copy link
Collaborator

lyakh commented Dec 19, 2024

@lyakh could we make all the mocks into modules and then load then into CI via UUID ?

@lgirdwood sure, but we'd need topologies that include those stub components to load and test them

@marcinszkudlinski
Copy link
Contributor

marcinszkudlinski commented Dec 19, 2024

@tmleman please compile AEC as a LLEXT loadable module, we don't need it in production.
@lyakh @lgirdwood MOCK is an integral part of AEC in case of testing and will be loaded together with AEC

EDIT: it may also require changes in testing, a module has to be loaded

@lyakh
Copy link
Collaborator

lyakh commented Dec 20, 2024

@tmleman please compile AEC as a LLEXT loadable module, we don't need it in production. @lyakh @lgirdwood MOCK is an integral part of AEC in case of testing and will be loaded together with AEC

EDIT: it may also require changes in testing, a module has to be loaded

@marcinszkudlinski so it is run-time tested in QB?

@marcinszkudlinski
Copy link
Contributor

@tmleman please compile AEC as a LLEXT loadable module, we don't need it in production. @lyakh @lgirdwood MOCK is an integral part of AEC in case of testing and will be loaded together with AEC
EDIT: it may also require changes in testing, a module has to be loaded

@marcinszkudlinski so it is run-time tested in QB?

test for AEC + Mock is included in internal CI tests

@tmleman tmleman force-pushed the topic/upstream/pr/intel/adsp/ace30/google_rtc branch from 9965610 to 4f70175 Compare January 7, 2025 11:01
@tmleman tmleman marked this pull request as ready for review January 7, 2025 15:25
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

I'd swap the commits - first convert to "m" where already enabled, and then add directly as "m" where not enabled yet, but the result is the same

@lgirdwood
Copy link
Member

@tmleman can you check Internal CI thanks !

@tmleman
Copy link
Contributor Author

tmleman commented Jan 9, 2025

@lgirdwood PR must wait until the required changes in the tests are implemented. Validation is working on it.

@tmleman tmleman force-pushed the topic/upstream/pr/intel/adsp/ace30/google_rtc branch 2 times, most recently from 727d836 to 8a77adf Compare January 22, 2025 12:03
@lgirdwood
Copy link
Member

@lgirdwood PR must wait until the required changes in the tests are implemented. Validation is working on it

@tmleman I assume we have a valid test implemented now and its being run in CI ?

@softwarecki
Copy link
Collaborator

softwarecki commented Jan 30, 2025

This module had an incorrect byte order in uuid, this was fixed by #9793. For me its good to merge.

@kv2019i
Copy link
Collaborator

kv2019i commented Feb 3, 2025

@tmleman This has one quickbuild failing. Otherwise looks good for merge.

@lgirdwood
Copy link
Member

@tmleman This has one quickbuild failing. Otherwise looks good for merge.

@tmleman ping

@tmleman tmleman force-pushed the topic/upstream/pr/intel/adsp/ace30/google_rtc branch from 24dbb6b to 8048e70 Compare February 17, 2025 08:57
@tmleman
Copy link
Contributor Author

tmleman commented Feb 17, 2025

@lgirdwood Previously, MTL CI failed in the RTC test due to an incorrect UUID. The problem was fixed by @softwarecki recent changes. Currently, the same test fails during module initialization:

[    0.692421] <inf> ipc: ipc_cmd: rx	: 0x40005000|0x12010026
[    0.692775] <wrn> ipc: ipc4_get_drv: get_drv(): the provided UUID (b780a0a6-269f-466f-b477-23dfa05af758) can't be found!
[    0.695606] <wrn> llext: llext_link_plt: PLT: cannot find idx 53 name ipc4_update_source_format
[    0.697148] <wrn> llext: llext_link_plt: PLT: cannot find idx 39 name comp_is_current_data_blob_valid
[    0.701273] <wrn> llext: llext_link_plt: PLT: cannot find idx 53 name ipc4_update_source_format
[    0.710821] <wrn> llext: llext_link_plt: PLT: cannot find idx 39 name comp_is_current_data_blob_valid
[    0.719726] <inf> llext: llext_load: Loaded extension RTC_AEC
[    0.721906] <err> lib_manager: lib_manager_module_create: lib_manager_allocate_module() failed!
[    0.721948] <err> ipc: ipc4_init_module_instance: error: failed to init module 5000 : 0
[    0.721991] <err> ipc: ipc_cmd: ipc4: MODULE_MSG failed with err 104
[    0.956415] <inf> ipc: ipc_cmd: rx	: 0x12010000|0x0

@lgirdwood
Copy link
Member

@lgirdwood Previously, MTL CI failed in the RTC test due to an incorrect UUID. The problem was fixed by @softwarecki recent changes. Currently, the same test fails during module initialization:

[    0.692421] <inf> ipc: ipc_cmd: rx	: 0x40005000|0x12010026
[    0.692775] <wrn> ipc: ipc4_get_drv: get_drv(): the provided UUID (b780a0a6-269f-466f-b477-23dfa05af758) can't be found!
[    0.695606] <wrn> llext: llext_link_plt: PLT: cannot find idx 53 name ipc4_update_source_format
[    0.697148] <wrn> llext: llext_link_plt: PLT: cannot find idx 39 name comp_is_current_data_blob_valid
[    0.701273] <wrn> llext: llext_link_plt: PLT: cannot find idx 53 name ipc4_update_source_format
[    0.710821] <wrn> llext: llext_link_plt: PLT: cannot find idx 39 name comp_is_current_data_blob_valid
[    0.719726] <inf> llext: llext_load: Loaded extension RTC_AEC
[    0.721906] <err> lib_manager: lib_manager_module_create: lib_manager_allocate_module() failed!
[    0.721948] <err> ipc: ipc4_init_module_instance: error: failed to init module 5000 : 0
[    0.721991] <err> ipc: ipc_cmd: ipc4: MODULE_MSG failed with err 104
[    0.956415] <inf> ipc: ipc_cmd: rx	: 0x12010000|0x0

@lyakh do we need a west update ?

lyakh
lyakh previously requested changes Feb 18, 2025
@lyakh
Copy link
Collaborator

lyakh commented Feb 18, 2025

@lgirdwood Previously, MTL CI failed in the RTC test due to an incorrect UUID. The problem was fixed by @softwarecki recent changes. Currently, the same test fails during module initialization:

[    0.692421] <inf> ipc: ipc_cmd: rx	: 0x40005000|0x12010026
[    0.692775] <wrn> ipc: ipc4_get_drv: get_drv(): the provided UUID (b780a0a6-269f-466f-b477-23dfa05af758) can't be found!
[    0.695606] <wrn> llext: llext_link_plt: PLT: cannot find idx 53 name ipc4_update_source_format
[    0.697148] <wrn> llext: llext_link_plt: PLT: cannot find idx 39 name comp_is_current_data_blob_valid
[    0.701273] <wrn> llext: llext_link_plt: PLT: cannot find idx 53 name ipc4_update_source_format
[    0.710821] <wrn> llext: llext_link_plt: PLT: cannot find idx 39 name comp_is_current_data_blob_valid
[    0.719726] <inf> llext: llext_load: Loaded extension RTC_AEC
[    0.721906] <err> lib_manager: lib_manager_module_create: lib_manager_allocate_module() failed!
[    0.721948] <err> ipc: ipc4_init_module_instance: error: failed to init module 5000 : 0
[    0.721991] <err> ipc: ipc_cmd: ipc4: MODULE_MSG failed with err 104
[    0.956415] <inf> ipc: ipc_cmd: rx	: 0x12010000|0x0

@lyakh do we need a west update ?

@lgirdwood I suppose this is an excerpt from an Internal CI log? I don't think there's anything relevant in recent Zephyr changes, that would fix this, and we don't have a topology with this module, so I cannot test, sorry. But since it's a proper reproducible error with a full log, it should be rather easy to debug

@tmleman tmleman force-pushed the topic/upstream/pr/intel/adsp/ace30/google_rtc branch from 8048e70 to 77b1587 Compare February 19, 2025 11:39
@lyakh lyakh dismissed their stale review February 20, 2025 08:40

offending hunk removed

@lyakh
Copy link
Collaborator

lyakh commented Feb 20, 2025

This is already good, thanks, but not immediately approving yet before I ask: would it be possible to just enable CONFIG_COMP_STUBS by default on all ACE? Any objections to that? @kv2019i @lgirdwood

@lgirdwood
Copy link
Member

This is already good, thanks, but not immediately approving yet before I ask: would it be possible to just enable CONFIG_COMP_STUBS by default on all ACE? Any objections to that? @kv2019i @lgirdwood

Do you mean build and load all stubs into DRAM for testing/CI reasons ?

@tmleman tmleman force-pushed the topic/upstream/pr/intel/adsp/ace30/google_rtc branch from 77b1587 to 17590eb Compare February 20, 2025 11:22
@tmleman tmleman requested a review from abonislawski February 20, 2025 11:25
@kv2019i
Copy link
Collaborator

kv2019i commented Feb 20, 2025

@lyakh wrote:

This is already good, thanks, but not immediately approving yet before I ask: would it be possible to just enable CONFIG_COMP_STUBS by default on all ACE? Any objections to that? @kv2019i @lgirdwood

Do you mean build and load all stubs into DRAM for testing/CI reasons ?

We (still) only have one config that we use for CI and what we release. So if stubs are enabled by default, they'll be in CI and in sof-bin releases.

This patch exports the symbols `comp_is_current_data_blob_valid` and
`ipc4_update_source_format` to resolve issues encountered when the
Google RTC module was converted into an LLEXT module. The lack of
exported symbols was causing warnings and subsequent firmware exceptions
when these functions were called from within the module.

Changes include:
- Adding `EXPORT_SYMBOL` for `comp_is_current_data_blob_valid` in
  src/audio/data_blob.c.
- Adding `EXPORT_SYMBOL` for `ipc4_update_source_format` in
  src/ipc/ipc4/helper.c.

Signed-off-by: Tomasz Leman <[email protected]>
This patch enables the Google Real Time Communication Audio Processing
(RTC_AEC) module for the LNL (Lunar Lake) and PTL (Panther Lake)
platforms. The RTC_AEC module is essential for performing
echo-cancelling and other real-time audio processing tasks.

Changes include:
- Enabling CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING and
  CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK in the intel_adsp_ace20_lnl.conf
  configuration file.
- Enabling CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING and
  CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK in the intel_adsp_ace30_ptl.conf
  configuration file.

This change ensures that the RTC_AEC module is included in the firmware
builds for these platforms, allowing for proper testing and
functionality of real-time audio processing features.

Signed-off-by: Tomasz Leman <[email protected]>
This patch modifies the configuration to change the Google Real Time
Communication Audio Processing (RTC_AEC) module from built-in to a
loadable module for the MTPM, LNL, and PTL platforms. This change allows
for more flexibility in managing the module and reduces the firmware
size.

Changes include:
- Changing CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING from 'y' to 'm' in
  the intel_adsp_ace15_mtpm.conf configuration file.
- Changing CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING from 'y' to 'm' in
  the intel_adsp_ace20_lnl.conf configuration file.
- Changing CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING from 'y' to 'm' in
  the intel_adsp_ace30_ptl.conf configuration file.
- Adding CONFIG_LLEXT, CONFIG_LLEXT_STORAGE_WRITABLE, and CONFIG_MODULES
  to the intel_adsp_ace30_ptl.conf configuration file to support loadable
  modules.

This change ensures that the RTC_AEC module can be dynamically loaded as
needed, providing greater flexibility and potentially improving system
performance.

Signed-off-by: Tomasz Leman <[email protected]>
@tmleman tmleman force-pushed the topic/upstream/pr/intel/adsp/ace30/google_rtc branch from 17590eb to 90aaeab Compare February 21, 2025 06:40
@lyakh
Copy link
Collaborator

lyakh commented Feb 21, 2025

@lyakh wrote:

This is already good, thanks, but not immediately approving yet before I ask: would it be possible to just enable CONFIG_COMP_STUBS by default on all ACE? Any objections to that? @kv2019i @lgirdwood

Do you mean build and load all stubs into DRAM for testing/CI reasons ?

We (still) only have one config that we use for CI and what we release. So if stubs are enabled by default, they'll be in CI and in sof-bin releases.

This PR already enables one of them - CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK. For many of the third party modules we have 3 options: disable, enable as a mock, using our in-tree stub, or enable using the actual third party library. Obviously we cannot enable the third option because we don't have those libraries. So we can either disable those modules or enable them as mocks. This PR enables one of them as a mock in our platform configurations for LNL and PTL. What I'm asking about is whether it would be easier and better for our testing coverage to enable CONFIG_COMP_STUBS instead which would then enable all third party modules in mock mode, including this one:

config GOOGLE_RTC_AUDIO_PROCESSING_MOCK
	bool "Google Real Time Communication Audio processing mock"
	default y if COMP_STUBS

And yes, @lgirdwood this should build them as modules which then will not be included in the openmodules library, then @kv2019i we should be easily able to avoid including them in our releases?

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.

7 participants