Skip to content

Add extraction of new component properties from topology and pass them to modules with new ext_init object array. #5460

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
ujfalusi merged 4 commits into from
Aug 19, 2025
Merged

Add extraction of new component properties from topology and pass them to modules with new ext_init object array. #5460

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
5f553a0
ASoC: sof: ipc4-topology: Add topology tokens domain_in stack & heap_…
May 27, 2025
92ffbcd
ASoC: sof: Add domain_id, heap_bytes and stack_bytes to snd_sof_widget
May 27, 2025
064fd55
ASoC: SOF: ipc4: sof_ipc4_module_init_ext_init structs and macros
May 30, 2025
b7e8825
ASoC: sof ipc4: Add sof_ipc4_widget_setup_msg_payload() and call it
Jun 9, 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
75 changes: 75 additions & 0 deletions include/sound/sof/ipc4/header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,10 @@ struct sof_ipc4_base_module_cfg {
#define SOF_IPC4_MOD_EXT_DOMAIN_MASK BIT(28)
#define SOF_IPC4_MOD_EXT_DOMAIN(x) ((x) << SOF_IPC4_MOD_EXT_DOMAIN_SHIFT)

#define SOF_IPC4_MOD_EXT_EXTENDED_INIT_SHIFT 29
Copy link
Collaborator

@kv2019i kv2019i Jul 1, 2025

Choose a reason for hiding this comment

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

Same comment as to FW 10064. We already have a concept of "extended init". E.g. we have:

»       struct sof_ipc4_base_module_cfg base_config;
»       struct sof_ipc4_base_module_cfg_ext *base_config_ext;

All modules have base_config, but some have base_config_ext additionally. This new PR (and the bit29) seems to add a way to extend the module init data, but still keeps the old "base_config_ext" around as well. Having too "extended" module configs is highly confusing. Can we call this new variant something clearly different?

Copy link
Author

@jsarha jsarha Jul 1, 2025

Choose a reason for hiding this comment

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

I address all the other comments first and push a new version. I take on this tomorrow, when I am certain we have an agreement on what to do. Let's continue the discussion on FW PR side.

#define SOF_IPC4_MOD_EXT_EXTENDED_INIT_MASK BIT(29)
#define SOF_IPC4_MOD_EXT_EXTENDED_INIT(x) ((x) << SOF_IPC4_MOD_EXT_EXTENDED_SHIFT)

/* bind/unbind module ipc msg */
#define SOF_IPC4_MOD_EXT_DST_MOD_ID_SHIFT 0
#define SOF_IPC4_MOD_EXT_DST_MOD_ID_MASK GENMASK(15, 0)
Expand Down Expand Up @@ -616,6 +620,77 @@ struct sof_ipc4_notify_module_data {
#define SOF_IPC4_NOTIFY_MODULE_EVENTID_ALSA_MAGIC_VAL 0xA15A0000
#define SOF_IPC4_NOTIFY_MODULE_EVENTID_ALSA_PARAMID_MASK GENMASK(15, 0)

/*
* Macros for creating struct sof_ipc4_module_init_ext_init payload
* with its associated data. ext_init payload should be the first
* piece of payload following SOF_IPC4_MOD_INIT_INSTANCE msg, and its
* existence is indicated with SOF_IPC4_MOD_EXT_EXTENDED-bit.
*
* The macros below apply to sof_ipc4_module_init_ext_init.word0
*/
#define SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN_SHIFT 0
#define SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN_MASK BIT(0)
#define SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN(x) ((x) << SOF_IPC4_MOD_INIT_EXT_RTOS_DOMAIN_SHIFT)

#define SOF_IPC4_MOD_INIT_EXT_GNA_USED_SHIFT 1
#define SOF_IPC4_MOD_INIT_EXT_GNA_USED_MASK BIT(1)
#define SOF_IPC4_MOD_INIT_EXT_GNA_USED(x) ((x) << SOF_IPC4_MOD_INIT_EXT_GNA_USED_SHIFT)

#define SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_SHIFT 2
#define SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_MASK BIT(2)
#define SOF_IPC4_MOD_INIT_EXT_DATA_ARRAY(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_SHIFT)

struct sof_ipc4_module_init_ext_init {
u32 word0;
u32 rsvd1;
u32 rsvd2;
} __packed __aligned(4);

/*
* SOF_IPC4_MOD_EXT_EXTENDED payload may be followed by arbitrary
* number of object array objects. SOF_IPC4_MOD_INIT_EXT_DATA_ARRAY
* -bit indicates that an array object follows struct
* sof_ipc4_module_init_ext_init.
*
* The object header's SOF_IPC4_MOD_INIT_EXT_OBJ_LAST-bit in struct
* sof_ipc4_module_init_ext_object indicates if the array is continued
* with another object. The header has also fields to identify the
* object, SOF_IPC4_MOD_INIT_EXT_OBJ_ID, and to indicate the object's
* size in 32-bit words, SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS, not
* including the header itself.
*
* The macros below apply to sof_ipc4_module_init_ext_object.header
*/
#define SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_SHIFT 0
#define SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_MASK BIT(0)
#define SOF_IPC4_MOD_INIT_EXT_OBJ_LAST(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_SHIFT)

#define SOF_IPC4_MOD_INIT_EXT_OBJ_ID_SHIFT 1
#define SOF_IPC4_MOD_INIT_EXT_OBJ_ID_MASK GENMASK(15, 1)
#define SOF_IPC4_MOD_INIT_EXT_OBJ_ID(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_ID_SHIFT)

#define SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS_SHIFT 16
#define SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS_MASK GENMASK(31, 16)
#define SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS(x) ((x) << SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS_SHIFT)

struct sof_ipc4_module_init_ext_object {
u32 header;
u32 data[];
} __packed __aligned(4);

enum sof_ipc4_mod_init_ext_obj_id {
SOF_IPC4_MOD_INIT_DATA_ID_INVALID = 0,
SOF_IPC4_MOD_INIT_DATA_ID_DP_DATA,
SOF_IPC4_MOD_INIT_DATA_ID_MAX = SOF_IPC4_MOD_INIT_DATA_ID_DP_DATA,
};

/* DP module memory configuration data object for ext_init object array */
struct sof_ipc4_mod_init_ext_dp_memory_data {
u32 domain_id; /* userspace domain ID */
u32 stack_bytes; /* stack size in bytes, 0 means default size */
u32 heap_bytes; /* stack size in bytes, 0 means default size */
} __packed __aligned(4);

/** @}*/

#endif
3 changes: 3 additions & 0 deletions include/uapi/sound/sof/tokens.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@
#define SOF_TKN_COMP_NO_WNAME_IN_KCONTROL_NAME 417

#define SOF_TKN_COMP_SCHED_DOMAIN 418
#define SOF_TKN_COMP_DOMAIN_ID 419
#define SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT 420
#define SOF_TKN_COMP_STACK_BYTES_REQUIREMENT 421

/* SSP */
#define SOF_TKN_INTEL_SSP_CLKS_CONTROL 500
Expand Down
91 changes: 91 additions & 0 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@ static const struct sof_topology_token comp_ext_tokens[] = {
offsetof(struct snd_sof_widget, core)},
{SOF_TKN_COMP_SCHED_DOMAIN, SND_SOC_TPLG_TUPLE_TYPE_STRING, get_token_comp_domain,
offsetof(struct snd_sof_widget, comp_domain)},
{SOF_TKN_COMP_DOMAIN_ID, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
offsetof(struct snd_sof_widget, dp_domain_id)},
{SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
offsetof(struct snd_sof_widget, dp_heap_bytes)},
{SOF_TKN_COMP_STACK_BYTES_REQUIREMENT, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
offsetof(struct snd_sof_widget, dp_stack_bytes)},
};

static const struct sof_topology_token gain_tokens[] = {
Expand Down Expand Up @@ -2946,13 +2952,85 @@ static int sof_ipc4_control_setup(struct snd_sof_dev *sdev, struct snd_sof_contr
return 0;
}

static int sof_ipc4_widget_setup_msg_payload(struct snd_sof_dev *sdev,
struct snd_sof_widget *swidget,
struct sof_ipc4_msg *msg,
void *ipc_data, u32 ipc_size,
void **new_data)
{
struct sof_ipc4_mod_init_ext_dp_memory_data *dp_mem_data;
struct sof_ipc4_module_init_ext_init *ext_init;
struct sof_ipc4_module_init_ext_object *hdr;
int new_size;
u32 *payload;
u32 ext_pos;

/* For the moment the only reason for adding init_ext_init payload is DP
* memory data. If both stack and heap size are 0 (= use default), then
* there is no need for init_ext_init payload.
*/
if (swidget->comp_domain != SOF_COMP_DOMAIN_DP) {
msg->extension &= ~SOF_IPC4_MOD_EXT_EXTENDED_INIT_MASK;
return 0;
}

payload = kzalloc(sdev->ipc->max_payload_size, GFP_KERNEL);
if (!payload)
return -ENOMEM;

/* Add ext_init first and set objects array flag to 1 */
ext_init = (struct sof_ipc4_module_init_ext_init *)payload;
ext_init->word0 |= SOF_IPC4_MOD_INIT_EXT_OBJ_ARRAY_MASK;
ext_pos = DIV_ROUND_UP(sizeof(*ext_init), sizeof(u32));

/* Add object array objects after ext_init */

/* Add dp_memory_data if comp_domain indicates DP */
if (swidget->comp_domain == SOF_COMP_DOMAIN_DP) {
hdr = (struct sof_ipc4_module_init_ext_object *)&payload[ext_pos];
hdr->header = SOF_IPC4_MOD_INIT_EXT_OBJ_LAST_MASK |
SOF_IPC4_MOD_INIT_EXT_OBJ_ID(SOF_IPC4_MOD_INIT_DATA_ID_DP_DATA) |
SOF_IPC4_MOD_INIT_EXT_OBJ_WORDS(DIV_ROUND_UP(sizeof(*dp_mem_data),
sizeof(u32)));
ext_pos += DIV_ROUND_UP(sizeof(*hdr), sizeof(u32));
dp_mem_data = (struct sof_ipc4_mod_init_ext_dp_memory_data *)&payload[ext_pos];
dp_mem_data->domain_id = swidget->dp_domain_id;
dp_mem_data->stack_bytes = swidget->dp_stack_bytes;
dp_mem_data->heap_bytes = swidget->dp_heap_bytes;
ext_pos += DIV_ROUND_UP(sizeof(*dp_mem_data), sizeof(u32));
}

/* If another array object is added, remember clear previous OBJ_LAST bit */

/* Calculate final size and check that it fits to max payload size */
new_size = ext_pos * sizeof(u32) + ipc_size;
if (new_size > sdev->ipc->max_payload_size) {
dev_err(sdev->dev, "Max ipc payload size %lu exceeded: %u",
sdev->ipc->max_payload_size, new_size);
kfree(payload);
return -EINVAL;
}
*new_data = payload;

/* Copy module specific ipc_payload to end */
memcpy(&payload[ext_pos], ipc_data, ipc_size);

/* Update msg extension bits according to the payload changes */
msg->extension |= SOF_IPC4_MOD_EXT_EXTENDED_INIT_MASK;
msg->extension &= ~SOF_IPC4_MOD_EXT_PARAM_SIZE_MASK;
msg->extension |= SOF_IPC4_MOD_EXT_PARAM_SIZE(DIV_ROUND_UP(new_size, sizeof(u32)));

return new_size;
}

static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
{
struct snd_sof_widget *pipe_widget = swidget->spipe->pipe_widget;
struct sof_ipc4_fw_data *ipc4_data = sdev->private;
struct sof_ipc4_pipeline *pipeline;
struct sof_ipc4_msg *msg;
void *ipc_data = NULL;
void *ext_data = NULL;
u32 ipc_size = 0;
int ret;

Expand Down Expand Up @@ -3097,6 +3175,16 @@ static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget
dev_dbg(sdev->dev, "Create widget %s (pipe %d) - ID %d, instance %d, core %d\n",
swidget->widget->name, swidget->pipeline_id, module_id,
swidget->instance_id, swidget->core);

ret = sof_ipc4_widget_setup_msg_payload(sdev, swidget, msg, ipc_data, ipc_size,
Copy link
Collaborator

@ranj063 ranj063 Jun 18, 2025

Choose a reason for hiding this comment

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

@jsarha does it make sense to move this to each widget's setup op? I mean this is static data parsed from the topology right? So it isnt going to change from every instance of start/stop isnt it?

Copy link
Author

@jsarha jsarha Jun 19, 2025

Choose a reason for hiding this comment

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

I put it there only because that is where the IPC payload is put together. The ext_init payload could be created already at sof_ipc4_widget_setup_msg() and store it somewhere, but we would still have to allocate memory and concatenate different payloads together in sof_ipc4_widget_setup(), so the benefit would be mostly cosmetic.

That is unless I take on a bigger rework, find a standard place for setup msg payloads in snd_sof_widget for all modules (except pipelines), and put custom made payloads there. That would still only be a trade from saved cycles to consumed memory.

Copy link
Collaborator

@ranj063 ranj063 Jun 23, 2025

Choose a reason for hiding this comment

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

@jsarha why would you need to allocate memory only here? You can do the same earlier as well.

Copy link
Author

@jsarha jsarha Jun 23, 2025

Choose a reason for hiding this comment

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

Currently the payloads are module specific, initialized where ever, and not having common place in sof_widget. The ext_init payload however would not depend on the module its using. It would be relatively straight forward to add a pointer for storing the ext_init payload to sof_widget in setup_msg(). However, the ext_init payload should still be concatenated with the current module specific payload in widget_setup().

I guess it would be possible to create the full IPC payloads also already at topology read phase. That would however require fixing the current situation, where there is no common place for init message payloads, and each module code stores it somewhere in its own private memory space.

&ext_data);
if (ret < 0)
goto fail;

if (ret > 0) {
ipc_size = ret;
ipc_data = ext_data;
}
} else {
dev_dbg(sdev->dev, "Create pipeline %s (pipe %d) - instance %d, core %d\n",
swidget->widget->name, swidget->pipeline_id,
Expand All @@ -3107,6 +3195,8 @@ static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget
msg->data_ptr = ipc_data;

ret = sof_ipc_tx_message_no_reply(sdev->ipc, msg, ipc_size);

fail:
if (ret < 0) {
dev_err(sdev->dev, "failed to create module %s\n", swidget->widget->name);

Expand All @@ -3119,6 +3209,7 @@ static int sof_ipc4_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget
}
}

kfree(ext_data);
return ret;
}

Expand Down
5 changes: 5 additions & 0 deletions sound/soc/sof/sof-audio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,11 @@ struct snd_sof_widget {
/* Scheduling domain (enum sof_comp_domain), unset, Low Latency, or Data Processing */
u32 comp_domain;

/* The values below are added to mod_init pay load if comp_domain indicates DP component */
u32 dp_domain_id; /* DP process userspace domain ID */
u32 dp_stack_bytes; /* DP process stack size requirement in bytes */
u32 dp_heap_bytes; /* DP process heap size requirement in bytes */

struct snd_soc_dapm_widget *widget;
struct list_head list; /* list in sdev widget list */
struct snd_sof_pipeline *spipe;
Expand Down
Loading