Skip to content

bugfix in DP/Src: some audio formats have incorrectly estimate period #9927

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

Merged
kv2019i merged 2 commits into from
Apr 2, 2025
Merged

bugfix in DP/Src: some audio formats have incorrectly estimate period #9927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73366d9
mod: align DP period down to LL cycle
marcinszkudlinski Mar 26, 2025
a82618b
dp: use 64bit integers for period calculation
marcinszkudlinski Mar 28, 2025
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
25 changes: 20 additions & 5 deletions src/audio/module_adapter/module_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <sof/audio/source_api.h>
#include <sof/audio/audio_buffer.h>
#include <sof/audio/pipeline.h>
#include <sof/schedule/ll_schedule_domain.h>
#include <sof/common.h>
#include <sof/platform.h>
#include <sof/ut.h>
Expand Down Expand Up @@ -138,8 +139,10 @@ static void module_adapter_calculate_dp_period(struct comp_dev *dev)
unsigned int period = UINT32_MAX;

for (int i = 0; i < mod->num_of_sinks; i++) {
/* calculate time required the module to provide OBS data portion - a period */
unsigned int sink_period = 1000000 * sink_get_min_free_space(mod->sinks[i]) /
/* calculate time required the module to provide OBS data portion - a period
* use 64bit integers to avoid overflows
*/
unsigned int sink_period = 1000000ULL * sink_get_min_free_space(mod->sinks[i]) /
(sink_get_frame_bytes(mod->sinks[i]) *
sink_get_rate(mod->sinks[i]));
/* note the minimal period for the module */
Expand Down Expand Up @@ -198,9 +201,21 @@ int module_adapter_prepare(struct comp_dev *dev)
* but events and therefore don't have any deadline for processing
* Second example is a module with variable data rate on output (like MPEG encoder)
*/
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP && !dev->period) {
module_adapter_calculate_dp_period(dev);
comp_info(dev, "DP Module period set to %u", dev->period);
if (mod->dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
/* calculate DP period if a module didn't */
if (!dev->period)
module_adapter_calculate_dp_period(dev);

if (dev->period < LL_TIMER_PERIOD_US) {
comp_err(dev, "DP Module period too short (%u us), must be at least 1LL cycle (%llu us)",
dev->period, LL_TIMER_PERIOD_US);
return -EINVAL;
}

/* align down period to LL cycle time */
dev->period /= LL_TIMER_PERIOD_US;
dev->period *= LL_TIMER_PERIOD_US;
comp_info(dev, "DP Module period set to %u us", dev->period);
}
#endif /* CONFIG_ZEPHYR_DP_SCHEDULER */

Expand Down
28 changes: 17 additions & 11 deletions src/audio/src/src_ipc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <sof/audio/sink_api.h>
#include <sof/audio/source_api.h>
#include <sof/audio/sink_source_utils.h>
#include <sof/schedule/ll_schedule_domain.h>
#include <rtos/panic.h>
#include <sof/ipc/msg.h>
#include <rtos/alloc.h>
Expand Down Expand Up @@ -99,17 +100,22 @@ int src_set_params(struct processing_module *mod, struct sof_sink *sink)

src_params.frame_fmt = valid_fmt;
ret = sink_set_params(sink, &src_params, true);

/* if module is to be run as DP, calculate module period
* according to OBS size and data rate
* as SRC uses period value to calculate its internal buffers,
* it must be done here, right after setting sink parameters
*/
if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
dev->period = 1000000 * sink_get_min_free_space(sink) /
(sink_get_frame_bytes(sink) * sink_get_rate(sink));

comp_info(dev, "SRC DP period calculated as: %u", dev->period);
if (dev->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
/* if module is to be run as DP, calculate module period
* according to OBS size and data rate
* as SRC uses period value to calculate its internal buffers,
* it must be done here, right after setting sink parameters
*
* calculate period using 64bit integer to avoid overflows
*/
dev->period = 1000000ULL * sink_get_min_free_space(sink) /
(sink_get_frame_bytes(sink) * sink_get_rate(sink));
/* align down period to LL cycle time */
dev->period /= LL_TIMER_PERIOD_US;
dev->period *= LL_TIMER_PERIOD_US;

comp_info(dev, "SRC DP period calculated as: %u us", dev->period);
}
Copy link
Collaborator

@kv2019i kv2019i Mar 28, 2025

Choose a reason for hiding this comment

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

Could this be a helper or otherwise shared code? Looks wrong to have DP constraints in multiple places in code. I.e. if the period needs to be aligned because of SRC specific reasons, it's good to have here, but if it's a limitation of DP infra, then it should be a common function used by all DP modules.

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

unfortunately SRC module needs a calculated period for internal purposes at "set params". It happens a bit earlier than "prepare", when module adapter do a "general" period calculation

So, in case of current SRC implementation, it cannot be unified.

in general, if a module wants to enforce its own period instead of a calculated one, it can do it just by setting dev->period to desired value. SRC does it - but for different reasons

Copy link
Member

@lgirdwood lgirdwood Mar 31, 2025

Choose a reason for hiding this comment

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

Lets make the automatic DP config calculation the default state, but allow the modules to override with some API calls as needed. This should all be visible in the logs for debug.

Copy link
Member

@lgirdwood lgirdwood Mar 31, 2025

Choose a reason for hiding this comment

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

@singalsu fyi - some SRC updates may be needed for DP.

Copy link
Collaborator

@singalsu singalsu Mar 31, 2025

Choose a reason for hiding this comment

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

Maybe calculating some things in SRC can be moved to prepare() if need. SRC needs a target period duration and it tries to match than with polyphase filter bank(s) constraints. In some cases with 44.1 family <--> 48 family rates those can't be met exactly.


component_set_nearest_period_frames(dev, src_params.rate);
/* Update module stream_params */
Expand Down
Loading