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

ShriramShastry · 2024-06-05T04:29:21Z

This check-in enhances memory management in the Multiband Dynamic
Range Control (MDRC) component.

Key Changes:

Streamlined memory allocation and initialization of crossover,
emphasis, and de-emphasis coefficients directly in
multiband_drc_init_coef.
Updated multiband_drc_init to eliminate the allocation of any
obsolete blocks.
Adjusted multiband_drc_free to no longer check and free any
now-nonexistent blocks.
Simplified overall memory management by removing unnecessary
layers of indirection.

Performance Improvements:

Reduced memory allocation overhead.
Enhanced data locality, improving cache efficiency.
Mitigated heap fragmentation, thereby reducing the chances of memory
leaks.

lyakh · 2024-06-05T07:14:17Z

src/audio/multiband_drc/multiband_drc.c

+
+	struct sof_eq_iir_biquad *coefficients_block =
+	    rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
+		    total_coefficients_size);


now this seems to be allocated twice: first time inside multiband_drc_init() and a second time here.

Thank you for your feedback regarding the memory allocation within the multiband DRC module

The allocation of memory for coefficients_block in multiband_drc_init_coef() is separate from the allocation of multiband_drc_comp_data in multiband_drc_init(). The latter is the component data for the multiband
DRC module, and it is allocated only once during module initialization.

I 've cross checked again.

The multiband_drc_comp_data allocation is for storing the component's state and is done once upon initialization (multiband_drc_init()).
The coefficients_block allocation is specifically for filter coefficients and occurs later during the module's preparation (multiband_drc_init_coef()).
The allocation for coefficients_block is not done in multiband_drc_init() and hence there is no duplication.
It looks like all allocations and initializations are following the correct sequence and avoiding double allocation for the same data structures. If there are any further concerns or points that require clarification, please let me know.

@ShriramShastry Maybe I didn't explain it well. See line 132 below that you're removing

sof/src/audio/multiband_drc/multiband_drc.c

Line 132 in fec9e99

crossover = config->crossover_coef;

- that's where previously crossover was coming from. Now you added an allocation for it. But I don't see where you removed that original data block? So the original array seems to still be there and you add a new one.

Thanks ! I'II make the adjustments.

Done ! Please have a look.

cujomalainey · 2024-06-05T17:07:33Z

src/audio/multiband_drc/multiband_drc.c

+	 * deemphasis is situated after the emphasis coefficients.
+	 * This ensures all filter coefficients are stored contiguously.
+	 */
+	crossover = coefficients_block;


I would prefer a modification to the datatypes rather than just packing them into a single buffer and crossing fingers no one screws up the math in the future

Thank you for the review and comments. The changes to the allocation and pointer setup are intended to enhance memory efficiency and cache performance for the MDRC component.

Here’s a summary of the changes and the rationale behind them:

Memory Allocation Consolidation: By allocating a single block of memory for crossover, emphasis, and deemphasis filter coefficients, we avoid separate heap allocations which can incur additional overhead and fragmentation.

size_t total_coefficients_size = sizeof(struct sof_eq_iir_biquad) * num_bands * nch * 3; struct sof_eq_iir_biquad *coefficients_block = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, total_coefficients_size); if (!coefficients_block) { comp_err(dev, "multiband_drc_init_coef(), failed to allocate memory for coefficients"); return -ENOMEM; }

Pointer Assignment: After the allocation, pointers for crossover, emphasis, and deemphasis filters are pointed to the appropriate locations within the single allocated block.

crossover = coefficients_block; emphasis = coefficients_block + num_bands * nch; deemphasis = emphasis + num_bands * nch;

This design assures that the data used concurrently during processing is also kept in memory, which will assist with cache usage when processing audio data.

Error Handling: A clear error handling path (goto err;) ensures that the allocated memory block is freed in the event of an error occurring after the allocation, preventing memory leaks.

if (ret < 0) { comp_err(dev, "multiband_drc_init_coef(), could not assign coeffs to ch %d", ch); rfree(coefficients_block); goto err; }

The check-in ensure both accuracy and safety, the pointer arithmetic is based on the size, count, and channel configurations of the coefficients.

I understand the motivation, but the code you have written here (by nature) is much more fragile than if you simply made a toplevel struct which contained everything and just allocated that directly.

Pointer math is hard enough to debug, pointer math on an embedded device is much worse. Lets not add any more than we absolutely should.

