From 43ff9f7f0c22a5fa72f65b60ccab1130cb0a9200 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Wed, 3 Jan 2024 11:58:23 -0800 Subject: [PATCH 01/10] Revert "[DO NOT UPSTREAM] re-introduce accidently removed bu size code" This reverts commit f24e9664bea18f50e8ee8f002334a807d39bd45d. --- src/ipc/ipc4/helper.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/ipc/ipc4/helper.c b/src/ipc/ipc4/helper.c index 2b80e62d5a5f..51ded55943b6 100644 --- a/src/ipc/ipc4/helper.c +++ b/src/ipc/ipc4/helper.c @@ -539,18 +539,8 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) ibs = sink_src_cfg.ibs; } - /* create a buffer - * in case of LL -> LL or LL->DP - * size = 2*obs of source module (obs is single buffer size) - * in case of DP -> LL - * size = 2*ibs of destination (LL) module. DP queue will handle obs of DP module - */ - if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL) - buf_size = source_src_cfg.obs * 2; - else - buf_size = sink_src_cfg.ibs * 2; - - + /* allocate buffer with size large enough to fit ibs of the sink or obs of the source */ + buf_size = MAX(ibs * 2, obs * 2); buffer = ipc4_create_buffer(source, cross_core_bind, buf_size, bu->extension.r.src_queue, bu->extension.r.dst_queue); if (!buffer) { From d206a13e5db8ef93f17b7a5201e23b5652ddfb36 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Wed, 3 Jan 2024 11:58:30 -0800 Subject: [PATCH 02/10] Revert "module: Add support for saving and retrieving the base config extension" This reverts commit f9f0ce3c02c4119bf008f9bd8e9f0784831fa9ff. --- .../google/google_rtc_audio_processing.c | 32 +----- .../module_adapter/module_adapter_ipc4.c | 16 --- src/include/module/module/base.h | 1 - src/include/sof/audio/component.h | 1 - src/include/sof/audio/component_ext.h | 53 ++------- .../sof/audio/module_adapter/module/generic.h | 1 - src/ipc/ipc4/helper.c | 106 ++++-------------- src/samples/audio/smart_amp_test_ipc4.c | 17 --- 8 files changed, 40 insertions(+), 187 deletions(-) diff --git a/src/audio/google/google_rtc_audio_processing.c b/src/audio/google/google_rtc_audio_processing.c index d7c9cc489ff8..2a045dea4e04 100644 --- a/src/audio/google/google_rtc_audio_processing.c +++ b/src/audio/google/google_rtc_audio_processing.c @@ -306,36 +306,19 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) struct module_config *cfg = &md->cfg; const struct ipc4_base_module_extended_cfg *base_cfg = md->cfg.init_data; struct ipc4_input_pin_format reference_fmt, output_fmt; - size_t in_fmt_size = sizeof(struct ipc4_input_pin_format); - size_t out_fmt_size = sizeof(struct ipc4_output_pin_format); - size_t size; + const size_t size = sizeof(struct ipc4_input_pin_format); cd->config.base_cfg = base_cfg->base_cfg; /* Copy the reference format from input pin 1 format */ - memcpy_s(&reference_fmt, in_fmt_size, - &base_cfg->base_cfg_ext.pin_formats[in_fmt_size], in_fmt_size); - memcpy_s(&output_fmt, in_fmt_size, - &base_cfg->base_cfg_ext.pin_formats[in_fmt_size * GOOGLE_RTC_NUM_INPUT_PINS], - in_fmt_size); + memcpy_s(&reference_fmt, size, + &base_cfg->base_cfg_ext.pin_formats[size], size); + memcpy_s(&output_fmt, size, + &base_cfg->base_cfg_ext.pin_formats[size * GOOGLE_RTC_NUM_INPUT_PINS], size); cd->config.reference_fmt = reference_fmt.audio_fmt; cd->config.output_fmt = output_fmt.audio_fmt; - - /* save the base config extension */ - size = base_cfg->base_cfg_ext.nb_input_pins * in_fmt_size + - base_cfg->base_cfg_ext.nb_output_pins * out_fmt_size; - - md->cfg.basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, - sizeof(struct ipc4_base_module_cfg_ext) + size); - if (!md->cfg.basecfg_ext) { - ret = -ENOMEM; - goto fail; - } - - memcpy_s(md->cfg.basecfg_ext, size + sizeof(struct ipc4_base_module_cfg_ext), - &base_cfg->base_cfg_ext, - size + sizeof(struct ipc4_base_module_cfg_ext)); + cd->config = *(const struct sof_ipc4_aec_config *)cfg->init_data; cd->tuning_handler = comp_data_blob_handler_new(dev); if (!cd->tuning_handler) { @@ -433,7 +416,6 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) } GoogleRtcAudioProcessingDetachMemoryBuffer(); rfree(cd->memory_buffer); - rfree(md->cfg.basecfg_ext); rfree(cd->process_buffer); comp_data_blob_handler_free(cd->tuning_handler); rfree(cd); @@ -445,7 +427,6 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) static int google_rtc_audio_processing_free(struct processing_module *mod) { struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); - struct module_data *md = &mod->priv; comp_dbg(mod->dev, "google_rtc_audio_processing_free()"); @@ -456,7 +437,6 @@ static int google_rtc_audio_processing_free(struct processing_module *mod) rfree(cd->memory_buffer); rfree(cd->process_buffer); comp_data_blob_handler_free(cd->tuning_handler); - rfree(md->cfg.basecfg_ext); rfree(cd); return 0; } diff --git a/src/audio/module_adapter/module_adapter_ipc4.c b/src/audio/module_adapter/module_adapter_ipc4.c index 62ca58ffcdf6..1e0257b0a1cb 100644 --- a/src/audio/module_adapter/module_adapter_ipc4.c +++ b/src/audio/module_adapter/module_adapter_ipc4.c @@ -147,22 +147,6 @@ int module_adapter_get_attribute(struct comp_dev *dev, uint32_t type, void *valu memcpy_s(value, sizeof(struct ipc4_base_module_cfg), &mod->priv.cfg.base_cfg, sizeof(mod->priv.cfg.base_cfg)); break; - case COMP_ATTR_BASE_CONFIG_EXT: - { - struct ipc4_base_module_cfg_ext *basecfg_ext = mod->priv.cfg.basecfg_ext; - size_t size; - - if (!basecfg_ext) { - comp_err(mod->dev, "No base config extn set for module"); - return -EINVAL; - } - - size = basecfg_ext->nb_input_pins * sizeof(struct ipc4_input_pin_format) + - basecfg_ext->nb_output_pins * sizeof(struct ipc4_output_pin_format); - memcpy_s(value, sizeof(*basecfg_ext) + size, - basecfg_ext, sizeof(*basecfg_ext) + size); - break; - } default: return -EINVAL; } diff --git a/src/include/module/module/base.h b/src/include/module/module/base.h index 1fd868e0d047..904b83741961 100644 --- a/src/include/module/module/base.h +++ b/src/include/module/module/base.h @@ -30,7 +30,6 @@ struct module_config { const void *init_data; /**< Initial IPC configuration. */ #if CONFIG_IPC_MAJOR_4 struct ipc4_base_module_cfg base_cfg; - struct ipc4_base_module_cfg_ext *basecfg_ext; #endif }; diff --git a/src/include/sof/audio/component.h b/src/include/sof/audio/component.h index daafb8a5bb98..3896d2f55065 100644 --- a/src/include/sof/audio/component.h +++ b/src/include/sof/audio/component.h @@ -117,7 +117,6 @@ enum { #define COMP_ATTR_COPY_DIR 2 /**< Comp copy direction */ #define COMP_ATTR_VDMA_INDEX 3 /**< Comp index of the virtual DMA at the gateway. */ #define COMP_ATTR_BASE_CONFIG 4 /**< Component base config */ -#define COMP_ATTR_BASE_CONFIG_EXT 5 /**< Component base config extension */ /** @}*/ /** \name Trace macros diff --git a/src/include/sof/audio/component_ext.h b/src/include/sof/audio/component_ext.h index d7abce7b3d94..a3d04843b3f9 100644 --- a/src/include/sof/audio/component_ext.h +++ b/src/include/sof/audio/component_ext.h @@ -16,7 +16,6 @@ #ifndef __SOF_AUDIO_COMPONENT_INT_H__ #define __SOF_AUDIO_COMPONENT_INT_H__ -#include #include #include @@ -217,59 +216,31 @@ struct get_attribute_remote_payload { static inline int comp_ipc4_get_attribute_remote(struct comp_dev *dev, uint32_t type, void *value) { - struct ipc4_base_module_cfg_ext *basecfg_ext; struct ipc4_base_module_cfg *base_cfg; struct get_attribute_remote_payload payload = {}; struct idc_msg msg = { IDC_MSG_GET_ATTRIBUTE, IDC_EXTENSION(dev->ipc_config.id), dev->ipc_config.core, sizeof(payload), &payload}; - size_t size = MODULE_MAX_SINKS * sizeof(struct ipc4_output_pin_format) + - MODULE_MAX_SOURCES * sizeof(struct ipc4_input_pin_format); int ret; - payload.type = type; + /* Only COMP_ATTR_BASE_CONFIG is supported for remote access */ + if (type != COMP_ATTR_BASE_CONFIG) + return -EINVAL; - /* - * Only COMP_ATTR_BASE_CONFIG and COMP_ATTR_BASE_CONFIG_EXT are supported for - * remote access - */ - switch (type) { - case COMP_ATTR_BASE_CONFIG: - base_cfg = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, - sizeof(*base_cfg)); - if (!base_cfg) - return -ENOMEM; - - payload.value = base_cfg; - break; - case COMP_ATTR_BASE_CONFIG_EXT: - basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, - sizeof(*basecfg_ext) + size); - if (!basecfg_ext) - return -ENOMEM; + base_cfg = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*base_cfg)); + if (!base_cfg) + return -ENOMEM; - payload.value = basecfg_ext; - break; - default: - return -EINVAL; - } + payload.type = type; + payload.value = base_cfg; ret = idc_send_msg(&msg, IDC_BLOCKING); - if (!ret) { - switch (type) { - case COMP_ATTR_BASE_CONFIG: - memcpy_s(value, sizeof(struct ipc4_base_module_cfg), - base_cfg, sizeof(struct ipc4_base_module_cfg)); - rfree(base_cfg); - break; - case COMP_ATTR_BASE_CONFIG_EXT: - memcpy_s(value, size, basecfg_ext, size); - rfree(basecfg_ext); - break; - } - } + if (ret == 0) + memcpy_s(value, sizeof(struct ipc4_base_module_cfg), + base_cfg, sizeof(struct ipc4_base_module_cfg)); + rfree(base_cfg); return ret; } #endif /* CONFIG_IPC_MAJOR_4 */ diff --git a/src/include/sof/audio/module_adapter/module/generic.h b/src/include/sof/audio/module_adapter/module/generic.h index 492e9cc927f9..3e587117210c 100644 --- a/src/include/sof/audio/module_adapter/module/generic.h +++ b/src/include/sof/audio/module_adapter/module/generic.h @@ -31,7 +31,6 @@ #define MAX_BLOB_SIZE 8192 #define MODULE_MAX_SOURCES 8 -#define MODULE_MAX_SINKS 8 #define API_CALL(cd, cmd, sub_cmd, value, ret) \ do { \ diff --git a/src/ipc/ipc4/helper.c b/src/ipc/ipc4/helper.c index 51ded55943b6..4006ce82be8d 100644 --- a/src/ipc/ipc4/helper.c +++ b/src/ipc/ipc4/helper.c @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include @@ -425,7 +424,6 @@ static int ll_wait_finished_on_core(struct comp_dev *dev) int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) { - struct ipc4_base_module_cfg_ext *src_basecfg_ext, *sink_basecfg_ext; struct ipc4_module_bind_unbind *bu; struct comp_buffer *buffer; struct comp_dev *source; @@ -433,12 +431,8 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) struct ipc4_base_module_cfg source_src_cfg; struct ipc4_base_module_cfg sink_src_cfg; uint32_t flags; - uint32_t ibs = 0; - uint32_t obs = 0; - uint32_t buf_size; int src_id, sink_id; int ret; - size_t size; bu = (struct ipc4_module_bind_unbind *)_connect; src_id = IPC4_COMP_ID(bu->primary.r.module_id, bu->primary.r.instance_id); @@ -459,88 +453,32 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) if (!cpu_is_me(source->ipc_config.core) && !cross_core_bind) return ipc4_process_on_core(source->ipc_config.core, false); - size = MODULE_MAX_SINKS * sizeof(struct ipc4_output_pin_format) + - MODULE_MAX_SOURCES * sizeof(struct ipc4_input_pin_format); - - /* get obs from the base config extension if the src queue ID is non-zero */ - if (bu->extension.r.src_queue) { - struct ipc4_output_pin_format *out_fmt; - size_t output_fmt_offset; - - src_basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, - sizeof(*src_basecfg_ext) + size); - if (!src_basecfg_ext) - return IPC4_OUT_OF_MEMORY; - - ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG_EXT, src_basecfg_ext); - if (!ret) { - size_t in_fmt_size = sizeof(struct ipc4_input_pin_format); - size_t out_fmt_size = sizeof(struct ipc4_output_pin_format); - - /* - * offset of the format for the output pin with ID 'src_queue' within the - * base config extension. - */ - output_fmt_offset = - src_basecfg_ext->nb_input_pins * in_fmt_size + - bu->extension.r.src_queue * out_fmt_size; - out_fmt = (struct ipc4_output_pin_format *)&sink_basecfg_ext->pin_formats[output_fmt_offset]; - obs = out_fmt->obs; - } - rfree(src_basecfg_ext); - } - - /* get obs from base config if src queue ID is 0 or if base config extn is missing */ - if (!obs) { - /* these might call comp_ipc4_get_attribute_remote() if necessary */ - ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg); - - if (ret < 0) { - tr_err(&ipc_tr, "failed to get base config for src module %#x", - dev_comp_id(source)); - return IPC4_FAILURE; - } - obs = source_src_cfg.obs; + /* these might call comp_ipc4_get_attribute_remote() if necessary */ + ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg); + if (ret < 0) { + tr_err(&ipc_tr, "failed to get base config for module %#x", dev_comp_id(source)); + return IPC4_FAILURE; } - /* get ibs from the base config extension if the sink queue ID is non-zero */ - if (bu->extension.r.dst_queue) { - struct ipc4_input_pin_format *in_fmt; - size_t input_fmt_offset; - - sink_basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, - sizeof(*sink_basecfg_ext) + size); - if (!sink_basecfg_ext) - return IPC4_OUT_OF_MEMORY; - - ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG_EXT, sink_basecfg_ext); - if (!ret) { - /* - * offset of the format for the input pin with ID 'dst_queue' within the - * base config extension. - */ - input_fmt_offset = - bu->extension.r.dst_queue * sizeof(struct ipc4_input_pin_format); - in_fmt = (struct ipc4_input_pin_format *)&sink_basecfg_ext->pin_formats[input_fmt_offset]; - ibs = in_fmt->ibs; - } - rfree(sink_basecfg_ext); + ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG, &sink_src_cfg); + if (ret < 0) { + tr_err(&ipc_tr, "failed to get base config for module %#x", dev_comp_id(sink)); + return IPC4_FAILURE; } - /* get ibs from base config if sink queue ID is 0 or if base config extn is missing */ - if (!ibs) { - ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG, &sink_src_cfg); - if (ret < 0) { - tr_err(&ipc_tr, "failed to get base config for sink module %#x", - dev_comp_id(sink)); - return IPC4_FAILURE; - } + /* create a buffer + * in case of LL -> LL or LL->DP + * size = 2*obs of source module (obs is single buffer size) + * in case of DP -> LL + * size = 2*ibs of destination (LL) module. DP queue will handle obs of DP module + */ + uint32_t buf_size; - ibs = sink_src_cfg.ibs; - } + if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL) + buf_size = source_src_cfg.obs * 2; + else + buf_size = sink_src_cfg.ibs * 2; - /* allocate buffer with size large enough to fit ibs of the sink or obs of the source */ - buf_size = MAX(ibs * 2, obs * 2); buffer = ipc4_create_buffer(source, cross_core_bind, buf_size, bu->extension.r.src_queue, bu->extension.r.dst_queue); if (!buffer) { @@ -558,8 +496,8 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) * sink_module needs to set its IBS (input buffer size) * as min_available in buffer's source ifc */ - sink_set_min_free_space(audio_stream_get_sink(&buffer->stream), obs); - source_set_min_available(audio_stream_get_source(&buffer->stream), ibs); + sink_set_min_free_space(audio_stream_get_sink(&buffer->stream), source_src_cfg.obs); + source_set_min_available(audio_stream_get_source(&buffer->stream), sink_src_cfg.ibs); /* * Connect and bind the buffer to both source and sink components with LL processing been diff --git a/src/samples/audio/smart_amp_test_ipc4.c b/src/samples/audio/smart_amp_test_ipc4.c index 31924781e666..f6ffd6dae115 100644 --- a/src/samples/audio/smart_amp_test_ipc4.c +++ b/src/samples/audio/smart_amp_test_ipc4.c @@ -66,13 +66,9 @@ static struct smart_amp_data smart_amp_priv; static int smart_amp_init(struct processing_module *mod) { struct smart_amp_data *sad; - struct comp_dev *dev = mod->dev; struct module_data *mod_data = &mod->priv; - const size_t in_size = sizeof(struct ipc4_input_pin_format) * SMART_AMP_NUM_IN_PINS; - const size_t out_size = sizeof(struct ipc4_output_pin_format) * SMART_AMP_NUM_OUT_PINS; int ret; const struct ipc4_base_module_extended_cfg *base_cfg = mod_data->cfg.init_data; - size_t size; LOG_DBG("smart_amp_init()"); @@ -100,16 +96,6 @@ static int smart_amp_init(struct processing_module *mod) *(struct ipc4_output_pin_format *)(base_cfg->base_cfg_ext.pin_formats + sizeof(sad->ipc4_cfg.input_pins)); - /* save the base config extension */ - size = sizeof(struct ipc4_base_module_cfg_ext) + in_size + out_size; - mod_data->cfg.basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, size); - if (!mod_data->cfg.basecfg_ext) { - ret = -ENOMEM; - goto sad_fail; - } - - memcpy_s(mod_data->cfg.basecfg_ext, size, &base_cfg->base_cfg_ext, size); - return 0; sad_fail: @@ -169,10 +155,7 @@ static int smart_amp_free(struct processing_module *mod) #ifndef __SOF_MODULE_SERVICE_BUILD__ struct smart_amp_data *sad = module_get_private_data(mod); - struct module_data *mod_data = &mod->priv; - struct comp_dev *dev = mod->dev; - rfree(mod_data->cfg.basecfg_ext); rfree(sad); #endif From e6392376d9adf9ce212f0c6abc3e843a9a314d32 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Thu, 2 Mar 2023 11:25:59 -0800 Subject: [PATCH 03/10] zephyr/alloc: Add newlib-style allocator stub for C++ builds This is a weak stub for the Cadence libc's allocator (which is just a newlib build). It's traditionally been provided like this in SOF for the benefit of C++ code where the standard library needs to link to a working malloc() even if it will never call it. Longer term this should be integrated as a working allocator, either unified with the one here or in the Zephyr libc layer. Zephyr already provides a newlib-compatible _sbrk_r(), we just have to tell it to use it when linking against Cadence libc. Signed-off-by: Andy Ross --- zephyr/lib/alloc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/zephyr/lib/alloc.c b/zephyr/lib/alloc.c index f20e125e9246..4e03d02c7310 100644 --- a/zephyr/lib/alloc.c +++ b/zephyr/lib/alloc.c @@ -367,4 +367,16 @@ static int heap_init(void) return 0; } +/* This is a weak stub for the Cadence libc's allocator (which is just + * a newlib build). It's traditionally been provided like this in SOF + * for the benefit of C++ code where the standard library needs to + * link to a working malloc() even if it will never call it. + */ +struct _reent; +__weak void *_sbrk_r(struct _reent *ptr, ptrdiff_t incr) +{ + k_panic(); + return NULL; +} + SYS_INIT(heap_init, PRE_KERNEL_1, CONFIG_KERNEL_INIT_PRIORITY_OBJECTS); From 0285502522e7892f9ca3908f440abd43d71dc5c1 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Thu, 2 Mar 2023 11:28:34 -0800 Subject: [PATCH 04/10] zephyr: Update GOOGLE_RTC_AUDIO_PROCESSING Unbreak the Zephyr build when this is enabled, and add the needed bits to produce a working executable. This is mostly just a recapitulation of the existing integration, which means that it's manually pulling in bits from the Cadence toolchain it needs. SOF isn't yet using the Zephyr C++ integration (which isn't xt-clang aware yet), nor does it really want to as SOF itself includes no such code. Zephyr doesn't have a "C++ binary linkage only" feature yet. Signed-off-by: Andy Ross --- zephyr/CMakeLists.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/zephyr/CMakeLists.txt b/zephyr/CMakeLists.txt index 936e6e9daef8..505d382aaec5 100644 --- a/zephyr/CMakeLists.txt +++ b/zephyr/CMakeLists.txt @@ -797,6 +797,17 @@ zephyr_library_sources_ifdef(CONFIG_DW_DMA ${SOF_DRIVERS_PATH}/dw/dma.c ) +if(CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING AND NOT CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MOCK) + zephyr_include_directories(../third_party/include) + target_link_directories(SOF INTERFACE ../third_party/lib) + target_link_libraries(SOF INTERFACE google_rtc_audio_processing) + target_link_libraries(SOF INTERFACE c++) + target_link_libraries(SOF INTERFACE c++abi) + target_link_libraries(SOF INTERFACE m) + target_link_libraries(SOF INTERFACE c) + target_link_libraries(SOF INTERFACE gcc) +endif() + zephyr_library_link_libraries(SOF) target_link_libraries(SOF INTERFACE zephyr_interface) From fb33be307a7938c2547b98df4b13ddef0c9c0e8b Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Fri, 22 Dec 2023 17:11:30 -0800 Subject: [PATCH 05/10] audio_stream: Fix uninitialized data in alignment_constants Audio stream objects are generally allocated out of zero'd heap memory, but the computed alignment fields need non-zero values to have correct behavior. These were being left as garbage. Set them to a byte and frame alignmnt of 1, as this corresponds to pervasive convention in the tree. But note that a byte alignment of 1 is actually technically incorrect, as all existing DSP targets are on architectures which don't allow misaligned loads. But existing component code is universally coded correctly anyway. (It's worth pointing out that "set_alignment_constants()" is now exposed as an API call on abstracted source/sink objects. But the only current implementation is on audio_stream. Future implementations will need to correctly initialize themselves.) This is a partial fix for Issue #8639 Signed-off-by: Andy Ross --- src/audio/audio_stream.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/audio/audio_stream.c b/src/audio/audio_stream.c index 2a1a7a05e64a..468a37290764 100644 --- a/src/audio/audio_stream.c +++ b/src/audio/audio_stream.c @@ -141,6 +141,7 @@ void audio_stream_init(struct audio_stream *audio_stream, void *buff_addr, uint3 audio_stream->addr = buff_addr; audio_stream->end_addr = (char *)audio_stream->addr + size; + audio_stream_init_alignment_constants(1, 1, audio_stream); source_init(audio_stream_get_source(audio_stream), &audio_stream_source_ops, &audio_stream->runtime_stream_params); sink_init(audio_stream_get_sink(audio_stream), &audio_stream_sink_ops, From 928156a79463ba4a4e11eeee2aaa0613f71fa6d5 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Fri, 22 Dec 2023 17:17:00 -0800 Subject: [PATCH 06/10] various components: Remove default/stub init_alignment_constants usage Traditionally audio_stream has failed to initialize its computed alignment fields, forcing components to do this themselves even when they don't actually have special alignment requirements. Remove all the code that was merely setting default values, leaving only a handful of spots with specialr equirements (e.g. eq/area need to treat pairs of samples, a few others have HiFi-optimized variants which need SIMD alignment). Signed-off-by: Andy Ross --- src/audio/aria/aria.c | 4 +--- src/audio/asrc/asrc.c | 3 --- src/audio/crossover/crossover.c | 5 +---- src/audio/dcblock/dcblock.c | 13 ------------- src/audio/drc/drc.c | 9 --------- src/audio/kpb.c | 4 ---- src/audio/mfcc/mfcc.c | 12 ------------ src/audio/mixer/mixer.c | 11 +---------- src/audio/multiband_drc/multiband_drc.c | 10 ---------- src/audio/mux/mux.c | 8 -------- src/audio/volume/volume.c | 12 +----------- 11 files changed, 4 insertions(+), 87 deletions(-) diff --git a/src/audio/aria/aria.c b/src/audio/aria/aria.c index d338ae22743e..cd9046e61dea 100644 --- a/src/audio/aria/aria.c +++ b/src/audio/aria/aria.c @@ -162,9 +162,7 @@ static void aria_set_stream_params(struct comp_buffer *buffer, const struct ipc4_audio_format *audio_fmt = &mod->priv.cfg.base_cfg.audio_fmt; ipc4_update_buffer_format(buffer, audio_fmt); -#ifdef ARIA_GENERIC - audio_stream_init_alignment_constants(1, 1, &buffer->stream); -#else +#ifndef ARIA_GENERIC audio_stream_init_alignment_constants(8, 1, &buffer->stream); #endif } diff --git a/src/audio/asrc/asrc.c b/src/audio/asrc/asrc.c index 5e7eb0f3300a..3bd8679639fc 100644 --- a/src/audio/asrc/asrc.c +++ b/src/audio/asrc/asrc.c @@ -755,9 +755,6 @@ static int asrc_prepare(struct processing_module *mod, sinkb = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); - audio_stream_init_alignment_constants(1, 1, &sourceb->stream); - audio_stream_init_alignment_constants(1, 1, &sinkb->stream); - /* get source data format and period bytes */ cd->source_format = audio_stream_get_frm_fmt(&sourceb->stream); source_period_bytes = audio_stream_period_bytes(&sourceb->stream, diff --git a/src/audio/crossover/crossover.c b/src/audio/crossover/crossover.c index 4e89a9d05042..1f82b7625cc7 100644 --- a/src/audio/crossover/crossover.c +++ b/src/audio/crossover/crossover.c @@ -733,14 +733,11 @@ static int crossover_prepare(struct processing_module *mod, /* Get source data format */ cd->source_format = audio_stream_get_frm_fmt(&source->stream); channels = audio_stream_get_channels(&source->stream); - audio_stream_init_alignment_constants(1, 1, &source->stream); /* Validate frame format and buffer size of sinks */ list_for_item(sink_list, &dev->bsink_list) { sink = container_of(sink_list, struct comp_buffer, source_list); - if (cd->source_format == audio_stream_get_frm_fmt(&sink->stream)) { - audio_stream_init_alignment_constants(1, 1, &sink->stream); - } else { + if (cd->source_format != audio_stream_get_frm_fmt(&sink->stream)) { comp_err(dev, "crossover_prepare(): Source fmt %d and sink fmt %d are different.", cd->source_format, audio_stream_get_frm_fmt(&sink->stream)); ret = -EINVAL; diff --git a/src/audio/dcblock/dcblock.c b/src/audio/dcblock/dcblock.c index 8e52820341ba..dfde5591815d 100644 --- a/src/audio/dcblock/dcblock.c +++ b/src/audio/dcblock/dcblock.c @@ -206,17 +206,6 @@ static int dcblock_process(struct processing_module *mod, return 0; } -/* init and calculate the aligned setting for available frames and free frames retrieve*/ -static inline void dcblock_set_frame_alignment(struct audio_stream *source, - struct audio_stream *sink) -{ - const uint32_t byte_align = 1; - const uint32_t frame_align_req = 1; - - audio_stream_init_alignment_constants(byte_align, frame_align_req, source); - audio_stream_init_alignment_constants(byte_align, frame_align_req, sink); -} - #if CONFIG_IPC_MAJOR_4 static void dcblock_params(struct processing_module *mod) { @@ -266,8 +255,6 @@ static int dcblock_prepare(struct processing_module *mod, /* get sink data format and period bytes */ cd->sink_format = audio_stream_get_frm_fmt(&sinkb->stream); - dcblock_set_frame_alignment(&sourceb->stream, &sinkb->stream); - dcblock_init_state(cd); cd->dcblock_func = dcblock_find_func(cd->source_format); if (!cd->dcblock_func) { diff --git a/src/audio/drc/drc.c b/src/audio/drc/drc.c index c69b3a742ea6..3c3bae268bd5 100644 --- a/src/audio/drc/drc.c +++ b/src/audio/drc/drc.c @@ -227,14 +227,6 @@ static int drc_get_config(struct processing_module *mod, return comp_data_blob_get_cmd(cd->model_handler, cdata, fragment_size); } -static void drc_set_alignment(struct audio_stream *source, - struct audio_stream *sink) -{ - /* Currently no optimizations those would use wider loads and stores */ - audio_stream_init_alignment_constants(1, 1, source); - audio_stream_init_alignment_constants(1, 1, sink); -} - static int drc_process(struct processing_module *mod, struct input_stream_buffer *input_buffers, int num_input_buffers, @@ -308,7 +300,6 @@ static int drc_prepare(struct processing_module *mod, /* DRC component will only ever have 1 source and 1 sink buffer */ sourceb = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sinkb = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); - drc_set_alignment(&sourceb->stream, &sinkb->stream); /* get source data format */ cd->source_format = audio_stream_get_frm_fmt(&sourceb->stream); diff --git a/src/audio/kpb.c b/src/audio/kpb.c index 6e4ce13bdca0..c195c5ba0d7c 100644 --- a/src/audio/kpb.c +++ b/src/audio/kpb.c @@ -893,16 +893,12 @@ static int kpb_prepare(struct comp_dev *dev) */ if (kpb->ipc4_cfg.base_cfg.ibs != kpb->ipc4_cfg.base_cfg.obs) { struct list_item *sink_list; - const uint32_t byte_align = 1; - const uint32_t frame_align_req = 1; uint32_t sink_id; list_for_item(sink_list, &dev->bsink_list) { struct comp_buffer *sink = container_of(sink_list, struct comp_buffer, source_list); - audio_stream_init_alignment_constants(byte_align, frame_align_req, - &sink->stream); sink_id = buf_get_id(sink); if (sink_id == 0) diff --git a/src/audio/mfcc/mfcc.c b/src/audio/mfcc/mfcc.c index 109c3636a4a1..9b88f1bc238a 100644 --- a/src/audio/mfcc/mfcc.c +++ b/src/audio/mfcc/mfcc.c @@ -177,15 +177,6 @@ static int mfcc_process(struct processing_module *mod, return 0; } -static void mfcc_set_alignment(struct audio_stream *source, struct audio_stream *sink) -{ - const uint32_t byte_align = 1; - const uint32_t frame_align_req = 1; - - audio_stream_init_alignment_constants(byte_align, frame_align_req, source); - audio_stream_init_alignment_constants(byte_align, frame_align_req, sink); -} - static int mfcc_prepare(struct processing_module *mod, struct sof_source **sources, int num_of_sources, struct sof_sink **sinks, int num_of_sinks) @@ -208,9 +199,6 @@ static int mfcc_prepare(struct processing_module *mod, /* get source data format */ source_format = audio_stream_get_frm_fmt(&sourceb->stream); - /* set align requirements */ - mfcc_set_alignment(&sourceb->stream, &sinkb->stream); - /* get sink data format and period bytes */ sink_format = audio_stream_get_frm_fmt(&sinkb->stream); sink_period_bytes = audio_stream_period_bytes(&sinkb->stream, dev->frames); diff --git a/src/audio/mixer/mixer.c b/src/audio/mixer/mixer.c index fce964f4da1b..a13e6652bf1e 100644 --- a/src/audio/mixer/mixer.c +++ b/src/audio/mixer/mixer.c @@ -204,17 +204,8 @@ static inline void mixer_set_frame_alignment(struct audio_stream *source) /*There is no limit for frame number, so set it as 1*/ const uint32_t frame_align_req = 1; -#else - - /* Since the generic version process signal sample by sample, so there is no - * limit for it, then set the byte_align and frame_align_req to be 1. - */ - const uint32_t byte_align = 1; - const uint32_t frame_align_req = 1; - -#endif - audio_stream_init_alignment_constants(byte_align, frame_align_req, source); +#endif } static int mixer_prepare(struct processing_module *mod, diff --git a/src/audio/multiband_drc/multiband_drc.c b/src/audio/multiband_drc/multiband_drc.c index fe1c11f3cdab..8bb203a6bacf 100644 --- a/src/audio/multiband_drc/multiband_drc.c +++ b/src/audio/multiband_drc/multiband_drc.c @@ -411,14 +411,6 @@ static int multiband_drc_get_config(struct processing_module *mod, return comp_data_blob_get_cmd(cd->model_handler, cdata, fragment_size); } -static void multiband_drc_set_alignment(struct audio_stream *source, - struct audio_stream *sink) -{ - /* Currently no optimizations those would use wider loads and stores */ - audio_stream_init_alignment_constants(1, 1, source); - audio_stream_init_alignment_constants(1, 1, sink); -} - static int multiband_drc_process(struct processing_module *mod, struct input_stream_buffer *input_buffers, int num_input_buffers, struct output_stream_buffer *output_buffers, @@ -509,8 +501,6 @@ static int multiband_drc_prepare(struct processing_module *mod, sourceb = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sinkb = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); - multiband_drc_set_alignment(&sourceb->stream, &sinkb->stream); - /* get source data format */ cd->source_format = audio_stream_get_frm_fmt(&sourceb->stream); channels = audio_stream_get_channels(&sourceb->stream); diff --git a/src/audio/mux/mux.c b/src/audio/mux/mux.c index de16ee754c98..ef308753c8ce 100644 --- a/src/audio/mux/mux.c +++ b/src/audio/mux/mux.c @@ -276,8 +276,6 @@ static void set_mux_params(struct processing_module *mod) struct comp_buffer *sink, *source; struct list_item *source_list; int j; - const uint32_t byte_align = 1; - const uint32_t frame_align_req = 1; params->direction = dev->direction; params->channels = cd->md.base_cfg.audio_fmt.channels_count; @@ -297,8 +295,6 @@ static void set_mux_params(struct processing_module *mod) /* update sink format */ if (!list_is_empty(&dev->bsink_list)) { sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list); - audio_stream_init_alignment_constants(byte_align, frame_align_req, - &sink->stream); if (!sink->hw_params_configured) { ipc4_update_buffer_format(sink, &cd->md.output_format); @@ -313,8 +309,6 @@ static void set_mux_params(struct processing_module *mod) list_for_item(source_list, &dev->bsource_list) { source = container_of(source_list, struct comp_buffer, sink_list); - audio_stream_init_alignment_constants(byte_align, frame_align_req, - &source->stream); j = buf_get_id(source); cd->config.streams[j].pipeline_id = source->pipeline_id; if (j == BASE_CFG_QUEUED_ID) @@ -607,7 +601,6 @@ static int mux_prepare(struct processing_module *mod, list_for_item(blist, &dev->bsource_list) { source = container_of(blist, struct comp_buffer, sink_list); state = source->source->state; - audio_stream_init_alignment_constants(1, 1, &source->stream); /* only prepare downstream if we have no active sources */ if (state == COMP_STATE_PAUSED || state == COMP_STATE_ACTIVE) @@ -617,7 +610,6 @@ static int mux_prepare(struct processing_module *mod, /* set sink align to 1 byte, 1 frame */ list_for_item(blist, &dev->bsink_list) { sink = container_of(blist, struct comp_buffer, source_list); - audio_stream_init_alignment_constants(1, 1, &sink->stream); } /* prepare downstream */ diff --git a/src/audio/volume/volume.c b/src/audio/volume/volume.c index 002ceff9d852..694d778db6b0 100644 --- a/src/audio/volume/volume.c +++ b/src/audio/volume/volume.c @@ -623,7 +623,6 @@ static void volume_set_alignment(struct audio_stream *source, struct audio_stream *sink) { #if XCHAL_HAVE_HIFI3 || XCHAL_HAVE_HIFI4 - /* Both source and sink buffer in HiFi 3 or HiFi4 processing version, * xtensa intrinsics ask for 8-byte aligned. 5.1 format SSE audio * requires 16-byte aligned. @@ -633,18 +632,9 @@ static void volume_set_alignment(struct audio_stream *source, /*There is no limit for frame number, so both source and sink set it to be 1*/ const uint32_t frame_align_req = 1; -#else - - /* Since the generic version process signal sample by sample, so there is no - * limit for it, then set the byte_align and frame_align_req to be 1. - */ - const uint32_t byte_align = 1; - const uint32_t frame_align_req = 1; - -#endif - audio_stream_init_alignment_constants(byte_align, frame_align_req, source); audio_stream_init_alignment_constants(byte_align, frame_align_req, sink); +#endif } /** From 6fd333e412eb8bf0eb82790c201af7ddfd139401 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 2 Jan 2024 11:29:05 -0800 Subject: [PATCH 07/10] module_adapter_ipc4: Save and pre-parse base_cfg_ext data The kernel-provided "base_cfg_ext" data wasn't being handled correctly by the module adapter layer. These structs (packed wire formats) only ever lived in the IPC memory. The module would set them on an "init_data" before calling into driver init, and then clear that poitner afterwards. That's a critical problem, because the values in that struct need to be used at pipeline setup time to configure buffer formats! Save the data correctly. Also pre-parse it so users don't need to do byte math on raw buffers and can just use "in/output_pins[]" arrays on the module_config struct. Signed-off-by: Andy Ross --- src/audio/module_adapter/module_adapter.c | 4 ++ .../module_adapter/module_adapter_ipc4.c | 38 ++++++++++++++----- src/include/module/module/base.h | 4 ++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/audio/module_adapter/module_adapter.c b/src/audio/module_adapter/module_adapter.c index dee5366875fc..ec0734323dc0 100644 --- a/src/audio/module_adapter/module_adapter.c +++ b/src/audio/module_adapter/module_adapter.c @@ -1329,6 +1329,10 @@ int module_adapter_reset(struct comp_dev *dev) rfree(mod->stream_params); mod->stream_params = NULL; +#if CONFIG_IPC_MAJOR_4 + rfree(mod->priv.cfg.input_pins); +#endif + comp_dbg(dev, "module_adapter_reset(): done"); return comp_set_state(dev, COMP_TRIGGER_RESET); diff --git a/src/audio/module_adapter/module_adapter_ipc4.c b/src/audio/module_adapter/module_adapter_ipc4.c index 1e0257b0a1cb..81f7fca1d22d 100644 --- a/src/audio/module_adapter/module_adapter_ipc4.c +++ b/src/audio/module_adapter/module_adapter_ipc4.c @@ -38,19 +38,39 @@ int module_adapter_init_data(struct comp_dev *dev, const struct comp_ipc_config *config, const void *spec) { - if (dev->drv->type == SOF_COMP_MODULE_ADAPTER) { - const struct ipc_config_process *ipc_module_adapter = spec; + const struct ipc_config_process *cp = spec; + const struct ipc4_base_module_extended_cfg *cfg = (void *)cp->data; + size_t cfgsz = cp->size; - dst->init_data = ipc_module_adapter->data; - dst->size = ipc_module_adapter->size; - dst->avail = true; + assert(dev->drv->type == SOF_COMP_MODULE_ADAPTER); + if (cp->size < sizeof(cfg->base_cfg)) + return -EINVAL; - memcpy_s(&dst->base_cfg, sizeof(dst->base_cfg), ipc_module_adapter->data, - sizeof(dst->base_cfg)); - } else { - dst->init_data = spec; + dst->base_cfg = cfg->base_cfg; + dst->size = cp->size; + + if (cp->size >= sizeof(*cfg)) { + int ni = cfg->base_cfg_ext.nb_input_pins; + int no = cfg->base_cfg_ext.nb_output_pins; + size_t pinsz = (ni * sizeof(*dst->input_pins)) + + (no * sizeof(*dst->output_pins)); + + if (cp->size == (sizeof(*cfg) + pinsz)) { + dst->nb_input_pins = ni; + dst->nb_output_pins = no; + dst->input_pins = rmalloc(SOF_MEM_ZONE_RUNTIME_SHARED, + 0, SOF_MEM_CAPS_RAM, pinsz); + if (!dst->input_pins) + return -ENOMEM; + + dst->output_pins = (void *) &dst->input_pins[ni]; + memcpy_s(dst->input_pins, pinsz, + &cfg->base_cfg_ext.pin_formats[0], pinsz); + } } + dst->init_data = cp->data; /* legacy API */ + dst->avail = true; return 0; } diff --git a/src/include/module/module/base.h b/src/include/module/module/base.h index 904b83741961..323599b055f2 100644 --- a/src/include/module/module/base.h +++ b/src/include/module/module/base.h @@ -30,6 +30,10 @@ struct module_config { const void *init_data; /**< Initial IPC configuration. */ #if CONFIG_IPC_MAJOR_4 struct ipc4_base_module_cfg base_cfg; + uint8_t nb_input_pins; + uint8_t nb_output_pins; + struct ipc4_input_pin_format *input_pins; + struct ipc4_output_pin_format *output_pins; #endif }; From af520b41405f3ac65251b3778b1aa42f28da5b9f Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 2 Jan 2024 16:43:44 -0800 Subject: [PATCH 08/10] ipc4: Use saved base_cfg_ext data in ipc_comp_connect() The code here never really worked right. The module base config values weren't saved by the core layers, so querying the attribute returned nothing. A few components have been modified with a whitebox workaround where they write directly to the basecfg_ext field in the module_config, but that's not really an API we should want to support. Get the values from the now-correctly-saved-and-parsed fields the module init layer has left for us, with considerable code savings and much less heap thrashing for the temporary copy. Signed-off-by: Andy Ross --- src/ipc/ipc4/helper.c | 56 +++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/src/ipc/ipc4/helper.c b/src/ipc/ipc4/helper.c index 4006ce82be8d..4f446623f62f 100644 --- a/src/ipc/ipc4/helper.c +++ b/src/ipc/ipc4/helper.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -424,6 +425,7 @@ static int ll_wait_finished_on_core(struct comp_dev *dev) int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) { + struct ipc4_base_module_cfg_ext *src_basecfg_ext, *sink_basecfg_ext; struct ipc4_module_bind_unbind *bu; struct comp_buffer *buffer; struct comp_dev *source; @@ -431,8 +433,12 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) struct ipc4_base_module_cfg source_src_cfg; struct ipc4_base_module_cfg sink_src_cfg; uint32_t flags; + uint32_t ibs = 0; + uint32_t obs = 0; + uint32_t buf_size; int src_id, sink_id; int ret; + size_t size; bu = (struct ipc4_module_bind_unbind *)_connect; src_id = IPC4_COMP_ID(bu->primary.r.module_id, bu->primary.r.instance_id); @@ -453,17 +459,42 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) if (!cpu_is_me(source->ipc_config.core) && !cross_core_bind) return ipc4_process_on_core(source->ipc_config.core, false); - /* these might call comp_ipc4_get_attribute_remote() if necessary */ - ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg); - if (ret < 0) { - tr_err(&ipc_tr, "failed to get base config for module %#x", dev_comp_id(source)); - return IPC4_FAILURE; + struct processing_module *srcmod = comp_get_drvdata(source); + struct processing_module *dstmod = comp_get_drvdata(sink); + struct module_config *dstcfg = &dstmod->priv.cfg; + struct module_config *srccfg = &srcmod->priv.cfg; + + /* get obs from the base config extension if the src queue ID is non-zero */ + if (bu->extension.r.src_queue && bu->extension.r.src_queue < srccfg->nb_output_pins) + obs = srccfg->output_pins[bu->extension.r.src_queue].obs; + + /* get obs from base config if src queue ID is 0 or if base config extn is missing */ + if (!obs) { + /* these might call comp_ipc4_get_attribute_remote() if necessary */ + ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg); + + if (ret < 0) { + tr_err(&ipc_tr, "failed to get base config for src module %#x", + dev_comp_id(source)); + return IPC4_FAILURE; + } + obs = source_src_cfg.obs; } - ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG, &sink_src_cfg); - if (ret < 0) { - tr_err(&ipc_tr, "failed to get base config for module %#x", dev_comp_id(sink)); - return IPC4_FAILURE; + /* get ibs from the base config extension if the sink queue ID is non-zero */ + if (bu->extension.r.dst_queue && bu->extension.r.dst_queue < dstcfg->nb_input_pins) + ibs = dstcfg->input_pins[bu->extension.r.dst_queue].ibs; + + /* get ibs from base config if sink queue ID is 0 or if base config extn is missing */ + if (!ibs) { + ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG, &sink_src_cfg); + if (ret < 0) { + tr_err(&ipc_tr, "failed to get base config for sink module %#x", + dev_comp_id(sink)); + return IPC4_FAILURE; + } + + ibs = sink_src_cfg.ibs; } /* create a buffer @@ -472,13 +503,12 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) * in case of DP -> LL * size = 2*ibs of destination (LL) module. DP queue will handle obs of DP module */ - uint32_t buf_size; - if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL) buf_size = source_src_cfg.obs * 2; else buf_size = sink_src_cfg.ibs * 2; + buffer = ipc4_create_buffer(source, cross_core_bind, buf_size, bu->extension.r.src_queue, bu->extension.r.dst_queue); if (!buffer) { @@ -496,8 +526,8 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect) * sink_module needs to set its IBS (input buffer size) * as min_available in buffer's source ifc */ - sink_set_min_free_space(audio_stream_get_sink(&buffer->stream), source_src_cfg.obs); - source_set_min_available(audio_stream_get_source(&buffer->stream), sink_src_cfg.ibs); + sink_set_min_free_space(audio_stream_get_sink(&buffer->stream), obs); + source_set_min_available(audio_stream_get_source(&buffer->stream), ibs); /* * Connect and bind the buffer to both source and sink components with LL processing been From 3014415f8410910fd452354ecb71bf2b8def9db1 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Fri, 22 Dec 2023 12:09:00 -0800 Subject: [PATCH 09/10] google_rtc_audio_processing: Fixes for MTL branch Squashed fixups to this code from https://github.com/thesofproject/sof/pull/8571 Signed-off-by: Andy Ross --- src/audio/google/Kconfig | 24 +- .../google/google_rtc_audio_processing.c | 826 +++++++++++------- .../google/google_rtc_audio_processing_mock.c | 54 +- 3 files changed, 540 insertions(+), 364 deletions(-) diff --git a/src/audio/google/Kconfig b/src/audio/google/Kconfig index 2b92dd573879..ac323716a7f8 100644 --- a/src/audio/google/Kconfig +++ b/src/audio/google/Kconfig @@ -15,8 +15,6 @@ config COMP_GOOGLE_HOTWORD_DETECT config COMP_GOOGLE_RTC_AUDIO_PROCESSING bool "Google Real Time Communication Audio processing" select COMP_BLOB - select GOOGLE_RTC_AUDIO_PROCESSING_MOCK if COMP_STUBS - depends on IPC_MAJOR_4 default n help Select for Google real-time communication audio processing. It @@ -25,13 +23,7 @@ config COMP_GOOGLE_RTC_AUDIO_PROCESSING This component takes raw microphones input and playback reference and outputs an echo-free microphone signal. -config COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API - depends on COMP_GOOGLE_RTC_AUDIO_PROCESSING - bool "Use 32bit API in Google Audio processing" - default n - help - Selects an API to be used in communication with the Google real-time - communication audio processing: 32bit float or 16bit integer +if COMP_GOOGLE_RTC_AUDIO_PROCESSING config COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ depends on COMP_GOOGLE_RTC_AUDIO_PROCESSING @@ -41,6 +33,16 @@ config COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ Sets the sample rate for the memory buffer for the Google real-time communication audio processing. +config COMP_GOOGLE_RTC_AUDIO_PROCESSING_CHANNEL_MAX + int "Max number of AEC channels" + default 2 + help + Sets the maximum number source/sink channels Google Real + Time Communication Audio Processing will use for. This is a + computation and memory budget tunable. Channel counts are + retrieved at runtime, but channels higher than this number + are ignored (on input) or cleared (output). + config COMP_GOOGLE_RTC_AUDIO_PROCESSING_MEMORY_BUFFER_SIZE_BYTES depends on COMP_GOOGLE_RTC_AUDIO_PROCESSING int "Memory buffer size for Google Real Time Communication Audio processing" @@ -67,10 +69,12 @@ config COMP_GOOGLE_RTC_AUDIO_PROCESSING_MIC_HEADROOM_LINEAR config GOOGLE_RTC_AUDIO_PROCESSING_MOCK bool "Google Real Time Communication Audio processing mock" - default n + default y if COMP_STUBS depends on COMP_GOOGLE_RTC_AUDIO_PROCESSING help Mock Google real-time communication audio processing. It allows for compilation check and basic audio flow checking. +endif # COMP_GOOGLE_RTC_AUDIO_PROCESSING + endmenu diff --git a/src/audio/google/google_rtc_audio_processing.c b/src/audio/google/google_rtc_audio_processing.c index 2a045dea4e04..27960632bd83 100644 --- a/src/audio/google/google_rtc_audio_processing.c +++ b/src/audio/google/google_rtc_audio_processing.c @@ -41,13 +41,6 @@ #define GOOGLE_RTC_AUDIO_PROCESSING_FREQENCY_TO_PERIOD_FRAMES 100 #define GOOGLE_RTC_NUM_INPUT_PINS 2 -#define GOOGLE_RTC_NUM_OUTPUT_PINS 1 - -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API -#define BUF_TYPE float -#else -#define BUF_TYPE int16_t -#endif LOG_MODULE_REGISTER(google_rtc_audio_processing, CONFIG_SOF_LOG_LEVEL); @@ -59,21 +52,54 @@ DECLARE_SOF_RT_UUID("google-rtc-audio-processing", google_rtc_audio_processing_u DECLARE_TR_CTX(google_rtc_audio_processing_tr, SOF_UUID(google_rtc_audio_processing_uuid), LOG_LEVEL_INFO); +#if !(defined(__ZEPHYR__) && defined(CONFIG_XTENSA)) +/* Zephyr provides uncached memory for static variables on SMP, but we + * are single-core component and know we can safely use the cache for + * AEC work. XTOS SOF is cached by default, so stub the Zephyr API. + */ +#define arch_xtensa_cached_ptr(p) (p) +#endif + +#ifndef __ZEPHYR__ +#define ALWAYS_INLINE inline __attribute__((always_inline)) +#endif + +static __aligned(PLATFORM_DCACHE_ALIGN) +uint8_t aec_mem_blob[CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_MEMORY_BUFFER_SIZE_BYTES]; + +#define NUM_FRAMES (CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ \ + / GOOGLE_RTC_AUDIO_PROCESSING_FREQENCY_TO_PERIOD_FRAMES) +#define CHAN_MAX CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_CHANNEL_MAX + +static __aligned(PLATFORM_DCACHE_ALIGN) +float refoutbuf[CHAN_MAX][NUM_FRAMES]; + +static __aligned(PLATFORM_DCACHE_ALIGN) +float micbuf[CHAN_MAX][NUM_FRAMES]; + struct google_rtc_audio_processing_comp_data { - struct sof_ipc4_aec_config config; +#ifdef CONFIG_IPC_MAJOR_4 + struct ipc4_audio_format in_fmts[2]; + struct ipc4_audio_format out_fmt; +#endif uint32_t num_frames; int num_aec_reference_channels; int num_capture_channels; GoogleRtcAudioProcessingState *state; - BUF_TYPE *aec_reference_buffer; - BUF_TYPE *aec_reference_buffer_ptrs[SOF_IPC_MAX_CHANNELS]; - BUF_TYPE *process_buffer; - BUF_TYPE *process_buffer_ptrs[SOF_IPC_MAX_CHANNELS]; - uint8_t *memory_buffer; + float *raw_mic_buffers[CHAN_MAX]; + float *refout_buffers[CHAN_MAX]; + int buffered_frames; struct comp_data_blob_handler *tuning_handler; bool reconfigure; + bool last_ref_ok; int aec_reference_source; int raw_microphone_source; + struct comp_buffer *ref_comp_buffer; + int ref_framesz; + int cap_framesz; + void (*mic_copy)(struct sof_source *src, int frames, float **dst_bufs, int frame0); + void (*ref_copy)(struct sof_source *src, int frames, float **dst_bufs, int frame0); + void (*out_copy)(struct sof_sink *dst, int frames, float **src_bufs); }; void *GoogleRtcMalloc(size_t size) @@ -86,6 +112,236 @@ void GoogleRtcFree(void *ptr) return rfree(ptr); } +#ifdef CONFIG_IPC_MAJOR_4 +/* Workaround: IPC4 fails to set up stream metadata on the buffer + * objects at pipeline creation time, and traditionally components + * have been responsible for doing this. The way this works is that + * during the init() call, we cache the relevant ipc4_audio_format + * records from the module "extended config" (which is untyped + * byte-packed data, I think it's raw bytes from the host?) that + * correspond to our connected streams. This config struct gets freed + * after initialization, so it has to be done then. + * + * Then at prepare time, we must use ipc4_update_source/sink_format() + * to set values on the (incompletely initialized) streams. + * Similarly, we have to call audio_stream_init_alignment_constants(), + * otherwise needed values on the audio stream remain uninitialized. + * + * It's not really documented what happens if our settings disagree + * with the component on the other side of the buffer! Presumably + * that would be a fatal topology bug, but nothing is set up to detect + * it. In fact in practice most other components DON'T do this, so + * our settings win (and thus we must do this, or else the rest of the + * stream sees garbage and misbehaves, generally with buffer overruns + * or short reads). + * + * Finally, there is somewhat... unique handling needed to get correct + * IBS/OBS buffer sizing. We have two inputs with different stream + * formats, and thus different minimum block sizes. The "base" module + * config only has space for one IBS value. So the pipeline code has + * an "extended" config with separate IBS per pin, but it will only + * use it if it knows about it, which it doesn't by default because + * module initialization throws away the data! So it falls to us to + * duplicate a copy and store it ourselves, in a separate field from + * where we found it. + */ +static int init_ipc4_fmts(struct processing_module *mod) +{ + int i; + struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); + const struct ipc4_base_module_extended_cfg *bmec = mod->priv.cfg.init_data; + const struct ipc4_input_pin_format *ipf; + const struct ipc4_output_pin_format *opf; + const struct ipc4_base_module_cfg_ext *bce; + + if (!bmec) + return -EINVAL; + + bce = &bmec->base_cfg_ext; + if (bce->nb_input_pins != 2 && bce->nb_output_pins != 1) { + comp_err(mod->dev, "Must have 2 input, 1 output pins"); + return -EINVAL; + } + + ipf = (void *)&bce->pin_formats[0]; + for (i = 0; i < bce->nb_input_pins; i++, ipf++) { + if (ipf->pin_index < 0 || ipf->pin_index >= 2) { + comp_err(mod->dev, "Incorrect input pin index %d", ipf->pin_index); + return -EINVAL; + } + cd->in_fmts[ipf->pin_index] = ipf->audio_fmt; + } + + opf = (void *)ipf; + if (opf->pin_index != 0) { + comp_err(mod->dev, "Incorrect output pin index %d\n", opf->pin_index); + return -EINVAL; + } + cd->out_fmt = opf->audio_fmt; + + size_t bcesz = sizeof(*bce) + (bce->nb_input_pins * sizeof(*ipf) + + bce->nb_output_pins * sizeof(*opf)); + void *bcedup = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, bcesz); + + if (!bcedup) + return -ENOMEM; + memcpy(bcedup, bce, bcesz); + mod->priv.cfg.basecfg_ext = bcedup; + + return 0; +} + +static void prepare_ipc4_fmts(struct processing_module *mod, + struct sof_source **sources, + struct sof_sink **sinks) +{ + struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); + + /* FIXME: The "pin" index (the value of the pin_index field of + * the ipc4_in/output_pin_format structs seen in module config + * at init() time) and the "source" index (the ordering of the + * sources[] argument to prepare()/process()) ARE PRESENTED + * BACKWARDS! And I don't see any API to tell me which is + * which (note that the ordering of the connected buffers in + * the comp_dev sink/source lists is a THIRD convention, and + * matches sources[]/sinks[]). + */ + ipc4_update_source_format(sources[0], &cd->in_fmts[1]); + ipc4_update_source_format(sources[1], &cd->in_fmts[0]); + ipc4_update_sink_format(sinks[0], &cd->out_fmt); + + source_set_alignment_constants(sources[0], 1, 1); + source_set_alignment_constants(sources[1], 1, 1); + sink_set_alignment_constants(sinks[0], 1, 1); +} +#endif + +static ALWAYS_INLINE float s16_to_float(const char *ptr) +{ + float scale = -(float)SHRT_MIN; + float x = *(int16_t *)ptr; + + return (1.0f / scale) * x; +} + +static ALWAYS_INLINE void float_to_s16(float x, char *dst) +{ + float scale = -(float)SHRT_MIN; + float min = -1.0f; + float max = 1.0f - 1.0f / scale; + int16_t i = (int16_t)(scale * (x < min ? min : (x > max ? max : x))); + + *(int16_t *)dst = i; +} + +static ALWAYS_INLINE float s32_to_float(const char *ptr) +{ + float scale = -(float)INT_MIN; + float x = *(int32_t *)ptr; + + return (1.0f / scale) * x; +} + +static ALWAYS_INLINE void float_to_s32(float x, char *dst) +{ + float scale = -(float)INT_MIN; + float min = -1.0f; + float max = 1.0f - 1.0f / scale; + int32_t i = (int32_t)(scale * (x < min ? min : (x > max ? max : x))); + + *(int32_t *)dst = i; +} + +static ALWAYS_INLINE void source_to_float(struct sof_source *src, float **dst_bufs, + float (*cvt_fn)(const char *), + int sample_sz, int frame0, int frames) +{ + size_t chan = source_get_channels(src); + size_t bytes = frames * chan * sample_sz; + int i, c, err, ndst = MIN(chan, CHAN_MAX); + const char *buf, *bufstart, *bufend; + float *dst[CHAN_MAX]; + size_t bufsz; + + for (i = 0; i < ndst; i++) + dst[i] = &dst_bufs[i][frame0]; + + err = source_get_data(src, bytes, (void *)&buf, (void *)&bufstart, &bufsz); + assert(err == 0); + bufend = &bufstart[bufsz]; + + while (frames) { + size_t n = MIN(frames, (bufsz - (buf - bufstart)) / (chan * sample_sz)); + + for (i = 0; i < n; i++) { + for (c = 0; c < ndst; c++) { + *dst[c]++ = cvt_fn(buf); + buf += sample_sz; + } + buf += sample_sz * (chan - ndst); /* skip unused channels */ + } + frames -= n; + if (buf >= bufend) + buf = bufstart; + } + source_release_data(src, bytes); +} + +static ALWAYS_INLINE void float_to_sink(struct sof_sink *dst, float **src_bufs, + void (*cvt_fn)(float, char *), + int sample_sz, int frames) +{ + size_t chan = sink_get_channels(dst); + size_t bytes = frames * chan * sample_sz; + int i, c, err, nsrc = MIN(chan, CHAN_MAX); + char *buf, *bufstart, *bufend; + float *src[CHAN_MAX]; + size_t bufsz; + + for (i = 0; i < nsrc; i++) + src[i] = &src_bufs[i][0]; + + err = sink_get_buffer(dst, bytes, (void *)&buf, (void *)&bufstart, &bufsz); + assert(err == 0); + bufend = &bufstart[bufsz]; + + while (frames) { + size_t n = MIN(frames, (bufsz - (buf - bufstart)) / (chan * sample_sz)); + + for (i = 0; i < n; i++) { + for (c = 0; c < nsrc; c++) { + cvt_fn(*src[c]++, buf); + buf += sample_sz; + } + buf += sample_sz * (chan - nsrc); /* skip unused channels */ + } + frames -= n; + if (buf >= bufend) + buf = bufstart; + } + sink_commit_buffer(dst, bytes); +} + +static void source_copy16(struct sof_source *src, int frames, float **dst_bufs, int frame0) +{ + source_to_float(src, dst_bufs, s16_to_float, sizeof(int16_t), frame0, frames); +} + +static void source_copy32(struct sof_source *src, int frames, float **dst_bufs, int frame0) +{ + source_to_float(src, dst_bufs, s32_to_float, sizeof(int32_t), frame0, frames); +} + +static void sink_copy16(struct sof_sink *dst, int frames, float **src_bufs) +{ + float_to_sink(dst, src_bufs, float_to_s16, sizeof(int16_t), frames); +} + +static void sink_copy32(struct sof_sink *dst, int frames, float **src_bufs) +{ + float_to_sink(dst, src_bufs, float_to_s32, sizeof(int32_t), frames); +} + static int google_rtc_audio_processing_reconfigure(struct processing_module *mod) { struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); @@ -233,6 +489,60 @@ static int google_rtc_audio_processing_reconfigure(struct processing_module *mod return 0; } +#if CONFIG_IPC_MAJOR_3 +static int google_rtc_audio_processing_cmd_set_data(struct processing_module *mod, + struct sof_ipc_ctrl_data *cdata) +{ + struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); + int ret; + + switch (cdata->cmd) { + case SOF_CTRL_CMD_BINARY: + ret = comp_data_blob_set_cmd(cd->tuning_handler, cdata); + if (ret) + return ret; + /* Accept the new blob immediately so that userspace can write + * the control in quick succession without error. + * This ensures the last successful control write from userspace + * before prepare/copy is applied. + * The config blob is not referenced after reconfigure() returns + * so it is safe to call comp_get_data_blob here which frees the + * old blob. This assumes cmd() and prepare()/copy() cannot run + * concurrently which is the case when there is no preemption. + */ + if (comp_is_new_data_blob_available(cd->tuning_handler)) { + comp_get_data_blob(cd->tuning_handler, NULL, NULL); + cd->reconfigure = true; + } + return 0; + default: + comp_err(mod->dev, + "google_rtc_audio_processing_ctrl_set_data(): Only binary controls supported %d", + cdata->cmd); + return -EINVAL; + } +} + +static int google_rtc_audio_processing_cmd_get_data(struct processing_module *mod, + struct sof_ipc_ctrl_data *cdata, + size_t max_data_size) +{ + struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); + + comp_info(mod->dev, "google_rtc_audio_processing_ctrl_get_data(): %u", cdata->cmd); + + switch (cdata->cmd) { + case SOF_CTRL_CMD_BINARY: + return comp_data_blob_get_cmd(cd->tuning_handler, cdata, max_data_size); + default: + comp_err(mod->dev, + "google_rtc_audio_processing_ctrl_get_data(): Only binary controls supported %d", + cdata->cmd); + return -EINVAL; + } +} +#endif + static int google_rtc_audio_processing_set_config(struct processing_module *mod, uint32_t param_id, enum module_cfg_fragment_position pos, uint32_t data_offset_size, @@ -240,6 +550,7 @@ static int google_rtc_audio_processing_set_config(struct processing_module *mod, size_t fragment_size, uint8_t *response, size_t response_size) { +#if CONFIG_IPC_MAJOR_4 struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); int ret; @@ -273,14 +584,25 @@ static int google_rtc_audio_processing_set_config(struct processing_module *mod, } return 0; +#elif CONFIG_IPC_MAJOR_3 + struct sof_ipc_ctrl_data *cdata = (struct sof_ipc_ctrl_data *)fragment; + + return google_rtc_audio_processing_cmd_set_data(mod, cdata); +#endif } static int google_rtc_audio_processing_get_config(struct processing_module *mod, uint32_t param_id, uint32_t *data_offset_size, uint8_t *fragment, size_t fragment_size) { +#if CONFIG_IPC_MAJOR_4 comp_err(mod->dev, "google_rtc_audio_processing_ctrl_get_config(): Not supported"); return -EINVAL; +#elif CONFIG_IPC_MAJOR_3 + struct sof_ipc_ctrl_data *cdata = (struct sof_ipc_ctrl_data *)fragment; + + return google_rtc_audio_processing_cmd_get_data(mod, cdata, fragment_size); +#endif } static int google_rtc_audio_processing_init(struct processing_module *mod) @@ -288,9 +610,7 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) struct module_data *md = &mod->priv; struct comp_dev *dev = mod->dev; struct google_rtc_audio_processing_comp_data *cd; - int ret; - int channel; - size_t buf_size; + int ret, i; comp_info(dev, "google_rtc_audio_processing_init()"); @@ -303,22 +623,11 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) md->private = cd; - struct module_config *cfg = &md->cfg; - const struct ipc4_base_module_extended_cfg *base_cfg = md->cfg.init_data; - struct ipc4_input_pin_format reference_fmt, output_fmt; - const size_t size = sizeof(struct ipc4_input_pin_format); - - cd->config.base_cfg = base_cfg->base_cfg; - - /* Copy the reference format from input pin 1 format */ - memcpy_s(&reference_fmt, size, - &base_cfg->base_cfg_ext.pin_formats[size], size); - memcpy_s(&output_fmt, size, - &base_cfg->base_cfg_ext.pin_formats[size * GOOGLE_RTC_NUM_INPUT_PINS], size); - - cd->config.reference_fmt = reference_fmt.audio_fmt; - cd->config.output_fmt = output_fmt.audio_fmt; - cd->config = *(const struct sof_ipc4_aec_config *)cfg->init_data; +#ifdef CONFIG_IPC_MAJOR_4 + ret = init_ipc4_fmts(mod); /* workaround, see above */ + if (ret < 0) + goto fail; +#endif cd->tuning_handler = comp_data_blob_handler_new(dev); if (!cd->tuning_handler) { @@ -326,23 +635,13 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) goto fail; } - cd->num_aec_reference_channels = cd->config.reference_fmt.channels_count; - cd->num_capture_channels = mod->priv.cfg.base_cfg.audio_fmt.channels_count; - cd->num_frames = CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ / - GOOGLE_RTC_AUDIO_PROCESSING_FREQENCY_TO_PERIOD_FRAMES; + cd->num_aec_reference_channels = CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_CHANNEL_MAX; + cd->num_capture_channels = CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_CHANNEL_MAX; + cd->num_frames = NUM_FRAMES; - if (CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_MEMORY_BUFFER_SIZE_BYTES > 0) { - cd->memory_buffer = rballoc(0, SOF_MEM_CAPS_RAM, - CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_MEMORY_BUFFER_SIZE_BYTES * - sizeof(cd->memory_buffer[0])); - if (!cd->memory_buffer) { - comp_err(dev, "google_rtc_audio_processing_init: failed to allocate memory buffer"); - ret = -ENOMEM; - goto fail; - } - - GoogleRtcAudioProcessingAttachMemoryBuffer(cd->memory_buffer, CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_MEMORY_BUFFER_SIZE_BYTES); - } + /* Giant blob of scratch memory. */ + GoogleRtcAudioProcessingAttachMemoryBuffer(arch_xtensa_cached_ptr(&aec_mem_blob[0]), + sizeof(aec_mem_blob)); cd->state = GoogleRtcAudioProcessingCreateWithConfig(CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ, cd->num_capture_channels, @@ -368,31 +667,12 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) goto fail; } - buf_size = cd->num_frames * cd->num_capture_channels * sizeof(cd->process_buffer[0]); - comp_dbg(dev, "Allocating process_buffer of size %u", buf_size); - cd->process_buffer = rballoc(0, SOF_MEM_CAPS_RAM, buf_size); - if (!cd->process_buffer) { - comp_err(dev, "Allocating process_buffer failure"); - ret = -EINVAL; - goto fail; + for (i = 0; i < CHAN_MAX; i++) { + cd->raw_mic_buffers[i] = arch_xtensa_cached_ptr(&micbuf[i][0]); + cd->refout_buffers[i] = arch_xtensa_cached_ptr(&refoutbuf[i][0]); } - bzero(cd->process_buffer, buf_size); - for (channel = 0; channel < cd->num_capture_channels; channel++) - cd->process_buffer_ptrs[channel] = &cd->process_buffer[channel * cd->num_frames]; - buf_size = cd->num_frames * sizeof(cd->aec_reference_buffer[0]) * - cd->num_aec_reference_channels; - comp_dbg(dev, "Allocating aec_reference_buffer of size %u", buf_size); - cd->aec_reference_buffer = rballoc(0, SOF_MEM_CAPS_RAM, buf_size); - if (!cd->aec_reference_buffer) { - comp_err(dev, "Allocating aec_reference_buffer failure"); - ret = -ENOMEM; - goto fail; - } - bzero(cd->aec_reference_buffer, buf_size); - for (channel = 0; channel < cd->num_aec_reference_channels; channel++) - cd->aec_reference_buffer_ptrs[channel] = - &cd->aec_reference_buffer[channel * cd->num_frames]; + cd->buffered_frames = 0; /* comp_is_new_data_blob_available always returns false for the first * control write with non-empty config. The first non-empty write may @@ -410,13 +690,10 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) fail: comp_err(dev, "google_rtc_audio_processing_init(): Failed"); if (cd) { - rfree(cd->aec_reference_buffer); if (cd->state) { GoogleRtcAudioProcessingFree(cd->state); } GoogleRtcAudioProcessingDetachMemoryBuffer(); - rfree(cd->memory_buffer); - rfree(cd->process_buffer); comp_data_blob_handler_free(cd->tuning_handler); rfree(cd); } @@ -432,15 +709,21 @@ static int google_rtc_audio_processing_free(struct processing_module *mod) GoogleRtcAudioProcessingFree(cd->state); cd->state = NULL; - rfree(cd->aec_reference_buffer); GoogleRtcAudioProcessingDetachMemoryBuffer(); - rfree(cd->memory_buffer); - rfree(cd->process_buffer); comp_data_blob_handler_free(cd->tuning_handler); rfree(cd); return 0; } +static bool is_ref_buffer(struct comp_dev *dev, struct comp_buffer *b) +{ +#if CONFIG_IPC_MAJOR_4 + return IPC4_SINK_QUEUE_ID(buf_get_id(b)) == SOF_AEC_FEEDBACK_QUEUE_ID; +#else + return b->source->pipeline->pipeline_id != dev->pipeline->pipeline_id; +#endif +} + static int google_rtc_audio_processing_prepare(struct processing_module *mod, struct sof_source **sources, int num_of_sources, @@ -450,304 +733,239 @@ static int google_rtc_audio_processing_prepare(struct processing_module *mod, struct comp_dev *dev = mod->dev; struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); struct list_item *source_buffer_list_item; - unsigned int aec_channels = 0, frame_fmt, rate; - int microphone_stream_channels = 0; - int output_stream_channels; - int ret; - int i = 0; + int ret = 0, i = 0; comp_info(dev, "google_rtc_audio_processing_prepare()"); - if (num_of_sources != GOOGLE_RTC_NUM_INPUT_PINS) { - comp_err(dev, "Expecting 2 sources - ref and mic, got %u", num_of_sources); + if (num_of_sources != 2 || num_of_sinks != 1) { + comp_err(dev, "Invalid source/sink count"); return -EINVAL; } - if (num_of_sinks != GOOGLE_RTC_NUM_OUTPUT_PINS) { - comp_err(dev, "Expecting 1 sink, got %u", num_of_sinks); - return -EINVAL; - } +#ifdef CONFIG_IPC_MAJOR_4 + prepare_ipc4_fmts(mod, sources, sinks); /* Workaround, see above */ +#endif /* searching for stream and feedback source buffers */ - for (i = 0; i < num_of_sources; i++) { - - if (IPC4_SINK_QUEUE_ID(source_get_id(sources[i])) == SOF_AEC_FEEDBACK_QUEUE_ID) { - + list_for_item(source_buffer_list_item, &dev->bsource_list) { + struct comp_buffer *source = container_of(source_buffer_list_item, + struct comp_buffer, sink_list); + if (is_ref_buffer(dev, source)) { cd->aec_reference_source = i; - aec_channels = source_get_channels(sources[i]); - comp_dbg(dev, "reference index = %d, channels = %d", i, aec_channels); + cd->ref_comp_buffer = source; } else { cd->raw_microphone_source = i; - microphone_stream_channels = source_get_channels(sources[i]); - comp_dbg(dev, "microphone index = %d, channels = %d", i, - microphone_stream_channels); } - source_set_alignment_constants(sources[i], 1, 1); + i++; } - /* enforce format on pins */ - ipc4_update_source_format(sources[cd->aec_reference_source], - &cd->config.reference_fmt); - ipc4_update_source_format(sources[cd->raw_microphone_source], - &mod->priv.cfg.base_cfg.audio_fmt); - ipc4_update_sink_format(sinks[0], &mod->priv.cfg.base_cfg.audio_fmt); - - /* On some platform the playback output is left right left right due to a crossover - * later on the signal processing chain. That makes the aec_reference be 4 channels - * and the AEC should only use the 2 first. + /* Validate channel, format and rate on each of our three + * inputs. All much match our build-time configuration, AEC + * does not handle dynamic stream formats. */ - if (cd->num_aec_reference_channels > aec_channels) { - comp_err(dev, "unsupported number of AEC reference channels: %d", - aec_channels); - return -EINVAL; + int ref_fmt = source_get_frm_fmt(sources[cd->aec_reference_source]); + int ref_chan = source_get_channels(sources[cd->aec_reference_source]); + int ref_rate = source_get_rate(sources[cd->aec_reference_source]); + + int mic_fmt = source_get_frm_fmt(sources[cd->raw_microphone_source]); + int mic_chan = source_get_channels(sources[cd->raw_microphone_source]); + int mic_rate = source_get_rate(sources[cd->raw_microphone_source]); + + int out_fmt = sink_get_frm_fmt(sinks[0]); + int out_chan = sink_get_channels(sinks[0]); + int out_rate = sink_get_rate(sinks[0]); + + cd->ref_framesz = source_get_frame_bytes(sources[cd->aec_reference_source]); + cd->cap_framesz = sink_get_frame_bytes(sinks[0]); + + cd->num_aec_reference_channels = MIN(ref_chan, CHAN_MAX); + cd->num_capture_channels = MIN(mic_chan, CHAN_MAX); + + /* Too many channels is a soft failure, AEC treats only the first N */ + if (mic_chan > CHAN_MAX) + comp_warn(dev, "Too many mic channels: %d, truncating to %d", + mic_chan, CHAN_MAX); + if (ref_chan > CHAN_MAX) + comp_warn(dev, "Too many ref channels: %d, truncating to %d", + ref_chan, CHAN_MAX); + + if (out_chan != mic_chan) { + comp_err(dev, "Input/output mic channel mismatch"); + ret = -EINVAL; } - sink_set_alignment_constants(sinks[0], 1, 1); - frame_fmt = sink_get_frm_fmt(sinks[0]); - rate = sink_get_rate(sinks[0]); - output_stream_channels = sink_get_channels(sinks[0]); - - if (cd->num_capture_channels > microphone_stream_channels) { - comp_err(dev, "unsupported number of microphone channels: %d", - microphone_stream_channels); - return -EINVAL; + if (ref_rate != mic_rate || ref_rate != out_rate || + ref_rate != CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ) { + comp_err(dev, "Incorrect source/sink sample rate, expect %d\n", + CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ); + ret = -EINVAL; } - if (cd->num_capture_channels > output_stream_channels) { - comp_err(dev, "unsupported number of output channels: %d", - output_stream_channels); - return -EINVAL; + if (mic_fmt != out_fmt) { + comp_err(dev, "Mismatched in/out frame format"); + ret = -EINVAL; } - switch (frame_fmt) { -#if CONFIG_FORMAT_S16LE - case SOF_IPC_FRAME_S16_LE: - break; -#endif /* CONFIG_FORMAT_S16LE */ - default: - comp_err(dev, "unsupported data format: %d", frame_fmt); - return -EINVAL; + if ((mic_fmt != SOF_IPC_FRAME_S32_LE && mic_fmt != SOF_IPC_FRAME_S16_LE) || + (ref_fmt != SOF_IPC_FRAME_S32_LE && ref_fmt != SOF_IPC_FRAME_S16_LE)) { + comp_err(dev, "Unsupported sample format"); + ret = -EINVAL; } - if (rate != CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_SAMPLE_RATE_HZ) { - comp_err(dev, "unsupported samplerate: %d", rate); - return -EINVAL; - } +#ifdef CONFIG_IPC_MAJOR_4 + int ref_bufsz = source_get_min_available(sources[cd->aec_reference_source]); + int mic_bufsz = source_get_min_available(sources[cd->raw_microphone_source]); + int out_bufsz = sink_get_min_free_space(sinks[0]); - /* check IBS/OBS in streams */ - if (cd->num_frames * source_get_frame_bytes(sources[cd->raw_microphone_source]) != - source_get_min_available(sources[cd->raw_microphone_source])) { - comp_err(dev, "Incorrect IBS on microphone source: %d, expected %u", - source_get_min_available(sources[cd->raw_microphone_source]), - cd->num_frames * - source_get_frame_bytes(sources[cd->raw_microphone_source])); - return -EINVAL; + if (mic_bufsz > cd->num_frames * cd->cap_framesz) { + comp_err(dev, "Mic IBS %d >1 AEC block, needless delay!", mic_bufsz); + ret = -EINVAL; } - if (cd->num_frames * sink_get_frame_bytes(sinks[0]) != - sink_get_min_free_space(sinks[0])) { - comp_err(dev, "Incorrect OBS on sink :%d, expected %u", - sink_get_min_free_space(sinks[0]), - cd->num_frames * sink_get_frame_bytes(sinks[0])); - return -EINVAL; + + if (ref_bufsz > cd->num_frames * cd->ref_framesz) { + comp_err(dev, "Ref IBS %d >1 one AEC block, needless delay!", ref_bufsz); + ret = -EINVAL; } - if (cd->num_frames * source_get_frame_bytes(sources[cd->aec_reference_source]) != - source_get_min_available(sources[cd->aec_reference_source])) { - comp_err(dev, "Incorrect IBS on reference source: %d, expected %u", - source_get_min_available(sources[cd->aec_reference_source]), - cd->num_frames * - source_get_frame_bytes(sources[cd->aec_reference_source])); - return -EINVAL; + + if (out_bufsz < cd->num_frames * cd->cap_framesz) { + comp_err(dev, "Capture OBS %d too small, must fit 1 AEC block", out_bufsz); + ret = -EINVAL; } +#endif + + if (ret < 0) + return ret; + + cd->mic_copy = mic_fmt == SOF_IPC_FRAME_S16_LE ? source_copy16 : source_copy32; + cd->ref_copy = ref_fmt == SOF_IPC_FRAME_S16_LE ? source_copy16 : source_copy32; + cd->out_copy = out_fmt == SOF_IPC_FRAME_S16_LE ? sink_copy16 : sink_copy32; + + cd->last_ref_ok = false; + + ret = GoogleRtcAudioProcessingSetStreamFormats(cd->state, mic_rate, + cd->num_capture_channels, + cd->num_capture_channels, + ref_rate, cd->num_aec_reference_channels); /* Blobs sent during COMP_STATE_READY is assigned to blob_handler->data * directly, so comp_is_new_data_blob_available always returns false. */ - ret = google_rtc_audio_processing_reconfigure(mod); - if (ret) - return ret; + if (ret == 0) + ret = google_rtc_audio_processing_reconfigure(mod); - comp_dbg(dev, "google_rtc_audio_processing_prepare() success"); - return 0; + return ret; +} + +static int trigger_handler(struct processing_module *mod, int cmd) +{ + struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); + + /* Ignore and halt propagation if we get a trigger from the + * playback pipeline: not for us. + */ + if (cd->ref_comp_buffer->walking) + return PPL_STATUS_PATH_STOP; + + /* Note: not module_adapter_set_state(). With IPC4 those are + * identical, but IPC3 has some odd-looking logic that + * validates that no sources are active when receiving a + * PRE_START command, which obviously breaks for our reference + * stream if playback was already running when our pipeline + * started + */ + return comp_set_state(mod->dev, cmd); } static int google_rtc_audio_processing_reset(struct processing_module *mod) { comp_dbg(mod->dev, "google_rtc_audio_processing_reset()"); - return 0; } -static inline int16_t convert_google_aec_format_to_int16(BUF_TYPE data) +static inline void execute_aec(struct google_rtc_audio_processing_comp_data *cd) { -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API - const xtfloat ratio = 2 << 14; - xtfloat x0 = data; - xtfloat x1; - int16_t x; - - x1 = XT_MUL_S(x0, ratio); - x = XT_TRUNC_S(x1, 0); - - return x; -#else /* CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API */ - return data; -#endif + /* Note that reference input and mic output share the same + * buffer for efficiency + */ + GoogleRtcAudioProcessingAnalyzeRender_float32(cd->state, + (const float **)cd->refout_buffers); + GoogleRtcAudioProcessingProcessCapture_float32(cd->state, + (const float **)cd->raw_mic_buffers, + cd->refout_buffers); + cd->buffered_frames = 0; } -static inline BUF_TYPE convert_int16_to_google_aec_format(int16_t data) +static bool ref_stream_active(struct google_rtc_audio_processing_comp_data *cd) { -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API - const xtfloat ratio = 2 << 14; - xtfloat x0 = data; - float x; - - x = XT_DIV_S(x0, ratio); - - return x; -#else /* CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API */ - return data; -#endif + return cd->ref_comp_buffer->source && + cd->ref_comp_buffer->source->state == COMP_STATE_ACTIVE; } -/* todo CONFIG_FORMAT_S32LE */ -static int google_rtc_audio_processing_process(struct processing_module *mod, - struct sof_source **sources, int num_of_sources, - struct sof_sink **sinks, int num_of_sinks) +static int mod_process(struct processing_module *mod, struct sof_source **sources, + int num_of_sources, struct sof_sink **sinks, int num_of_sinks) { - - int ret; - uint16_t const *src; - uint8_t const *src_buf_start; - uint8_t const *src_buf_end; - size_t src_buf_size; - - uint16_t const *ref; - uint8_t const *ref_buf_start; - uint8_t const *ref_buf_end; - size_t ref_buf_size; - - uint16_t *dst; - uint8_t *dst_buf_start; - uint8_t *dst_buf_end; - size_t dst_buf_size; - - size_t num_of_bytes_to_process; - int num_samples_remaining; - int num_frames_remaining; - int channel; - int nmax; - - struct sof_source *ref_stream, *src_stream; - struct sof_sink *dst_stream; - struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); - if (cd->reconfigure) { - ret = google_rtc_audio_processing_reconfigure(mod); - if (ret) - return ret; - } + if (cd->reconfigure) + google_rtc_audio_processing_reconfigure(mod); - src_stream = sources[cd->raw_microphone_source]; - ref_stream = sources[cd->aec_reference_source]; - dst_stream = sinks[0]; + struct sof_source *mic = sources[cd->raw_microphone_source]; + struct sof_source *ref = sources[cd->aec_reference_source]; + struct sof_sink *out = sinks[0]; + bool ref_ok = ref_stream_active(cd); - num_of_bytes_to_process = cd->num_frames * source_get_frame_bytes(ref_stream); - ret = source_get_data(ref_stream, num_of_bytes_to_process, (const void **)&ref, - (const void **)&ref_buf_start, &ref_buf_size); - - /* problems here are extremely unlikely, as it has been checked that - * the buffer contains enough data - */ - assert(!ret); - ref_buf_end = ref_buf_start + ref_buf_size; + /* Clear the buffer if the reference pipeline shuts off */ + if (!ref_ok && cd->last_ref_ok) + bzero(arch_xtensa_cached_ptr(refoutbuf), sizeof(refoutbuf)); + int fmic = source_get_data_frames_available(mic); + int fref = source_get_data_frames_available(ref); + int frames = ref_ok ? MIN(fmic, fref) : fmic; + int n, frames_rem; - /* can't use source_get_data_frames_available as number of available data may have changed - * other processes may put some data to the buffer + /* If fref > fmic at the startup of the reference stream, we + * should consume the early samples so AEC compares the most + * recent values. It's common to have stale reference data + * waiting in the pipe when the first capture bytes arrive. */ - num_samples_remaining = num_of_bytes_to_process * source_get_channels(ref_stream) / - source_get_frame_bytes(ref_stream); - - /* de-interlace ref buffer, convert it to float */ - for (int i = 0; i < cd->num_frames; i++) { - for (channel = 0; channel < cd->num_aec_reference_channels; ++channel) { - cd->aec_reference_buffer_ptrs[channel][i] = - convert_int16_to_google_aec_format(ref[channel]); - } - ref += cd->num_aec_reference_channels; - if ((void *)ref >= (void *)ref_buf_end) - ref = (void *)ref_buf_start; + if (ref_ok && !cd->last_ref_ok && fref > fmic) { + comp_info(mod->dev, "Ref startup, prune %d stale frames", fref-fmic); + source_release_data(ref, (fref - fmic) * cd->ref_framesz); } -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API - GoogleRtcAudioProcessingAnalyzeRender_float32( - cd->state, - (const float **)cd->aec_reference_buffer_ptrs); -#else - GoogleRtcAudioProcessingAnalyzeRender_int16( - cd->state, - (const int16_t *)cd->aec_reference_buffer); -#endif - source_release_data(ref_stream, num_of_bytes_to_process); - - /* process main stream - de interlace and convert */ - num_of_bytes_to_process = cd->num_frames * source_get_frame_bytes(src_stream); - ret = source_get_data(src_stream, num_of_bytes_to_process, (const void **)&src, - (const void **)&src_buf_start, &src_buf_size); - assert(!ret); - src_buf_end = src_buf_start + src_buf_size; + for (frames_rem = frames; frames_rem; frames_rem -= n) { + n = MIN(frames_rem, cd->num_frames - cd->buffered_frames); - for (int i = 0; i < cd->num_frames; i++) { - for (channel = 0; channel < cd->num_capture_channels; channel++) - cd->process_buffer_ptrs[channel][i] = - convert_int16_to_google_aec_format(src[channel]); + cd->mic_copy(mic, n, cd->raw_mic_buffers, cd->buffered_frames); - src += cd->num_capture_channels; - if ((void *)src >= (void *)src_buf_end) - src = (void *)src_buf_start; - } - - source_release_data(src_stream, num_of_bytes_to_process); + if (ref_ok) + cd->ref_copy(ref, n, cd->refout_buffers, cd->buffered_frames); - /* call the library, use same in/out buffers */ -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API - GoogleRtcAudioProcessingProcessCapture_float32(cd->state, - (const float **)cd->process_buffer_ptrs, - cd->process_buffer_ptrs); -#else - GoogleRtcAudioProcessingProcessCapture_int16(cd->state, - (const int16_t *)cd->process_buffer, - cd->process_buffer); -#endif + cd->buffered_frames += n; - /* same numnber of bytes to process for output stream as for mic stream */ - ret = sink_get_buffer(dst_stream, num_of_bytes_to_process, (void **)&dst, - (void **)&dst_buf_start, &dst_buf_size); - assert(!ret); - dst_buf_end = dst_buf_start + dst_buf_size; + if (cd->buffered_frames >= cd->num_frames) { + /* Safety valve; OBS only guarantees us space for one block */ + if (sink_get_free_size(out) < cd->num_frames * cd->cap_framesz) { + comp_warn(mod->dev, "AEC sink backed up!"); + break; + } - for (int i = 0; i < cd->num_frames; i++) { - for (channel = 0; channel < cd->num_capture_channels; channel++) - dst[channel] = convert_google_aec_format_to_int16( - cd->process_buffer_ptrs[channel][i]); - dst += cd->num_capture_channels; - if ((void *)dst >= (void *)dst_buf_end) - dst = (void *)dst_buf_start; + execute_aec(cd); + cd->out_copy(out, cd->num_frames, cd->refout_buffers); + } } - - sink_commit_buffer(dst_stream, num_of_bytes_to_process); - + cd->last_ref_ok = ref_ok; return 0; } static struct module_interface google_rtc_audio_processing_interface = { .init = google_rtc_audio_processing_init, .free = google_rtc_audio_processing_free, - .process = google_rtc_audio_processing_process, + .process = mod_process, .prepare = google_rtc_audio_processing_prepare, .set_configuration = google_rtc_audio_processing_set_config, .get_configuration = google_rtc_audio_processing_get_config, + .trigger = trigger_handler, .reset = google_rtc_audio_processing_reset, }; diff --git a/src/audio/google/google_rtc_audio_processing_mock.c b/src/audio/google/google_rtc_audio_processing_mock.c index a20a31ae1a6c..681d003d3592 100644 --- a/src/audio/google/google_rtc_audio_processing_mock.c +++ b/src/audio/google/google_rtc_audio_processing_mock.c @@ -10,9 +10,6 @@ #include #include -#include -#include - #include #include "ipc/topology.h" @@ -24,11 +21,7 @@ struct GoogleRtcAudioProcessingState { int num_aec_reference_channels; int num_output_channels; int num_frames; -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API float *aec_reference; -#else - int16_t *aec_reference; -#endif }; static void SetFormats(GoogleRtcAudioProcessingState *const state, @@ -145,9 +138,8 @@ int GoogleRtcAudioProcessingReconfigure(GoogleRtcAudioProcessingState *const sta return 0; } -#if CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API -int GoogleRtcAudioProcessingProcessCapture_float32(GoogleRtcAudioProcessingState *const state, - const float *const *src, +int GoogleRtcAudioProcessingProcessCapture_float32(GoogleRtcAudioProcessingState * const state, + const float * const *src, float * const *dest) { float *ref = state->aec_reference; @@ -167,8 +159,8 @@ int GoogleRtcAudioProcessingProcessCapture_float32(GoogleRtcAudioProcessingState return 0; } -int GoogleRtcAudioProcessingAnalyzeRender_float32(GoogleRtcAudioProcessingState *const state, - const float *const *data) +int GoogleRtcAudioProcessingAnalyzeRender_float32(GoogleRtcAudioProcessingState * const state, + const float * const *data) { const size_t buffer_size = sizeof(state->aec_reference[0]) @@ -182,44 +174,6 @@ int GoogleRtcAudioProcessingAnalyzeRender_float32(GoogleRtcAudioProcessingState return 0; } -#else /* CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API */ -int GoogleRtcAudioProcessingProcessCapture_int16(GoogleRtcAudioProcessingState *const state, - const int16_t *const src, - int16_t *const dest) -{ - int16_t *ref = state->aec_reference; - int16_t *mic = (int16_t *) src; - int16_t *out = dest; - int n, chan; - - for (chan = 0; chan < state->num_output_channels; chan++) { - for (n = 0; n < state->num_frames; ++n) { - int16_t mic_save = mic[n + (chan * state->num_frames)]; - - if (chan < state->num_aec_reference_channels) - dest[n + (chan * state->num_frames)] = - mic_save + ref[n + (chan * state->num_frames)]; - else - dest[n + (chan * state->num_frames)] = mic_save; - } - } - - return 0; -} - -int GoogleRtcAudioProcessingAnalyzeRender_int16(GoogleRtcAudioProcessingState *const state, - const int16_t *const data) -{ - const size_t buffer_size = - sizeof(state->aec_reference[0]) - * state->num_frames - * state->num_aec_reference_channels; - memcpy_s(state->aec_reference, buffer_size, - data, buffer_size); - return 0; -} - -#endif /* CONFIG_COMP_GOOGLE_RTC_USE_32_BIT_FLOAT_API */ void GoogleRtcAudioProcessingParseSofConfigMessage(uint8_t *message, size_t message_size, From 511dbafab60e945e07ea3d26cdc253aa4f037d59 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 2 Jan 2024 18:04:03 -0800 Subject: [PATCH 10/10] [DNM] Google AEC: minimize workaround This is down to just one line now with existing (still in review) PRs applied --- .../google/google_rtc_audio_processing.c | 116 +----------------- 1 file changed, 1 insertion(+), 115 deletions(-) diff --git a/src/audio/google/google_rtc_audio_processing.c b/src/audio/google/google_rtc_audio_processing.c index 27960632bd83..aff45a268662 100644 --- a/src/audio/google/google_rtc_audio_processing.c +++ b/src/audio/google/google_rtc_audio_processing.c @@ -78,10 +78,6 @@ static __aligned(PLATFORM_DCACHE_ALIGN) float micbuf[CHAN_MAX][NUM_FRAMES]; struct google_rtc_audio_processing_comp_data { -#ifdef CONFIG_IPC_MAJOR_4 - struct ipc4_audio_format in_fmts[2]; - struct ipc4_audio_format out_fmt; -#endif uint32_t num_frames; int num_aec_reference_channels; int num_capture_channels; @@ -112,110 +108,6 @@ void GoogleRtcFree(void *ptr) return rfree(ptr); } -#ifdef CONFIG_IPC_MAJOR_4 -/* Workaround: IPC4 fails to set up stream metadata on the buffer - * objects at pipeline creation time, and traditionally components - * have been responsible for doing this. The way this works is that - * during the init() call, we cache the relevant ipc4_audio_format - * records from the module "extended config" (which is untyped - * byte-packed data, I think it's raw bytes from the host?) that - * correspond to our connected streams. This config struct gets freed - * after initialization, so it has to be done then. - * - * Then at prepare time, we must use ipc4_update_source/sink_format() - * to set values on the (incompletely initialized) streams. - * Similarly, we have to call audio_stream_init_alignment_constants(), - * otherwise needed values on the audio stream remain uninitialized. - * - * It's not really documented what happens if our settings disagree - * with the component on the other side of the buffer! Presumably - * that would be a fatal topology bug, but nothing is set up to detect - * it. In fact in practice most other components DON'T do this, so - * our settings win (and thus we must do this, or else the rest of the - * stream sees garbage and misbehaves, generally with buffer overruns - * or short reads). - * - * Finally, there is somewhat... unique handling needed to get correct - * IBS/OBS buffer sizing. We have two inputs with different stream - * formats, and thus different minimum block sizes. The "base" module - * config only has space for one IBS value. So the pipeline code has - * an "extended" config with separate IBS per pin, but it will only - * use it if it knows about it, which it doesn't by default because - * module initialization throws away the data! So it falls to us to - * duplicate a copy and store it ourselves, in a separate field from - * where we found it. - */ -static int init_ipc4_fmts(struct processing_module *mod) -{ - int i; - struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); - const struct ipc4_base_module_extended_cfg *bmec = mod->priv.cfg.init_data; - const struct ipc4_input_pin_format *ipf; - const struct ipc4_output_pin_format *opf; - const struct ipc4_base_module_cfg_ext *bce; - - if (!bmec) - return -EINVAL; - - bce = &bmec->base_cfg_ext; - if (bce->nb_input_pins != 2 && bce->nb_output_pins != 1) { - comp_err(mod->dev, "Must have 2 input, 1 output pins"); - return -EINVAL; - } - - ipf = (void *)&bce->pin_formats[0]; - for (i = 0; i < bce->nb_input_pins; i++, ipf++) { - if (ipf->pin_index < 0 || ipf->pin_index >= 2) { - comp_err(mod->dev, "Incorrect input pin index %d", ipf->pin_index); - return -EINVAL; - } - cd->in_fmts[ipf->pin_index] = ipf->audio_fmt; - } - - opf = (void *)ipf; - if (opf->pin_index != 0) { - comp_err(mod->dev, "Incorrect output pin index %d\n", opf->pin_index); - return -EINVAL; - } - cd->out_fmt = opf->audio_fmt; - - size_t bcesz = sizeof(*bce) + (bce->nb_input_pins * sizeof(*ipf) - + bce->nb_output_pins * sizeof(*opf)); - void *bcedup = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, bcesz); - - if (!bcedup) - return -ENOMEM; - memcpy(bcedup, bce, bcesz); - mod->priv.cfg.basecfg_ext = bcedup; - - return 0; -} - -static void prepare_ipc4_fmts(struct processing_module *mod, - struct sof_source **sources, - struct sof_sink **sinks) -{ - struct google_rtc_audio_processing_comp_data *cd = module_get_private_data(mod); - - /* FIXME: The "pin" index (the value of the pin_index field of - * the ipc4_in/output_pin_format structs seen in module config - * at init() time) and the "source" index (the ordering of the - * sources[] argument to prepare()/process()) ARE PRESENTED - * BACKWARDS! And I don't see any API to tell me which is - * which (note that the ordering of the connected buffers in - * the comp_dev sink/source lists is a THIRD convention, and - * matches sources[]/sinks[]). - */ - ipc4_update_source_format(sources[0], &cd->in_fmts[1]); - ipc4_update_source_format(sources[1], &cd->in_fmts[0]); - ipc4_update_sink_format(sinks[0], &cd->out_fmt); - - source_set_alignment_constants(sources[0], 1, 1); - source_set_alignment_constants(sources[1], 1, 1); - sink_set_alignment_constants(sinks[0], 1, 1); -} -#endif - static ALWAYS_INLINE float s16_to_float(const char *ptr) { float scale = -(float)SHRT_MIN; @@ -623,12 +515,6 @@ static int google_rtc_audio_processing_init(struct processing_module *mod) md->private = cd; -#ifdef CONFIG_IPC_MAJOR_4 - ret = init_ipc4_fmts(mod); /* workaround, see above */ - if (ret < 0) - goto fail; -#endif - cd->tuning_handler = comp_data_blob_handler_new(dev); if (!cd->tuning_handler) { ret = -ENOMEM; @@ -743,7 +629,7 @@ static int google_rtc_audio_processing_prepare(struct processing_module *mod, } #ifdef CONFIG_IPC_MAJOR_4 - prepare_ipc4_fmts(mod, sources, sinks); /* Workaround, see above */ + ipc4_update_source_format(sources[0], &mod->priv.cfg.input_pins[1].audio_fmt); #endif /* searching for stream and feedback source buffers */