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.

Sign up for GitHub

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

Jump to bottom

Crossover refactor #8652

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Crossover refactor #8652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db06d28
Audio: crossover: create share header file for crossover and multiban…
btian1 Dec 20, 2023
558a862
crossover: move header file to module folder
btian1 Dec 20, 2023
49be689
Crossover: move some inline functions to header file
btian1 Dec 20, 2023
7779866
audio: crossover: split out ipc3 and ipc4 specific code
btian1 Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/audio/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ if(CONFIG_IPC_MAJOR_3)
set(tdfb_sources multiband_drc/multiband_drc_ipc3.c)
set(dcblock_sources dcblock/dcblock_ipc3.c)
set(mux_sources mux/mux_ipc3.c)
set(crossover_sources crossover/crossover_ipc3.c)
elseif(CONFIG_IPC_MAJOR_4)
set(volume_sources volume/volume.c volume/volume_generic.c volume/volume_ipc4.c)
set(asrc_sources asrc/asrc_ipc4.c)
Expand All @@ -195,6 +196,7 @@ elseif(CONFIG_IPC_MAJOR_4)
set(tdfb_sources multiband_drc/multiband_drc_ipc4.c)
set(dcblock_sources dcblock/dcblock_ipc4.c)
set(mux_sources mux/mux_ipc4.c)
set(crossover_sources crossover/crossover_ipc4.c)
endif()
set(mixer_sources ${mixer_src})
set(asrc_sources asrc/asrc.c asrc/asrc_farrow.c asrc/asrc_farrow_generic.c)
Expand All @@ -204,7 +206,7 @@ set(dcblock_sources dcblock/dcblock.c dcblock/dcblock_generic.c dcblock/dcblock_
set(crossover_sources crossover/crossover.c crossover/crossover_generic.c)
set(tdfb_sources tdfb/tdfb.c tdfb/tdfb_generic.c tdfb/tdfb_direction.c)
set(drc_sources drc/drc.c drc/drc_generic.c drc/drc_math_generic.c)
set(multiband_drc_sources multiband_drc/multiband_drc_generic.c crossover/crossover.c crossover/crossover_generic.c drc/drc.c drc/drc_generic.c drc/drc_math_generic.c multiband_drc/multiband_drc.c )
set(multiband_drc_sources multiband_drc/multiband_drc_generic.c crossover/crossover.c drc/drc.c drc/drc_generic.c drc/drc_math_generic.c multiband_drc/multiband_drc.c )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure it fits scope of this PR but eventually the crossover/crossover.c content should move to maths library where it's enabled by kconfig if crossover or multiband-drc component is built. I think the current sources linking duplicates the crossover .text and consumes more RAM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all of crossover.c (e.g. prepare and other component ops) would be in maths library but only the common filter related functions those are really shared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to figure out yesterday, however, the common part is prepare / params, not actually the processing functions, not easy to move filter related to math, you can check crossover_common.h
all listed are required by multiband_drc.

Regarding linking, I searched sof assembly, take crossover.c:
static void crossover_s32_default(struct comp_data *cd,
struct input_stream_buffer *bsource,
struct output_stream_buffer **bsinks,
int32_t num_sinks,
uint32_t frames)

as example, it only have one time results.

also for crossover_common part, all are embeded as inline, no more duplicated code found in assembly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now move it to include/module, seems more reasonable compared with math, since most of common part does not related with math.

set(mfcc_sources mfcc/mfcc.c mfcc/mfcc_setup.c mfcc/mfcc_common.c mfcc/mfcc_generic.c mfcc/mfcc_hifi4.c mfcc/mfcc_hifi3.c)
set(mux_sources mux/mux.c mux/mux_generic.c)

Expand Down
5 changes: 5 additions & 0 deletions src/audio/crossover/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
add_local_sources(sof crossover.c)
add_local_sources(sof crossover_generic.c)
if(CONFIG_IPC_MAJOR_3)
add_local_sources(sof crossover_ipc3.c)
elseif(CONFIG_IPC_MAJOR_4)
add_local_sources(sof crossover_ipc4.c)
endif()
221 changes: 17 additions & 204 deletions src/audio/crossover/crossover.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@
#include <sof/audio/format.h>
#include <sof/audio/pipeline.h>
#include <sof/audio/ipc-config.h>
#include <sof/audio/crossover/crossover.h>
#include <sof/audio/crossover/crossover_algorithm.h>
#include <module/crossover/crossover_common.h>
#include <sof/common.h>
#include <rtos/panic.h>
#include <sof/ipc/msg.h>
Expand All @@ -30,13 +29,15 @@
#include <ipc/stream.h>
#include <ipc/topology.h>
#include <user/trace.h>
#include <user/crossover.h>
#include <user/eq.h>
#include <errno.h>
#include <stddef.h>
#include <stdint.h>
#include <limits.h>

#include "crossover_user.h"
#include "crossover.h"

LOG_MODULE_REGISTER(crossover, CONFIG_SOF_LOG_LEVEL);

/* 948c9ad1-806a-4131-ad6c-b2bda9e35a9f */
Expand All @@ -45,43 +46,11 @@ DECLARE_SOF_RT_UUID("crossover", crossover_uuid, 0x948c9ad1, 0x806a, 0x4131,

DECLARE_TR_CTX(crossover_tr, SOF_UUID(crossover_uuid), LOG_LEVEL_INFO);

static inline void crossover_free_config(struct sof_crossover_config **config)
{
rfree(*config);
*config = NULL;
}

/**
* \brief Reset the state of an LR4 filter.
*/
static inline void crossover_reset_state_lr4(struct iir_state_df2t *lr4)
{
rfree(lr4->coef);
rfree(lr4->delay);

lr4->coef = NULL;
lr4->delay = NULL;
}

/**
* \brief Reset the state (coefficients and delay) of the crossover filter
* of a single channel.
*/
inline void crossover_reset_state_ch(struct crossover_state *ch_state)
{
int i;

for (i = 0; i < CROSSOVER_MAX_LR4; i++) {
crossover_reset_state_lr4(&ch_state->lowpass[i]);
crossover_reset_state_lr4(&ch_state->highpass[i]);
}
}

/**
* \brief Reset the state (coefficients and delay) of the crossover filter
* across all channels
*/
static inline void crossover_reset_state(struct comp_data *cd)
static void crossover_reset_state(struct comp_data *cd)
{
int i;

Expand All @@ -97,8 +66,8 @@ static inline void crossover_reset_state(struct comp_data *cd)
* \return the position at which pipe_id is found in config->assign_sink.
* -EINVAL if not found.
*/
static int crossover_get_stream_index(struct processing_module *mod,
struct sof_crossover_config *config, uint32_t pipe_id)
int crossover_get_stream_index(struct processing_module *mod,
struct sof_crossover_config *config, uint32_t pipe_id)
{
int i;
uint32_t *assign_sink = config->assign_sink;
Expand Down Expand Up @@ -142,11 +111,7 @@ static int crossover_assign_sinks(struct processing_module *mod,
unsigned int sink_id, state;

sink = container_of(sink_list, struct comp_buffer, source_list);
#if CONFIG_IPC_MAJOR_4
sink_id = cd->output_pin_index[j];
#else
sink_id = sink->pipeline_id;
#endif
sink_id = crossover_get_sink_id(cd, sink->pipeline_id, j);
state = sink->sink->state;
if (state != dev->state) {
j++;
Expand Down Expand Up @@ -326,42 +291,6 @@ static int crossover_setup(struct processing_module *mod, int nch)
return ret;
}

#if CONFIG_IPC_MAJOR_4
/* Note: Crossover needs to have in the rimage manifest the init_config set to 1 to let
* kernel know that the base_cfg_ext needs to be appended to the IPC payload. The
* Extension is needed to know the output pin indices.
*/
static int crossover_init_output_pins(struct processing_module *mod)
{
struct comp_data *cd = module_get_private_data(mod);
struct comp_dev *dev = mod->dev;
const struct ipc4_base_module_extended_cfg *base_cfg = mod->priv.cfg.init_data;
uint16_t num_input_pins = base_cfg->base_cfg_ext.nb_input_pins;
uint16_t num_output_pins = base_cfg->base_cfg_ext.nb_output_pins;
struct ipc4_input_pin_format *input_pin;
struct ipc4_output_pin_format *output_pin;
int i;

comp_dbg(dev, "Number of input pins %u, output pins %u", num_input_pins, num_output_pins);

if (num_input_pins != 1 || num_output_pins > SOF_CROSSOVER_MAX_STREAMS) {
comp_err(dev, "Illegal number of pins %u %u", num_input_pins, num_output_pins);
return -EINVAL;
}

input_pin = (struct ipc4_input_pin_format *)base_cfg->base_cfg_ext.pin_formats;
output_pin = (struct ipc4_output_pin_format *)(input_pin + 1);
cd->num_output_pins = num_output_pins;
comp_dbg(dev, "input pin index = %u", input_pin->pin_index);
for (i = 0; i < num_output_pins; i++) {
comp_dbg(dev, "output pin %d index = %u", i, output_pin[i].pin_index);
cd->output_pin_index[i] = output_pin[i].pin_index;
}

return 0;
}
#endif

/**
* \brief Creates a Crossover Filter component.
* \return Pointer to Crossover Filter component device.
Expand Down Expand Up @@ -405,13 +334,11 @@ static int crossover_init(struct processing_module *mod)
goto cd_fail;
}

#if CONFIG_IPC_MAJOR_4
ret = crossover_init_output_pins(mod);
ret = crossover_output_pin_init(mod);
if (ret < 0) {
comp_err(dev, "crossover_init(): crossover_init_output_pins() failed.");
goto cd_fail;
}
#endif

crossover_reset_state(cd);
return 0;
Expand Down Expand Up @@ -439,82 +366,6 @@ static int crossover_free(struct processing_module *mod)
return 0;
}

#if CONFIG_IPC_MAJOR_4
/**
* \brief Check sink streams configuration for matching pin index for output pins
*/
static int crossover_check_sink_assign(struct processing_module *mod,
struct sof_crossover_config *config)
{
struct comp_data *cd = module_get_private_data(mod);
struct comp_dev *dev = mod->dev;
uint32_t pin_index;
int num_assigned_sinks = 0;
int i, j;
uint8_t assigned_sinks[SOF_CROSSOVER_MAX_STREAMS] = {0};

for (j = 0; j < cd->num_output_pins; j++) {
pin_index = cd->output_pin_index[j];
i = crossover_get_stream_index(mod, config, pin_index);
if (i < 0) {
comp_warn(dev, "crossover_check_sink_assign(), could not assign sink %u",
pin_index);
break;
}

if (assigned_sinks[i]) {
comp_warn(dev, "crossover_check_sink_assign(), multiple sinks from pin %u are assigned",
pin_index);
break;
}

assigned_sinks[i] = true;
num_assigned_sinks++;
}

return num_assigned_sinks;
}
#else
/**
* \brief Check sink streams configuration for matching pipeline IDs
*/
static int crossover_check_sink_assign(struct processing_module *mod,
struct sof_crossover_config *config)
{
struct comp_dev *dev = mod->dev;
struct comp_buffer *sink;
struct list_item *sink_list;
int num_assigned_sinks = 0;
uint8_t assigned_sinks[SOF_CROSSOVER_MAX_STREAMS] = {0};
int i;

list_for_item(sink_list, &dev->bsink_list) {
unsigned int pipeline_id;

sink = container_of(sink_list, struct comp_buffer, source_list);
pipeline_id = sink->pipeline_id;

i = crossover_get_stream_index(mod, config, pipeline_id);
if (i < 0) {
comp_warn(dev, "crossover_check_sink_assign(), could not assign sink %d",
pipeline_id);
break;
}

if (assigned_sinks[i]) {
comp_warn(dev, "crossover_check_sink_assign(), multiple sinks from pipeline %d are assigned",
pipeline_id);
break;
}

assigned_sinks[i] = true;
num_assigned_sinks++;
}

return num_assigned_sinks;
}
#endif

/**
* \brief Verifies that the config is formatted correctly.
*
Expand Down Expand Up @@ -563,20 +414,13 @@ static int crossover_set_config(struct processing_module *mod, uint32_t config_i
size_t response_size)
{
struct comp_data *cd = module_get_private_data(mod);
int ret;

comp_info(mod->dev, "crossover_set_config()");

#if CONFIG_IPC_MAJOR_3
/* TODO: This check seems to work only for IPC3, FW crash happens from reject from
* topology embedded blob.
*/
struct sof_ipc_ctrl_data *cdata = (struct sof_ipc_ctrl_data *)fragment;

if (cdata->cmd != SOF_CTRL_CMD_BINARY) {
comp_err(mod->dev, "crossover_set_config(), invalid command");
return -EINVAL;
}
#endif
ret = crossover_check_config(mod, fragment);
if (ret < 0)
return ret;

return comp_data_blob_set(cd->model_handler, pos, data_offset_size, fragment,
fragment_size);
Expand All @@ -588,16 +432,13 @@ static int crossover_get_config(struct processing_module *mod,
{
struct comp_data *cd = module_get_private_data(mod);
struct sof_ipc_ctrl_data *cdata = (struct sof_ipc_ctrl_data *)fragment;
int ret;

comp_info(mod->dev, "crossover_get_config()");

#if CONFIG_IPC_MAJOR_3

if (cdata->cmd != SOF_CTRL_CMD_BINARY) {
comp_err(mod->dev, "crossover_get_config(), invalid command");
return -EINVAL;
}
#endif
ret = crossover_check_config(mod, fragment);
if (ret < 0)
return ret;

return comp_data_blob_get_cmd(cd->model_handler, cdata, fragment_size);
}
Expand Down Expand Up @@ -678,32 +519,6 @@ static int crossover_process_audio_stream(struct processing_module *mod,
return 0;
}

#if CONFIG_IPC_MAJOR_4
/**
* \brief IPC4 specific component prepare, updates source and sink buffers formats from base_cfg
*/
static void crossover_params(struct processing_module *mod)
{
struct sof_ipc_stream_params *params = mod->stream_params;
struct comp_buffer *sinkb, *sourceb;
struct list_item *sink_list;
struct comp_dev *dev = mod->dev;

comp_dbg(dev, "crossover_params()");

ipc4_base_module_cfg_to_stream_params(&mod->priv.cfg.base_cfg, params);
component_set_nearest_period_frames(dev, params->rate);

sourceb = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);
ipc4_update_buffer_format(sourceb, &mod->priv.cfg.base_cfg.audio_fmt);

list_for_item(sink_list, &dev->bsink_list) {
sinkb = container_of(sink_list, struct comp_buffer, source_list);
ipc4_update_buffer_format(sinkb, &mod->priv.cfg.base_cfg.audio_fmt);
}
}
#endif

/**
* \brief Prepares Crossover Filter component for processing.
* \param[in,out] dev Crossover Filter base component device.
Expand All @@ -722,9 +537,7 @@ static int crossover_prepare(struct processing_module *mod,

comp_info(dev, "crossover_prepare()");

#if CONFIG_IPC_MAJOR_4
crossover_params(mod);
#endif

/* Crossover has a variable number of sinks */
mod->max_sinks = SOF_CROSSOVER_MAX_STREAMS;
Expand Down
Loading
Loading