Given that I'm organizing coefficients in contiguous memory, should multiband_drc_init_coef() be designed to handle separate coefficients instantiation for each channel and band?

If our architecture allows for different filters per channel/band, this could mean dynamically revising the processing loop in multiband_drc_init_coef() to fetch and apply the correct filter coefficients for each specific band.

Could you shed some light on whether unique filters per channel/band are an aspect of our system's design? If so, are there any particular considerations or adjustments to the coefficient fetch and allocation logic within multiband_drc_init_coef() that I should be aware of to implement this correctly?

/** *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; * * if (!config) { * comp_err(dev, "multiband_drc_init_coef(), no config is set"); * return -EINVAL; * } * * num_bands = config->num_bands; * * // Sanity checks * if (nch > PLATFORM_MAX_CHANNELS) { * comp_err(dev, * "multiband_drc_init_coef(), invalid channels count(%i)", nch); * 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); * return -EINVAL; * } * * comp_info(dev, "multiband_drc_init_coef(), initializing %i-way crossover", * config->num_bands); * * // Crossover: collect the coef array and assign it to every channel * 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 * if (ret < 0) { * comp_err(dev, * "multiband_drc_init_coef(), could not assign coeffs to ch %d", ch); * goto err; * } * } * * 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; * 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 * if (ret < 0) { * comp_err(dev, "multiband_drc_init_coef(), could not assign coeffs to ch %d", * ch); * goto err; * } * } * * 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; * 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", * ch); * goto err; * } * } * * // Allocate all DRC pre-delay buffers and set delay time with band number * for (i = 0; i < num_bands; i++) { * comp_info(dev, "multiband_drc_init_coef(), initializing drc band %d", i); * * 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"); * goto err; * } * * ret = drc_set_pre_delay_time(&state->drc[i], * cd->config->drc_coef[i].pre_delay_time, rate); * if (ret < 0) { * comp_err(dev, "multiband_drc_init_coef(), could not set pre delay time"); * goto err; * } * } * * return 0; * *err: * multiband_drc_reset_state(state); * return ret; *} */ static int multiband_drc_setup(struct processing_module *mod, int16_t channels, uint32_t rate) { struct multiband_drc_comp_data *cd = module_get_private_data(mod); int ret; /* Reset any previous state */ multiband_drc_reset_state(&cd->state); /* Setup Crossover, Emphasis EQ, Deemphasis EQ, and DRC */ ret = multiband_drc_init_coef(mod, channels, rate); }

Currently, the coefficients for each of the emphasis/de-emphasis filters and crossovers per channel are initialised, but it appears that the multiband DRC assumes a configured number of filters applied uniformly across channels; however, if individual adjustments/adapt to filters per channel/band are required, the code should fetch and set up the coefficients accordingly within the multiband_drc_setup and related functions.

Your expertise in this area will help to clarify the best course of action and ensure that our implementation remains efficient and in line with our system's architectural goals.

ShriramShastry

Please let me know if you have any specific concerns or suggestions for further improvements. Your feedback is extremely helpful, and I am willing to make any necessary changes to this patch..

Thank you for review.

ShriramShastry · 2024-06-06T04:38:15Z

src/audio/multiband_drc/multiband_drc.c

+	 * deemphasis is situated after the emphasis coefficients.
+	 * This ensures all filter coefficients are stored contiguously.
+	 */
+	crossover = coefficients_block;


Thank you for the review and comments. The changes to the allocation and pointer setup are intended to enhance memory efficiency and cache performance for the MDRC component.

Here’s a summary of the changes and the rationale behind them:

Memory Allocation Consolidation: By allocating a single block of memory for crossover, emphasis, and deemphasis filter coefficients, we avoid separate heap allocations which can incur additional overhead and fragmentation.

size_t total_coefficients_size = sizeof(struct sof_eq_iir_biquad) * num_bands * nch * 3; struct sof_eq_iir_biquad *coefficients_block = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, total_coefficients_size); if (!coefficients_block) { comp_err(dev, "multiband_drc_init_coef(), failed to allocate memory for coefficients"); return -ENOMEM; }

Pointer Assignment: After the allocation, pointers for crossover, emphasis, and deemphasis filters are pointed to the appropriate locations within the single allocated block.

crossover = coefficients_block; emphasis = coefficients_block + num_bands * nch; deemphasis = emphasis + num_bands * nch;

This design assures that the data used concurrently during processing is also kept in memory, which will assist with cache usage when processing audio data.

