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.

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

LLEXT: fix failures and make DRC an LLEXT module by default on MTL #9116

Merged
merged 11 commits into from
Jul 16, 2024
2 changes: 1 addition & 1 deletion app/boards/intel_adsp_ace15_mtpm.conf
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CONFIG_IPC4_BASE_FW_INTEL=y
CONFIG_COMP_SRC=y
CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y
CONFIG_COMP_SRC_LITE=y
CONFIG_COMP_DRC=y
CONFIG_COMP_DRC=m
CONFIG_COMP_CROSSOVER=y
CONFIG_COMP_MULTIBAND_DRC=y

Expand Down
2 changes: 1 addition & 1 deletion app/boards/intel_adsp_ace20_lnl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ CONFIG_IPC4_BASE_FW_INTEL=y

CONFIG_COMP_SRC=y
CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y
CONFIG_COMP_DRC=y
CONFIG_COMP_DRC=m

# power settings
CONFIG_PM=y
Expand Down
1 change: 1 addition & 0 deletions app/overlays/lnl/module_overlay.conf
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
CONFIG_SAMPLE_SMART_AMP=m
CONFIG_COMP_DRC=m
1 change: 1 addition & 0 deletions app/overlays/mtl/module_overlay.conf
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
CONFIG_SAMPLE_SMART_AMP=m
CONFIG_COMP_MIXIN_MIXOUT=m
CONFIG_COMP_IIR=m
CONFIG_COMP_DRC=m
1 change: 1 addition & 0 deletions app/overlays/repro-build.conf
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
CONFIG_OUTPUT_DISASSEMBLY=y
CONFIG_OUTPUT_DISASSEMBLY_WITH_SOURCE=n
CONFIG_MODULES=n
6 changes: 5 additions & 1 deletion scripts/xtensa-build-zephyr.py
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,11 @@ def install_lib(sof_lib_dir, abs_build_dir, platform_wconfig):
llext_input = entry_path / (llext_base + '.llext')
llext_output = entry_path / (llext_file + '.ri')

