Skip to content

Audio: MDRC: Restructure Multiband DRC for more effective memory alloc… #9195

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

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

Audio: MDRC: Restructure Multiband DRC for more effective memory alloc… #9195

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cc36e4
MDRC: Optimize initialization in multiband_drc_init_coef
ShriramShastry Aug 18, 2024
40200c9
MDRC: Enhance Initialization of multiband_drc_init
ShriramShastry Aug 18, 2024
ddbdb17
MDRC: Improve Resource Cleanup in multiband_drc_free
ShriramShastry Aug 18, 2024
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
152 changes: 124 additions & 28 deletions src/audio/multiband_drc/multiband_drc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// Copyright(c) 2020 Google LLC. All rights reserved.
//
// Author: Pin-chih Lin <johnylin@google.com>
// Shriram Shastry <malladi.sastry@linux.intel.com>

#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/buffer.h>
Expand Down Expand Up @@ -92,18 +93,40 @@ static int multiband_drc_eq_init_coef_ch(struct sof_eq_iir_biquad *coef,
return 0;
}

static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, uint32_t rate)
/**
* \brief Initializes coefficients for the multiband DRC processing module.
*
* This function performs initialization of the crossover, emphasis, and
* deemphasis coefficients for each channel of the multiband DRC. It also
* allocates memory for pre-delay buffers and sets the delay time for each band.
* Memory is allocated ensuring alignment requirements are met to guarantee
* efficient access to the data structures.
*
* \param[in] mod Processing module containing pertinent data.
* \param[in] nch Number of channels to process.
* \param[in] rate Sample rate of the audio stream.
*
* \return 0 on success, error code otherwise.
Copy link
Contributor

@cujomalainey cujomalainey Aug 28, 2024

Choose a reason for hiding this comment

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

this should be a separate commit

*/
static int
multiband_drc_init_coef(struct processing_module *mod, int16_t nch, uint32_t rate)
{
struct comp_dev *dev = mod->dev;
struct multiband_drc_comp_data *cd = module_get_private_data(mod);
struct sof_eq_iir_biquad *crossover;
struct sof_eq_iir_biquad *emphasis;
struct sof_eq_iir_biquad *deemphasis;
struct sof_multiband_drc_config *config = cd->config;
struct multiband_drc_state *state = &cd->state;
uint32_t sample_bytes = get_sample_bytes(cd->source_format);
int i, ch, ret, num_bands;

/* Alignment requirement for the iir_state_df2t structure */
size_t alignment = __alignof__(struct iir_state_df2t);

/* Initialize total_size to accumulate the total memory needed */
size_t total_size = 0;

/* Declare sizes for emphasis, crossover delay, and per channel calculation */
size_t emp_deemp_size, crossover_delay_size, per_channel_size;

if (!config) {
comp_err(dev, "multiband_drc_init_coef(), no config is set");
return -EINVAL;
Expand All @@ -118,8 +141,8 @@ static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, u
return -EINVAL;
}
if (config->num_bands > SOF_MULTIBAND_DRC_MAX_BANDS) {
comp_err(dev, "multiband_drc_init_coef(), invalid bands count(%i)",
config->num_bands);
comp_err(dev,
Copy link
Contributor

@cujomalainey cujomalainey Aug 27, 2024

Choose a reason for hiding this comment

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

one commit, for one change please. See the scale of my commits here as a reference. https://github.com/thesofproject/sof/pull/9383/commits

"multiband_drc_init_coef(), invalid bands count(%i)", config->num_bands);
return -EINVAL;
}

Expand All @@ -134,12 +157,50 @@ static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, u
return -EINVAL;
}

/* Calculate sizes for various components */
emp_deemp_size = sizeof(struct iir_state_df2t) * SOF_EMP_DEEMP_BIQUADS;
crossover_delay_size = sizeof(uint64_t) * CROSSOVER_NUM_DELAYS_LR4;
per_channel_size = emp_deemp_size * 3 + crossover_delay_size * 3;

/* Calculate aligned total size */
total_size = (per_channel_size + alignment - 1) * nch;
total_size += sizeof(struct drc_state) * num_bands * nch;

/* Allocate base memory */
uint8_t *base_addr = (uint8_t *)rballoc(0, SOF_MEM_CAPS_RAM, total_size);
Copy link
Contributor