Error Handling: A clear error handling path (goto err;) ensures that the allocated memory block is freed in the event of an error occurring after the allocation, preventing memory leaks.

if (ret < 0) { comp_err(dev, "multiband_drc_init_coef(), could not assign coeffs to ch %d", ch); rfree(coefficients_block); goto err; }

The check-in ensure both accuracy and safety, the pointer arithmetic is based on the size, count, and channel configurations of the coefficients.

ShriramShastry

Thanks for reviewing the code, I 'm in need of your/Google guidance on Coefficient allocation logic for Channel/Band-Specific Filters in multiband_drc_init_coef().

Because the task requires us to properly allocate and assign emphasis/de-emphasis filter coefficients within a shared memory block.

However, I need to know if our architecture requires these filters to be unique per channel/band, which will affect our allocation strategy and coefficient fetch logic.

ShriramShastry · 2024-06-08T16:59:32Z

src/audio/multiband_drc/multiband_drc.c

+	 * deemphasis is situated after the emphasis coefficients.
+	 * This ensures all filter coefficients are stored contiguously.
+	 */
+	crossover = coefficients_block;


Given that I'm organizing coefficients in contiguous memory, should multiband_drc_init_coef() be designed to handle separate coefficients instantiation for each channel and band?

If our architecture allows for different filters per channel/band, this could mean dynamically revising the processing loop in multiband_drc_init_coef() to fetch and apply the correct filter coefficients for each specific band.

Could you shed some light on whether unique filters per channel/band are an aspect of our system's design? If so, are there any particular considerations or adjustments to the coefficient fetch and allocation logic within multiband_drc_init_coef() that I should be aware of to implement this correctly?

/** *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; * * if (!config) { * comp_err(dev, "multiband_drc_init_coef(), no config is set"); * return -EINVAL; * } * * num_bands = config->num_bands; * * // Sanity checks * if (nch > PLATFORM_MAX_CHANNELS) { * comp_err(dev, * "multiband_drc_init_coef(), invalid channels count(%i)", nch); * 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); * return -EINVAL; * } * * comp_info(dev, "multiband_drc_init_coef(), initializing %i-way crossover", * config->num_bands); * * // Crossover: collect the coef array and assign it to every channel * 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 * if (ret < 0) { * comp_err(dev, * "multiband_drc_init_coef(), could not assign coeffs to ch %d", ch); * goto err; * } * } * * 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; * 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 * if (ret < 0) { * comp_err(dev, "multiband_drc_init_coef(), could not assign coeffs to ch %d", * ch); * goto err; * } * } * * 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; * 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", * ch); * goto err; * } * } * * // Allocate all DRC pre-delay buffers and set delay time with band number * for (i = 0; i < num_bands; i++) { * comp_info(dev, "multiband_drc_init_coef(), initializing drc band %d", i); * * 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"); * goto err; * } * * ret = drc_set_pre_delay_time(&state->drc[i], * cd->config->drc_coef[i].pre_delay_time, rate); * if (ret < 0) { * comp_err(dev, "multiband_drc_init_coef(), could not set pre delay time"); * goto err; * } * } * * return 0; * *err: * multiband_drc_reset_state(state); * return ret; *} */ static int multiband_drc_setup(struct processing_module *mod, int16_t channels, uint32_t rate) { struct multiband_drc_comp_data *cd = module_get_private_data(mod); int ret; /* Reset any previous state */ multiband_drc_reset_state(&cd->state); /* Setup Crossover, Emphasis EQ, Deemphasis EQ, and DRC */ ret = multiband_drc_init_coef(mod, channels, rate); }

Currently, the coefficients for each of the emphasis/de-emphasis filters and crossovers per channel are initialised, but it appears that the multiband DRC assumes a configured number of filters applied uniformly across channels; however, if individual adjustments/adapt to filters per channel/band are required, the code should fetch and set up the coefficients accordingly within the multiband_drc_setup and related functions.

Your expertise in this area will help to clarify the best course of action and ensure that our implementation remains efficient and in line with our system's architectural goals.

cujomalainey · 2024-06-10T17:09:30Z

src/audio/multiband_drc/multiband_drc.c

 	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;
+	bool alloc_success = false;


you don't need this, you can just check if coefficients_block == NULL in the error path.

cujomalainey · 2024-06-10T17:15:00Z

src/audio/multiband_drc/multiband_drc.h