sign_cmd = [platform_wconfig.get("rimage.path"), "-o", str(llext_output),
# See why the shlex() parsing step is required at
# https://docs.zephyrproject.org/latest/develop/west/sign.html#rimage
# and in Zephyr commit 030b740bd1ec
rimage_cmd = shlex.split(platform_wconfig.get('rimage.path'))[0]
sign_cmd = [rimage_cmd, "-o", str(llext_output),
"-e", "-c", str(rimage_cfg),
"-k", str(signing_key), "-l", "-r",
str(llext_input)]
Expand Down
2 changes: 1 addition & 1 deletion src/audio/drc/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
rsource "Kconfig.simd"

config COMP_DRC
bool "Dynamic Range Compressor component"
tristate "Dynamic Range Compressor component"
select CORDIC_FIXED
select MATH_LUT_SINE_FIXED
select NUMBERS_NORM
Expand Down
23 changes: 19 additions & 4 deletions src/audio/drc/drc.c
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,6 @@ static int drc_free(struct processing_module *mod)
{
struct drc_comp_data *cd = module_get_private_data(mod);

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

comp_data_blob_handler_free(cd->model_handler);
rfree(cd);
return 0;
Expand Down Expand Up @@ -370,8 +368,6 @@ static int drc_reset(struct processing_module *mod)
{
struct drc_comp_data *cd = module_get_private_data(mod);

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

drc_reset_state(&cd->state);

return 0;
Expand All @@ -389,3 +385,22 @@ static const struct module_interface drc_interface = {

DECLARE_MODULE_ADAPTER(drc_interface, drc_uuid, drc_tr);
SOF_MODULE_INIT(drc, sys_comp_module_drc_interface_init);

#if CONFIG_COMP_DRC_MODULE
/* modular: llext dynamic link */

#include <module/module/api_ver.h>
#include <module/module/llext.h>
#include <rimage/sof/user/manifest.h>

#define UUID_DRC 0xda, 0xe4, 0x6e, 0xb3, 0x6f, 0x00, 0xf9, 0x47, \
Copy link
Member

Choose a reason for hiding this comment

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

@andyross is this needed with your changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's compatible (UUIDs aren't required to be registered), but yes, this should be added to the registry.

0xa0, 0x6d, 0xfe, 0xcb, 0xe2, 0xd8, 0xb6, 0xce

SOF_LLEXT_MOD_ENTRY(drc, &drc_interface);

static const struct sof_man_module_manifest mod_manifest __section(".module") __used =
SOF_LLEXT_MODULE_MANIFEST("DRC", drc_llext_entry, 1, UUID_DRC, 40);

SOF_LLEXT_BUILDINFO;

#endif
6 changes: 5 additions & 1 deletion src/audio/drc/drc.toml
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
#ifndef LOAD_TYPE
#define LOAD_TYPE "0"
#endif

REM # DRC module config
[[module.entry]]
name = "DRC"
uuid = "B36EE4DA-006F-47F9-A06D-FECBE2D8B6CE"
affinity_mask = "0x1"
instance_count = "40"
domain_types = "0"
load_type = "0"
load_type = LOAD_TYPE
module_type = "9"
auto_start = "0"
sched_caps = [1, 0x00008000]
Expand Down
13 changes: 13 additions & 0 deletions src/audio/drc/llext/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Copyright (c) 2024 Intel Corporation.
# SPDX-License-Identifier: Apache-2.0

# Hard-coded .text address to be moved to a common place
sof_llext_build("drc"
SOURCES ../drc.c
../drc_generic.c
../drc_math_generic.c
../drc_hifi3.c
../drc_hifi4.c
../drc_math_hifi3.c
TEXT_ADDR 0xa068a000
)
6 changes: 6 additions & 0 deletions src/audio/drc/llext/llext.toml.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include <tools/rimage/config/platform.toml>
#define LOAD_TYPE "2"
#include "../drc.toml"

[module]
count = __COUNTER__
6 changes: 1 addition & 5 deletions src/audio/eq_iir/eq_iir.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ static int eq_iir_free(struct processing_module *mod)
{
struct comp_data *cd = module_get_private_data(mod);

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

eq_iir_free_delaylines(cd);
comp_data_blob_handler_free(cd->model_handler);

Expand Down Expand Up @@ -234,8 +232,6 @@ static int eq_iir_reset(struct processing_module *mod)
struct comp_data *cd = module_get_private_data(mod);
int i;

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

eq_iir_free_delaylines(cd);

cd->eq_iir_func = NULL;
Expand Down Expand Up @@ -271,7 +267,7 @@ SOF_MODULE_INIT(eq_iir, sys_comp_module_eq_iir_interface_init);
SOF_LLEXT_MOD_ENTRY(eq_iir, &eq_iir_interface);

static const struct sof_man_module_manifest mod_manifest __section(".module") __used =
SOF_LLEXT_MODULE_MANIFEST("EQIIR", eq_iir_llext_entry, 1, UUID_EQIIR);
SOF_LLEXT_MODULE_MANIFEST("EQIIR", eq_iir_llext_entry, 1, UUID_EQIIR, 40);
Copy link
Member

Choose a reason for hiding this comment

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

Can we do a -1 i.e. dont care ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lgirdwood actually we do care now. Not because LLEXT modules need this, but because it's used by the infrastructure. We should remove this for LLEXT, but that should be coordinated with other loadable module implementations

Copy link
Member

Choose a reason for hiding this comment

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

ok, lets circle back on this, we need to avoid artificial hard coded limitations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lgirdwood this is where it's used and this is where it used to fail without these two commits: https://github.com/thesofproject/linux/blob/8245a1411fef45a208f182c176660107b7f582aa/sound/soc/sof/ipc4-topology.c#L1119 So we'd need to modify the driver to allow arbitrarily large instance IDs, or we can define and use a macro like #define LLEXT_MODULE_INSTANCE_COUNT 65535 and use it for all LLEXT modules?

Copy link
Member

Choose a reason for hiding this comment

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

@lyakh pls align with @ujfalusi re driver updates as he's back soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can as well use ida_alloc() in kernel and ignore the max, but I don't know what LLEXT vs non LLEXT modules are and why would they need to be handled differently...


SOF_LLEXT_BUILDINFO;

Expand Down
12 changes: 2 additions & 10 deletions src/audio/mixin_mixout/mixin_mixout.c
Original file line number Diff line number Diff line change
Expand Up @@ -175,17 +175,14 @@ static int mixout_init(struct processing_module *mod)
static int mixin_free(struct processing_module *mod)
{
struct mixin_data *md = module_get_private_data(mod);
struct comp_dev *dev = mod->dev;

comp_dbg(dev, "mixin_free()");
rfree(md);

return 0;
}

static int mixout_free(struct processing_module *mod)
{
comp_dbg(mod->dev, "mixout_free()");
rfree(module_get_private_data(mod));

return 0;
Expand Down Expand Up @@ -556,9 +553,6 @@ static int mixout_process(struct processing_module *mod,
static int mixin_reset(struct processing_module *mod)
{
struct mixin_data *mixin_data = module_get_private_data(mod);
struct comp_dev *dev = mod->dev;

comp_dbg(dev, "mixin_reset()");

mixin_data->mix = NULL;
mixin_data->gain_mix = NULL;
Expand All @@ -570,8 +564,6 @@ static int mixout_reset(struct processing_module *mod)
{
struct comp_dev *dev = mod->dev;

comp_dbg(dev, "mixout_reset()");

/* FIXME: move this to module_adapter_reset() */
if (dev->pipeline->source_comp->direction == SOF_IPC_STREAM_PLAYBACK) {
int i;
Expand Down Expand Up @@ -982,8 +974,8 @@ SOF_LLEXT_MOD_ENTRY(mixout, &mixout_interface);

static const struct sof_man_module_manifest mod_manifest[] __section(".module") __used =
{
SOF_LLEXT_MODULE_MANIFEST("MIXIN", mixin_llext_entry, 1, UUID_MIXIN),
SOF_LLEXT_MODULE_MANIFEST("MIXOUT", mixout_llext_entry, 1, UUID_MIXOUT),
SOF_LLEXT_MODULE_MANIFEST("MIXIN", mixin_llext_entry, 1, UUID_MIXIN, 30),
SOF_LLEXT_MODULE_MANIFEST("MIXOUT", mixout_llext_entry, 1, UUID_MIXOUT, 30),
};

SOF_LLEXT_BUILDINFO;
Expand Down
2 changes: 1 addition & 1 deletion src/audio/multiband_drc/Kconfig
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: BSD-3-Clause

config COMP_MULTIBAND_DRC
depends on COMP_IIR && COMP_CROSSOVER && COMP_DRC
depends on COMP_IIR && COMP_CROSSOVER && COMP_DRC = y
bool "Multiband Dynamic Range Compressor component"
select CORDIC_FIXED
select COMP_BLOB
Expand Down
22 changes: 20 additions & 2 deletions src/audio/pipeline/pipeline-stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,15 @@ static int pipeline_calc_cps_consumption(struct comp_dev *current,

if (cd->cpc == 0) {
/* Use maximum clock budget, assume 1ms chunk size */
ppl_data->kcps[comp_core] = CLK_MAX_CPU_HZ / 1000;
uint32_t core_kcps = core_kcps_get(comp_core);

if (!current->kcps_inc[comp_core]) {
current->kcps_inc[comp_core] = core_kcps;
ppl_data->kcps[comp_core] = CLK_MAX_CPU_HZ / 1000 - core_kcps;
} else {
ppl_data->kcps[comp_core] = core_kcps - current->kcps_inc[comp_core];
current->kcps_inc[comp_core] = 0;
}
tr_warn(pipe,
"0 CPS requested for module: %#x, core: %d using safe max KCPS: %u",
current->ipc_config.id, comp_core, ppl_data->kcps[comp_core]);
Expand All @@ -367,6 +375,9 @@ int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd)
{
int ret;
#if CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL
/* FIXME: this must be a platform-specific parameter or a Kconfig option */
#define DSP_MIN_KCPS 50000
Copy link
Member

Choose a reason for hiding this comment

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

There is no point in keeping KCPS feature with such limit because it means we cannot go lower than highest available clock which is not true because there are basic topologies which we can handle on 38.4.

So why is this change required for llext module?

Copy link
Collaborator Author

@lyakh lyakh Jul 16, 2024

Choose a reason for hiding this comment

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

@abonislawski what do you mean "lower than highest available clock?" Our highest available clock is hundreds of MHz, i.e. hundreds of thousands of KCPS, isn't it? So this is definitely lower than that.
This patch is needed not only for LLEXT, I think there's a bug in general in the current CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL implementation - see commit description. Using a "safe" value for modules with a 0 CPC isn't implemented correctly. After I've fixed it I started getting lock ups when destroying pipelines. E.g. I had cases when the system was starting with 20000 KCPS, then a pipeline was constructed with a 0-CPC module, so the KCPS went to the maximum. Then as the pipeline was destroyed I tried to return to 20000 KCPS and then I got IPC timeouts in the driver. Introducing a minimum of 50000 (or 40000 worked too) KCPS fixed the problem.

Copy link
Member

Choose a reason for hiding this comment

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

KCPS higher than 38400 will switch to our highest clock and DSP_MIN_KCPS is already higher than that. PRIMARY_CORE_BASE_CPS_USAGE is currently 20000 (higher than needed).
So the one thing is to fix 0CPC problem (which I thought was fixed with previous versions of this commit) but another thing is to introduce such limit like DSP_MIN_KCPS which I dont understand


struct pipeline_data data = {
.start = p->source_comp,
.p = p,
Expand Down Expand Up @@ -431,8 +442,15 @@ int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd)

for (int i = 0; i < arch_num_cpus(); i++) {
if (data.kcps[i] > 0) {
uint32_t core_kcps = core_kcps_get(i);

/* Tests showed, that we cannot go below 40000kcps on MTL */
if (data.kcps[i] > core_kcps - DSP_MIN_KCPS)
data.kcps[i] = core_kcps - DSP_MIN_KCPS;

core_kcps_adjust(i, -data.kcps[i]);
tr_info(pipe, "Sum of KCPS consumption: %d, core: %d", core_kcps_get(i), i);
tr_info(pipe, "Sum of KCPS consumption: %d, core: %d",
core_kcps, i);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/include/module/module/llext.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@
#ifndef MODULE_LLEXT_H
#define MODULE_LLEXT_H

#define SOF_LLEXT_MODULE_MANIFEST(manifest_name, entry, affinity, mod_uuid) \
#define SOF_LLEXT_MODULE_MANIFEST(manifest_name, entry, affinity, mod_uuid, instances) \
{ \
.module = { \
.name = manifest_name, \
.uuid = {mod_uuid}, \
.entry_point = (uint32_t)(entry), \
.instance_max_count = instances, \
.type = { \
.load_type = SOF_MAN_MOD_TYPE_LLEXT, \
.domain_ll = 1, \
Expand Down
4 changes: 4 additions & 0 deletions src/include/sof/audio/component.h
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,10 @@ struct comp_dev {
#if CONFIG_PERFORMANCE_COUNTERS
struct perf_cnt_data pcd;
#endif

#if CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL
int32_t kcps_inc[CONFIG_CORE_COUNT];
#endif
};

/** @}*/
Expand Down
2 changes: 1 addition & 1 deletion src/include/sof/audio/pipeline.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ struct pipeline_data {
int cmd;
uint32_t delay_ms; /* between PRE_{START,RELEASE} and {START,RELEASE} */
#if CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL
uint32_t kcps[CONFIG_CORE_COUNT]; /**< the max count of KCPS */
int32_t kcps[CONFIG_CORE_COUNT]; /**< the max count of KCPS */
#endif
};

Expand Down
10 changes: 5 additions & 5 deletions src/ipc/ipc4/helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -911,11 +911,11 @@ const struct comp_driver *ipc4_get_drv(const uint8_t *uuid)
}
}

tr_err(&comp_tr, "get_drv(): the provided UUID (%08x %08x %08x %08x) can't be found!",
*(uint32_t *)(&uuid[0]),
*(uint32_t *)(&uuid[4]),
*(uint32_t *)(&uuid[8]),
*(uint32_t *)(&uuid[12]));
tr_warn(&comp_tr, "get_drv(): the provided UUID (%08x %08x %08x %08x) can't be found!",
abonislawski marked this conversation as resolved.
Show resolved Hide resolved
*(uint32_t *)(&uuid[0]),
*(uint32_t *)(&uuid[4]),
*(uint32_t *)(&uuid[8]),
*(uint32_t *)(&uuid[12]));

out:
irq_local_enable(flags);
Expand Down
3 changes: 3 additions & 0 deletions src/math/exp_fcn.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <sof/math/numbers.h>
#include <sof/common.h>
#include <rtos/bit.h>
#include <rtos/symbol.h>
#include <stdbool.h>
#include <stdint.h>
#include <stddef.h>
Expand Down Expand Up @@ -258,6 +259,7 @@ int32_t sofm_exp_fixed(int32_t x)

return y;
}
EXPORT_SYMBOL(sofm_exp_fixed);

#endif /* EXPONENTIAL_GENERIC */

Expand All @@ -284,3 +286,4 @@ int32_t sofm_db2lin_fixed(int32_t db)
arg = (int32_t)Q_MULTSR_32X32((int64_t)db, SOFM_EXP_LOG10_DIV20_Q27, 24, 27, 27);
return sofm_exp_fixed(arg);
}
EXPORT_SYMBOL(sofm_db2lin_fixed);
2 changes: 2 additions & 0 deletions src/math/exp_fcn_hifi.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <sof/math/exp_fcn.h>
#include <sof/common.h>
#include <rtos/symbol.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
Expand Down Expand Up @@ -370,5 +371,6 @@ int32_t sofm_exp_fixed(int32_t x)

return y;
}
EXPORT_SYMBOL(sofm_exp_fixed);

#endif
2 changes: 2 additions & 0 deletions src/math/lut_trig.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <sof/audio/format.h>
#include <sof/math/lut_trig.h>
#include <rtos/symbol.h>
#include <stdint.h>

#define SOFM_LUT_SINE_C_Q20 341782638 /* 2 * SINE_NQUART / pi in Q12.20 */
Expand Down Expand Up @@ -106,3 +107,4 @@ int16_t sofm_lut_sin_fixed_16b(int32_t w)
sine = s0 + q_mults_32x32(frac, delta, Q_SHIFT_BITS_64(31, 16, 16)); /* Q1.16 */
return sat_int16((sine + 1) >> 1); /* Round to Q1.15 */
}
EXPORT_SYMBOL(sofm_lut_sin_fixed_16b);
Loading
Loading