@cujomalainey cujomalainey Aug 28, 2024

Choose a reason for hiding this comment

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

do you need to make sure this is aligned?


if (!base_addr) {
comp_err(dev, "multiband_drc_init_coef(), memory allocation failed for size %zu",
total_size);
return -ENOMEM;
}

for (ch = 0; ch < nch; ++ch) {
size_t emp_offset = ch * per_channel_size;
size_t crossover_offset = emp_offset + emp_deemp_size;
size_t deemp_offset = crossover_offset + crossover_delay_size;

/* Align the emp_offset to the required alignment boundary */
emp_offset = ((uintptr_t)(base_addr + emp_offset) + alignment - 1) &
Copy link
Contributor

@cujomalainey cujomalainey Aug 28, 2024

Choose a reason for hiding this comment

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

use ALIGN_UP

~(alignment - 1);
/* Align the crossover_offset to the required alignment boundary */
crossover_offset = ((uintptr_t)(base_addr + crossover_offset) + alignment - 1) &
~(alignment - 1);
/* Align the deemp_offset to the required alignment boundary */
deemp_offset = ((uintptr_t)(base_addr + deemp_offset) + alignment - 1) &
~(alignment - 1);

/* Assign the aligned pointers for the current channel */
state->emphasis[ch] = *(struct iir_state_df2t *)(base_addr + emp_offset);
state->crossover[ch] = *(struct crossover_state *)(base_addr + crossover_offset);
state->deemphasis[ch] = *(struct iir_state_df2t *)(base_addr + deemp_offset);
}

/* Crossover: collect the coef array and assign it to every channel */
crossover = config->crossover_coef;
struct sof_eq_iir_biquad *crossover = config->crossover_coef;