+	struct sof_eq_iir_biquad crossover[SOF_MULTIBAND_DRC_MAX_BANDS * PLATFORM_MAX_CHANNELS];
+	struct sof_eq_iir_biquad emphasis[SOF_MULTIBAND_DRC_MAX_BANDS * PLATFORM_MAX_CHANNELS];
+	struct sof_eq_iir_biquad deemphasis[SOF_MULTIBAND_DRC_MAX_BANDS * PLATFORM_MAX_CHANNELS];
+} __packed;


Won't packing cause a lot of slow reads/writes because things might not be aligned?

While check-in I was getting the error if I do __attribute__((packed)); instead of "} __packed; so I drop the attribute.

"

No codespell typos will be found - file '/usr/share/codespell/dictionary.txt': No such file or directory WARNING: Prefer __packed over __attribute__((packed)) #10: FILE: src/audio/multiband_drc/multiband_drc.h:28: +} __attribute__((packed)); total: 0 errors, 1 warnings, 8 lines checked

Understood. We're using __packed to save memory, but could you confirm if the potential misalignment outweighs the memory gain in our context? And whether our target platform can handle unaligned accesses efficiently.

@ShriramShastry we mainly using packing for any data shared with host, but in this case isn't this data provate to FW only ? If so, we can drop the packed and allow compiler to best organise the data.

@ShriramShastry please read this

The odds here are you saving at most 10s of bytes and paying back way worse in code size and access times

agree, packed isn't needed. It should only be used where data format is important and must be preserved, e.g. when sending data over networks, or between the host and the DSP

Done, removed packed

cujomalainey · 2024-06-10T17:15:59Z

src/audio/multiband_drc/multiband_drc.c

+	struct multiband_drc_comp_data *cd = rzalloc(SOF_MEM_ZONE_RUNTIME, 0,
+						     SOF_MEM_CAPS_RAM, sizeof(*cd));
+	if (!cd) {
+		comp_err(dev, "multiband_drc_init(), allocation for multiband_drc_comp_data failed");


Could you please clarify if the removal of the NULL check after rzalloc is due to guaranteed error handling within the allocator, or are we following a specific coding standard that omits such checks?

the change is unneeded (i.e. the trace), the check is still very much needed

cujomalainey · 2024-06-10T17:17:01Z

src/audio/multiband_drc/multiband_drc.c

+
+	/* Allocation for coefficients_block */
+	if (!cd->coefficients_block) {
+		cd->coefficients_block = rballoc(0, SOF_MEM_CAPS_RAM,


just allocate this upfront directly into cd so you don't have to constantly check this.

Could you provide more detail on how to allocate coefficients_block directly with cd upfront, given that we typically need to check and allocate each separately in standard C?

Just do it at the module creation step so you don't have to check if the memory exists when you are parsing params

ShriramShastry

The allocation of coefficients_block is currently conditional on whether it is NULL. Would you prefer that this allocation be performed unconditionally at the time of cd allocation, ensuring that coefficients_block is never NULL and eliminating all subsequent NULL checks on it?

lgirdwood · 2024-06-12T14:47:21Z

The allocation of coefficients_block is currently conditional on whether it is NULL. Would you prefer that this allocation be performed unconditionally at the time of cd allocation, ensuring that coefficients_block is never NULL and eliminating all subsequent NULL checks on it?

I would allocate it when needed if it relies on IPC configuration data for allocation size, if its always constant size you could allocate per instance when pipeline is created.

cujomalainey · 2024-06-13T02:42:38Z

The allocation of coefficients_block is currently conditional on whether it is NULL. Would you prefer that this allocation be performed unconditionally at the time of cd allocation, ensuring that coefficients_block is never NULL and eliminating all subsequent NULL checks on it?

I would allocate it when needed if it relies on IPC configuration data for allocation size, if its always constant size you could allocate per instance when pipeline is created.

+1, I think the only thing that would possibly vary is the crossover bands and that is fairly small

cujomalainey · 2024-06-17T19:43:15Z

src/audio/multiband_drc/multiband_drc.c

+	struct multiband_drc_comp_data *cd = rzalloc(SOF_MEM_ZONE_RUNTIME, 0,
+						     SOF_MEM_CAPS_RAM, sizeof(*cd));
+	if (!cd) {
+		comp_err(dev, "multiband_drc_init(), allocation for multiband_drc_comp_data failed");


the change is unneeded (i.e. the trace), the check is still very much needed

cujomalainey · 2024-06-17T19:53:07Z

src/audio/multiband_drc/multiband_drc.h

+	struct sof_eq_iir_biquad crossover[SOF_MULTIBAND_DRC_MAX_BANDS * PLATFORM_MAX_CHANNELS];
+	struct sof_eq_iir_biquad emphasis[SOF_MULTIBAND_DRC_MAX_BANDS * PLATFORM_MAX_CHANNELS];
+	struct sof_eq_iir_biquad deemphasis[SOF_MULTIBAND_DRC_MAX_BANDS * PLATFORM_MAX_CHANNELS];
+};


the more I read this code the more I think this change is incorrect. Look at the struct directly below. This is a copy of that struct which is already directly embedded in the component data. How does this benefit anything when the data was already allocated in place in a single block?

Thanks ! Please take a look at the latest changes. The patch saves 54 MCPS in TGL i.e. from 188 to 134

singalsu · 2024-06-18T09:06:48Z

This PR version has an issue with TGL HiFi3 build. If I set with topology sof-hda-benchmark-drc_multiband32.tplg the control amixer cset name='Analog Playback MULTIBAND_DRC enable' on the playback becomes silent. Sound can be heard with setting amixer cset name='Analog Playback MULTIBAND_DRC enable' off. The current DRC versin does not follow the switch control in runtime, the control needs to be applied when streaming is stopped (or stop the stream to get impact of previously applied control).

I have no idea if MTL HiFi4 has the issue, I don't have a suitable device to check own FW builds.

cujomalainey · 2024-06-18T18:02:11Z

This PR version has an issue with TGL HiFi3 build. If I set with topology sof-hda-benchmark-drc_multiband32.tplg the control amixer cset name='Analog Playback MULTIBAND_DRC enable' on the playback becomes silent. Sound can be heard with setting amixer cset name='Analog Playback MULTIBAND_DRC enable' off. The current DRC versin does not follow the switch control in runtime, the control needs to be applied when streaming is stopped (or stop the stream to get impact of previously applied control).

I have no idea if MTL HiFi4 has the issue, I don't have a suitable device to check own FW builds.

@singalsu no that is by design for our code that the on/off switch is applied while the stream is open.

lyakh · 2024-08-12T09:58:57Z

Thank you very much for the reviews. As for specific improvements, I'm still not sure whether this actually improves anything It would be helpful to understand what specific concerns exist; the patch reduces TGL by 54 MCPS, from 188 to 134. ~28% savings.

that's the information I was looking for, yes. Maybe it was already provided above, sorry, difficult to find, it has become rather long. So, you're saying that just by rearranging initialisation code to regroup buffers improves run-time (i.e. during copying) performance on TGL by 28%?.. That's the complete .copy() function? Amazing.

I'm glad to clarify the differences that contribute to the performance improvements:

Memory Allocation Optimization:

Original: Separate loops for allocating crossover, emphasis, and de-emphasis coefficients per channel. Modified: A single loop handles allocation and initialization for all coefficients per channel, enhancing data locality and cache efficiency. This is achieved by grouping these allocations by channel (crossover, emphasis, deemphasis), which is more cache-friendly during runtime processing.

Initialization Enhancement:

Original: Filter coefficients were initialized in separate routines. Modified: Consolidated initialization routines (multiband_drc_init_coef) streamline the setup process, reducing computational overhead and simplifying the code structure.

Improved Clean-up Procedures:

Original: Clean-up procedures were less detailed. Modified: Detailed clean-up steps (multiband_drc_free) ensure thorough resource deallocation, preventing memory leaks and enhancing long-term performance stability.

Error Handling:

Original: Error handling was less comprehensive during initialization. Modified: Rigorous error handling during initialization to ensure stability and prevent resource leaks in case of failures.

These changes collectively lead to a 28% reduction in MCPS during the .copy() function execution on TGL platforms by improving cache performance and reducing initialization overhead.

Sorry, I don't follow which of these improvements could bring you a 28% run-time performance improvement. The fact that buffers are now grouped by channel? That might improve runtime a bit, but very unlikely 28%.

Re: improved error handling - I only see a check for cd which I'm not sure is needed, don't think that function would be called if cd allocation failed. If anything, this should be split into multiple commits - one commit per improvement, then you can check which specific commit / change makes the claimed performance difference, and would be good to tell us how exactly you measure that difference - between which points.

src/audio/multiband_drc/multiband_drc.c

cujomalainey · 2024-08-12T20:35:23Z

src/audio/multiband_drc/multiband_drc.c

 	if (!config) {
 		comp_err(dev, "multiband_drc_init_coef(), no config is set");
 		return -EINVAL;
 	}

 	num_bands = config->num_bands;

-	/* Sanity checks */


ping on revertion

singalsu

There's merge conflict to fix.

I tried this with UPX i11, topology sof-hda-efx-mbdrc-generic-4ch.tplg. The code runs if processing is not enabled.

If I enable processing with these commands there is a FW crash:

sof-ctl -c name='Post Mixer Analog Playback MBDRC bytes' -s ctl4/multiband_drc/default.txt
amixer cset name='Post Mixer Analog Playback MBDRC switch' on

The the kernel log shows this:

elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ------------[ DSP dump start ]------------
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: DSP panic!
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: fw_state: SOF_FW_BOOT_COMPLETE (7)
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: 0x00000005: module: ROM, state: FW_ENTERED, running
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: FW is built with XCC toolchain
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: error: DSP Firmware Oops
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: error: Exception Cause: InstrPIFDataErrorCause, PIF data error during instruction fetch
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: EXCCAUSE 0x0000000c EXCVADDR 0x00000000 PS       0x00060f20 SAR     0x00000003
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: EPC1     0x00000000 EPC2     0x00000000 EPC3     0x00000000 EPC4    0x00000000
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: EPC5     0x00000000 EPC6     0x00000000 EPC7     0x00000000 DEPC    0x00000000
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: EPS2     0x00000000 EPS3     0x00000000 EPS4     0x00000000 EPS5    0x00000000
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: EPS6     0x00000000 EPS7     0x00000000 INTENABL 0x00000000 INTERRU 0x00000000
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: stack dump from 0x00000000
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: AR registers:
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: 0x0: be044cd2 be0a0b50 be0bbd40 00000000
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: 0x10: be0a0be0 be0a0c00 be0bbdc0 be0a0b60
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: 0x20: be0444e3 be0a0b30 00000000 be0a0b50
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: 0x30: be0444e3 be0a0b30 00000000 be0a0b50
elo 14 11:38:19 ekstremisti kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ------------[ DSP dump end ]------------

The first AR register address seems to be in multiband_drc_process_emp_crossover() at code lines:

			band_buf_drc_src = buf_drc_src;
			band_buf_drc_sink = buf_drc_sink;
			for (band = 0; band < nband; ++band) {
be044cd2:	5c2192               	l32i	a9, a1, 0x170
be044cd5:	572162               	l32i	a6, a1, 0x15c
be044cd8:	070c                	movi.n	a7, 0
be044cda:	3419a6               	blti	a9, 1, be044d12 <multiband_drc_s32_default+0x142>
be044cdd:	50c122               	addi	a2, a1, 80
be044ce0:	7fc152               	addi	a5, a1, 127
be044ce3:	51c552               	addi	a5, a5, 81

singalsu · 2024-08-14T08:47:55Z

src/audio/multiband_drc/multiband_drc.c


-	/* Crossover: collect the coef array and assign it to every channel */
+	/* Initialize constants for shared coefficients */


The original comment is in my opinion a lot more informative.

singalsu · 2024-08-14T08:51:31Z

src/audio/multiband_drc/multiband_drc.c

 		ret = multiband_drc_eq_init_coef_ch(emphasis, &state->emphasis[ch]);
-		/* Free all previously allocated blocks in case of an error */


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.

lgirdwood

@ShriramShastry can you make this 3 commits, one for each numbered topic in the commit message. This will make it faster to review and merge. Thanks !

ShriramShastry

Thanks. I have made new changes.
Pending work. CI Testbench still needs attention. I'II work further.
Can you please take a look.

src/audio/multiband_drc/multiband_drc.c

cujomalainey · 2024-08-22T21:03:28Z

src/audio/multiband_drc/multiband_drc.c

 	}
 	multiband_drc_reset_state(&cd->state);

+	/* Initialize to enabled is a workaround for IPC4 kernel version 6.6 and


why is this moved?

cujomalainey · 2024-08-22T21:04:08Z

src/audio/multiband_drc/multiband_drc.c

 	return 0;

-cd_fail:
+cd_model_fail:


again this isn't needed. cd is initialized to all NULL and the function is smart enough to check to see if NULL is passed in. So this split logic just adds more paths for no reason.

cujomalainey · 2024-08-22T21:19:26Z

src/audio/multiband_drc/multiband_drc.c

+
+		total_size += (crossover_delay_size % __alignof__(uint64_t) != 0) ?
+			__alignof__(uint64_t) -
+			(crossover_delay_size % __alignof__(uint64_t)) : 0;


assuming an initial aligned address, won't the total size per channel be the same? Then you can just multiple instead of looping.

cujomalainey · 2024-08-22T21:24:38Z

src/audio/multiband_drc/multiband_drc.c

+	void *ptrs[3 * nch * 3 + num_bands];
+	size_t sizes[3 * nch * 3 + num_bands];
+
+	for (ch = 0; ch < nch; ++ch) {


rather than doing this convoluted offset loop, why not simply store your offsets for each field, then do base_addr + offset and store it directly to the field in the struct.

cujomalainey · 2024-08-22T21:25:50Z

src/audio/multiband_drc/multiband_drc.c

+	void *base_addr = allocate_contiguous_memory(dev, total_size,
+						     ptrs, sizes, 3 * nch + num_bands);
+
+	if (!base_addr)


I don't see any modifications to the free logic in this commit which has me really concerned this is creating a massive double free

ShriramShastry

Thank you for reviewing the code. I have update the code. Please take a look.

src/audio/multiband_drc/multiband_drc.c

- Enhance memory allocation by using contiguous memory blocks. - Ensure proper memory alignment using __alignof__ for sub-structures. - Implement alignment checks with uint8_t* for precise byte-wise calcs. - Add detailed comments for improved readability and maintainability. - Ensure each sub-block starts at a properly aligned address to minimize performance issues due to misaligned memory accesses. - Streamline size calculations for emphasis, crossover, and DRC delay blocks. Advantages: - Optimizes memory management and ensures efficient access to structures. - Provides robust performance by aligning memory allocations correctly. - Prevents potential crashes or bugs caused by improper memory handling. - Improves code clarity and structure, making future maintenance easier. These changes result in a more efficient and robust initialization process within the multiband_drc_init_coef function. Signed-off-by: Shriram Shastry <[email protected]>

- Improved documentation for the multiband_drc_init function, specifying its purpose, parameters, and initialization steps. - Clearly defined all initialization steps, including memory allocations and configuration checks. - Simplified error handling with a single cleanup path, relying on existing functions to handle NULL pointers. - Ensured proper initialization sequence for all components. Signed-off-by: Shriram Shastry <[email protected]>

- Added comprehensive documentation for multiband_drc_free function, detailing its purpose, parameters, and the cleanup process. - Introduced validation to check if component data (`cd`) is not NULL before freeing associated resources. - Ensured all allocated resources are properly freed, and nullified the private module data pointer upon cleanup. - Provided clear log messages indicating the start and successful completion of the free operation. - Included necessary comments to clarify code intent and improve maintainability. Signed-off-by: Shriram Shastry <[email protected]>

singalsu

Git main is OK. Can't check the performance impact of this PR.

With this PR the FW crashes in playback to device hw:0,0 with default settings for topology sof-hda-efx-mbdrc-generic-4ch.tplg. Crash happens with both on and off setting of control name='Post Mixer Analog Playback MBDRC switch'.

singalsu · 2024-08-27T15:15:14Z

You can also see a valgrind reported error with scripts/host-testbench.sh run. Please try that yourself.

cujomalainey · 2024-08-27T19:15:03Z

src/audio/multiband_drc/multiband_drc.c

@@ -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,


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

cujomalainey · 2024-08-28T18:15:21Z

src/audio/multiband_drc/multiband_drc.c

+ * \param[in] nch Number of channels to process.
+ * \param[in] rate Sample rate of the audio stream.
+ *
+ * \return 0 on success, error code otherwise.


this should be a separate commit

cujomalainey · 2024-08-28T18:19:57Z

src/audio/multiband_drc/multiband_drc.c

+		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) &


use ALIGN_UP

cujomalainey · 2024-08-28T18:21:55Z

src/audio/multiband_drc/multiband_drc.c

+	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);


do you need to make sure this is aligned?

cujomalainey · 2024-08-28T18:22:34Z

src/audio/multiband_drc/multiband_drc.c

 			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,


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

cujomalainey · 2024-08-28T18:23:21Z

src/audio/multiband_drc/multiband_drc.c

@@ -278,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.
+ *


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

cujomalainey · 2024-08-28T18:24:33Z

src/audio/multiband_drc/multiband_drc.c

+	 * 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);


why was this moved?

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

src/audio/multiband_drc/multiband_drc.c

kv2019i · 2024-09-06T11:36:00Z

Release reminder - one week to v2.11-rc1.

kv2019i · 2024-09-13T07:21:54Z

FYI @ShriramShastry , moving to v2.12

kv2019i · 2024-12-13T12:01:09Z

Feature cutoff for v2.12, moving this to v2.13.

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch 2 times, most recently from d6d6854 to 012e192 Compare June 5, 2024 04:33

ShriramShastry requested review from lyakh, lgirdwood and singalsu June 5, 2024 05:02

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch 2 times, most recently from a8a3a75 to aee6da8 Compare June 5, 2024 05:44

lyakh requested changes Jun 5, 2024

View reviewed changes

ShriramShastry marked this pull request as ready for review June 5, 2024 14:18

ShriramShastry requested a review from a team as a code owner June 5, 2024 14:18

cujomalainey suggested changes Jun 5, 2024

View reviewed changes

ShriramShastry commented Jun 6, 2024

View reviewed changes

ShriramShastry requested review from cujomalainey and lyakh June 6, 2024 05:20

ShriramShastry commented Jun 8, 2024

View reviewed changes

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch 2 times, most recently from 3b6a9cd to b5919ce Compare June 10, 2024 12:36

cujomalainey suggested changes Jun 10, 2024

View reviewed changes

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch from b5919ce to c86a1f7 Compare June 11, 2024 08:14

ShriramShastry commented Jun 11, 2024

View reviewed changes

ShriramShastry requested a review from cujomalainey June 11, 2024 08:45

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch from c86a1f7 to f770cd2 Compare June 14, 2024 06:33

cujomalainey requested a review from johnylin76 June 17, 2024 19:44

cujomalainey suggested changes Jun 17, 2024

View reviewed changes

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch from f770cd2 to 9ce82c1 Compare June 24, 2024 13:08

ShriramShastry requested a review from cujomalainey June 24, 2024 13:12

cujomalainey suggested changes Aug 12, 2024

View reviewed changes

singalsu requested changes Aug 14, 2024

View reviewed changes

singalsu reviewed Aug 14, 2024

View reviewed changes

lgirdwood reviewed Aug 14, 2024

View reviewed changes

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch 4 times, most recently from fb011df to 828eff1 Compare August 18, 2024 20:59

ShriramShastry commented Aug 19, 2024

View reviewed changes

lgirdwood added this to the v2.11 milestone Aug 21, 2024

cujomalainey suggested changes Aug 22, 2024

View reviewed changes

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch from 828eff1 to 37306fe Compare August 24, 2024 08:06

ShriramShastry commented Aug 24, 2024

View reviewed changes

src/audio/multiband_drc/multiband_drc.c Show resolved Hide resolved

ShriramShastry requested review from cujomalainey and singalsu August 24, 2024 08:13

ShriramShastry added 3 commits August 24, 2024 16:10

ShriramShastry force-pushed the improve_multiband_drc_memory_optimization branch from 37306fe to ddbdb17 Compare August 24, 2024 12:17

singalsu requested changes Aug 27, 2024

View reviewed changes

cujomalainey suggested changes Aug 28, 2024

View reviewed changes

kv2019i modified the milestones: v2.11, v2.12 Sep 13, 2024

kv2019i modified the milestones: v2.12, v2.13 Dec 13, 2024


		/* Crossover: collect the coef array and assign it to every channel */
		/* Initialize constants for shared coefficients */

		ret = multiband_drc_eq_init_coef_ch(emphasis, &state->emphasis[ch]);
		/* Free all previously allocated blocks in case of an error */

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

Are you sure you want to change the base?

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

Conversation

ShriramShastry commented Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

ShriramShastry Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

lgirdwood commented Jun 12, 2024

cujomalainey commented Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

singalsu commented Jun 18, 2024 • edited Loading

cujomalainey commented Jun 18, 2024

lyakh commented Aug 12, 2024

Choose a reason for hiding this comment

singalsu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

singalsu left a comment • edited Loading

Choose a reason for hiding this comment

singalsu commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Sep 6, 2024

kv2019i commented Sep 13, 2024

kv2019i commented Dec 13, 2024

ShriramShastry commented Jun 5, 2024 •

edited

Loading

ShriramShastry Jun 5, 2024 •

edited

Loading

ShriramShastry Jun 11, 2024 •

edited

Loading

ShriramShastry Jun 24, 2024 •

edited

Loading

singalsu commented Jun 18, 2024 •

edited

Loading

singalsu left a comment •

edited

Loading

singalsu left a comment •

edited

Loading