for (ch = 0; ch < nch; ch++) {
ret = crossover_init_coef_ch(crossover, &state->crossover[ch],
config->num_bands);
/* Free all previously allocated blocks in case of an error */
ret = crossover_init_coef_ch(crossover, &state->crossover[ch], config->num_bands);
if (ret < 0) {
comp_err(dev,
"multiband_drc_init_coef(), could not assign coeffs to ch %d", ch);
Expand All @@ -150,12 +211,12 @@ static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, u
comp_info(dev, "multiband_drc_init_coef(), initializing emphasis_eq");

/* Emphasis: collect the coef array and assign it to every channel */
emphasis = config->emp_coef;
struct sof_eq_iir_biquad *emphasis = config->emp_coef;

for (ch = 0; ch < nch; ch++) {
ret = multiband_drc_eq_init_coef_ch(emphasis, &state->emphasis[ch]);
/* Free all previously allocated blocks in case of an error */
Copy link
Collaborator

@singalsu singalsu Aug 14, 2024

Choose a reason for hiding this comment

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

I'd not delete so eagerly comments by original author. Please do the functional changes and try to keep original comments. Add another cosmetic commit to improve and clean up the comments if you feel so.

if (ret < 0) {
comp_err(dev, "multiband_drc_init_coef(), could not assign coeffs to ch %d",
comp_err(dev, "multiband_drc_init_coef(), could not assign emphasis coeffs to ch %d",
ch);
goto err;
}
Expand All @@ -164,12 +225,12 @@ static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, u
comp_info(dev, "multiband_drc_init_coef(), initializing deemphasis_eq");

/* Deemphasis: collect the coef array and assign it to every channel */
deemphasis = config->deemp_coef;
struct sof_eq_iir_biquad *deemphasis = config->deemp_coef;

for (ch = 0; ch < nch; ch++) {
ret = multiband_drc_eq_init_coef_ch(deemphasis, &state->deemphasis[ch]);
/* Free all previously allocated blocks in case of an error */
if (ret < 0) {
comp_err(dev, "multiband_drc_init_coef(), could not assign coeffs to ch %d",
comp_err(dev, "multiband_drc_init_coef(), could not assign deemphasis coeffs to ch %d",
ch);
goto err;
}
Expand All @@ -182,14 +243,14 @@ static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, u
ret = drc_init_pre_delay_buffers(&state->drc[i], (size_t)sample_bytes, (int)nch);
if (ret < 0) {
comp_err(dev,
"multiband_drc_init_coef(), could not init pre delay buffers");
"multiband_drc_init_coef(), could not init pre-delay buffers");
goto err;
}

ret = drc_set_pre_delay_time(&state->drc[i],
cd->config->drc_coef[i].pre_delay_time, rate);
ret = drc_set_pre_delay_time(&state->drc[i], cd->config->drc_coef[i].pre_delay_time,
Copy link
Contributor

@cujomalainey cujomalainey Aug 28, 2024

Choose a reason for hiding this comment

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

this is all formatting and should be done in a different commit or should be dropped

rate);
if (ret < 0) {
comp_err(dev, "multiband_drc_init_coef(), could not set pre delay time");
comp_err(dev, "multiband_drc_init_coef(), could not set pre-delay time");
goto err;
}
}
Expand All @@ -198,6 +259,7 @@ static int multiband_drc_init_coef(struct processing_module *mod, int16_t nch, u

err:
multiband_drc_reset_state(state);
rfree(base_addr); /* Free allocated memory in case of error */
return ret;
}

Expand All @@ -216,6 +278,21 @@ static int multiband_drc_setup(struct processing_module *mod, int16_t channels,
* End of Multiband DRC setup code. Next the standard component methods.
*/

/**
* @brief Initialize Multiband Dynamic Range Control (DRC) component.
*
Copy link
Contributor

@cujomalainey cujomalainey Aug 28, 2024

Choose a reason for hiding this comment

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

documentation is its own commit, do not mix this with functional changes.

* Allocates and initializes memory for the multiband DRC component data, including
* state structure, coefficient blocks, and module interface.
*
* The function checks and ensures that the provided configuration blob size is
* within expected limits. If successful, the component state is reset and multiband
* DRC processing is enabled by default.
*
* @param[in] mod Pointer to the processing module to be initialized.
*
* @return 0 on success, -EINVAL if configuration blob size is too large,
* -ENOMEM if memory allocation fails for component data.
*/
static int multiband_drc_init(struct processing_module *mod)
{
struct module_data *md = &mod->priv;
Expand Down Expand Up @@ -243,12 +320,6 @@ static int multiband_drc_init(struct processing_module *mod)
md->private = cd;
cd->multiband_drc_func = NULL;
cd->crossover_split = NULL;
/* Initialize to enabled is a workaround for IPC4 kernel version 6.6 and
* before where the processing is never enabled via switch control. New
* kernel sends the IPC4 switch control and sets this to desired state
* before prepare.
*/
multiband_drc_process_enable(&cd->process_enabled);

/* Handler for configuration data */
cd->model_handler = comp_data_blob_handler_new(dev);
Expand All @@ -266,6 +337,12 @@ static int multiband_drc_init(struct processing_module *mod)
}
multiband_drc_reset_state(&cd->state);

/* Initialize to enabled as a workaround for IPC4 kernel version 6.6
* and before where the processing is never enabled via switch
* control. The new kernel sends the IPC4 switch control and sets
* this to the desired state before prepare.
*/
multiband_drc_process_enable(&cd->process_enabled);
Copy link
Contributor

@cujomalainey cujomalainey Aug 28, 2024

Choose a reason for hiding this comment

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

why was this moved?

Also your commit message is out of date, please update it

return 0;

cd_fail:
Expand All @@ -274,15 +351,34 @@ static int multiband_drc_init(struct processing_module *mod)
return ret;
}

/**
* @brief Free resources allocated by Multiband DRC processing component.
*
* This function releases all memory and resources associated with the
* multiband DRC component's operation. This includes dynamically allocated
* filter state instances as well as freeing up the model handler.
*
* @param[in] mod Pointer to the processing module to be freed.
*
* @return 0 indicating success.
*/
static int multiband_drc_free(struct processing_module *mod)
{
struct multiband_drc_comp_data *cd = module_get_private_data(mod);

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

comp_data_blob_handler_free(cd->model_handler);
if (cd) {
/* Freeing other resources as part of the component data */
comp_data_blob_handler_free(cd->model_handler);

/* Free the main component data structure */
rfree(cd);

/* Clear the private module data pointer */
module_set_private_data(mod, NULL);
}

rfree(cd);
return 0;
}

Expand Down