Add object array abstraction to ipc4_module_init_ext_init and use it to pass stack and heap size for DP modules #10064

jsarha · 2025-06-17T09:55:43Z

Extend the existing struct ipc4_module_init_ext_init payload with an abstract object array, and use the object array to pass DP module memory configuration e.g. stack and heap size. The PR contains decoding of the payload and extraction of stack and heap size properties, but no actual functionality for applying the memory configuration for the module. The decoding also lacks the decoding of rtos_domain and gna_used flags and payload.

lgirdwood

LGTM, just some opens. @ranj063 pls also review since this may be used in your params patch ?

src/include/ipc4/module.h

tools/topology/topology2/include/common/tokens.conf

lgirdwood · 2025-06-18T15:05:09Z

src/include/module/module/base.h

 	struct ipc4_input_pin_format *input_pins;
 	struct ipc4_output_pin_format *output_pins;
+	uint32_t stack_size;	/* stack size in bytes, 0 means default value */
+	uint32_t heap_size;	/* stack size in bytes, 0 means default value */


This will be max size for heap. i.e. the module may allocate less than this size but never more.

src/audio/module_adapter/module_adapter_ipc4.c

jsarha · 2025-06-19T07:14:04Z

Let's mark this ready for review to get mor attention.

lgirdwood · 2025-06-25T12:06:37Z

@jsarha LGTM, but last HACK patch, can it be made non hack for SRC using DP ?

lyakh · 2025-06-26T07:23:28Z

src/include/module/module/base.h

 	struct ipc4_output_pin_format *output_pins;
+	uint32_t domain_id;	/* userspace domain ID */
+	uint32_t stack_bytes;	/* stack size in bytes, 0 means default value */
+	uint32_t heap_bytes;	/* stack size in bytes, 0 means default value */


typo in the comment: "heap size"

jsarha · 2025-06-27T10:50:04Z

This PR is bound to conflict with #10009 , but I fix which ever was left behind.

kv2019i

A few comments, please see inline.

src/audio/module_adapter/module_adapter_ipc4.c

kv2019i · 2025-07-01T10:16:43Z

src/audio/module_adapter/module_adapter_ipc4.c

+	if (config->ipc_extended_init)
+		cfg = module_ext_init_decode(dev, dst, args->data, &cfgsz);
+	else
+		cfg = (const struct ipc4_base_module_extended_cfg *)args->data;


Same comment as to kernel PR. 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 two "extended" module configs is highly confusing. Can we call this new variant (ipc_extended_init) something clearly different?

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.

So @kv2019i , should I go and rename struct ipc4_module_init_ext_init that has been there since 6c29004 ? Or would something less be enough?

What I find most annoying is that the both payload extesions are described in completely different places in the source tree without any cross reference or comment that the both entities may exist in the same payload. The comments here

sof/src/include/ipc4/module.h

Line 175 in 4ecaa95

*/

give an idea that this is all that should be in module init payload.

Would it be enough if I would document in the above place that struct ipc4_base_module_cfg_ext may follow the already mentioned structs structs, and document the situation the same way on the Linux side?

BTW it currently does not mention here

sof/src/include/module/ipc4/base-config.h

Line 149 in 4ecaa95

struct ipc4_base_module_cfg_ext {

at all how, when, or where this struct is used. Is the payload of module init message the only place or is there some other uses too? I could add some comments there too, but I am not certain what they should be.

Ack @jsarha . I got (again) confused we extended the module init message both at 1) TOP level (t ipc4_module_init_ext_init) and at 2) BASEFW level (sof_ipc4_base_module_cfg). Given the naming predates your PR, no need to rename now.

src/audio/module_adapter/module_adapter_ipc4.c

src/include/module/module/base.h

jsarha · 2025-07-01T21:37:05Z

src/audio/module_adapter/module_adapter_ipc4.c

+	if (config->ipc_extended_init)
+		cfg = module_ext_init_decode(dev, dst, args->data, &cfgsz);
+	else
+		cfg = (const struct ipc4_base_module_extended_cfg *)args->data;


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.

So @kv2019i , should I go and rename struct ipc4_module_init_ext_init that has been there since 6c29004 ? Or would something less be enough?

What I find most annoying is that the both payload extesions are described in completely different places in the source tree without any cross reference or comment that the both entities may exist in the same payload. The comments here

sof/src/include/ipc4/module.h

Line 175 in 4ecaa95

*/

give an idea that this is all that should be in module init payload.

Would it be enough if I would document in the above place that struct ipc4_base_module_cfg_ext may follow the already mentioned structs structs, and document the situation the same way on the Linux side?

BTW it currently does not mention here

sof/src/include/module/ipc4/base-config.h

Line 149 in 4ecaa95

struct ipc4_base_module_cfg_ext {

at all how, when, or where this struct is used. Is the payload of module init message the only place or is there some other uses too? I could add some comments there too, but I am not certain what they should be.

kv2019i

Thanks @jsarha , looks good now.

kv2019i · 2025-07-02T09:26:02Z

src/audio/module_adapter/module_adapter_ipc4.c

+	if (config->ipc_extended_init)
+		cfg = module_ext_init_decode(dev, dst, args->data, &cfgsz);
+	else
+		cfg = (const struct ipc4_base_module_extended_cfg *)args->data;


Ack @jsarha . I got (again) confused we extended the module init message both at 1) TOP level (t ipc4_module_init_ext_init) and at 2) BASEFW level (sof_ipc4_base_module_cfg). Given the naming predates your PR, no need to rename now.

lgirdwood · 2025-07-02T10:29:44Z

@jsarha can you fix conflicts. Thanks !

Adds definition, enums, and bits for adding array of struct ipc4_module_init_ext_object:s with associated data to struct ipc4_module_init_ext_init payload. Also adds struct ipc4_module_init_ext_obj_dp_data as an object to be sent as an object in the array. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

Add ipc_extended_init to struct comp_ipc_config to let component know that a struct ipc4_module_init_ext_init and possible friends are added into the struct ipc4_module_init_instance payload. The value of the boolean is set according to extension bit 29 of struct ipc4_module_init_instance message. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

…onfig Add domain_id, stack_bytes and heap_bytes members to module_config. These may be be filled from ext_init payload. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

jsarha · 2025-07-02T13:29:02Z

@jsarha can you fix conflicts. Thanks !

@lgirdwood done

ranj063

LGTM

wszypelt · 2025-07-04T08:39:19Z

@jsarha TGL IPC4 not building correctly in Internal Intel CI System

jsarha · 2025-07-04T09:37:31Z

@jsarha TGL IPC4 not building correctly in Internal Intel CI System

I simply do not understand what is wrong with this line:
https://github.com/jsarha/sof/blob/b020bd2ee3e2e762a258bb878b3125dd7aecdd30/src/audio/module_adapter/module_adapter_ipc4.c#L65

... or why this error comes:
/home/runner/work/sof/sof/workspace/sof/src/audio/module_adapter/module_adapter_ipc4.c:65:4: error: expected expression
300
const struct ipc4_module_init_ext_obj_dp_data *dp_data =

The only thing I can think of doing is adding extra { } -block after "case IPC4_MOD_INIT_DATA_ID_DP_DATA:". Let's hope for the best, but I think there is something wrong with the compiler.

lyakh · 2025-07-04T09:47:06Z

@jsarha TGL IPC4 not building correctly in Internal Intel CI System

I simply do not understand what is wrong with this line: https://github.com/jsarha/sof/blob/b020bd2ee3e2e762a258bb878b3125dd7aecdd30/src/audio/module_adapter/module_adapter_ipc4.c#L65

... or why this error comes: /home/runner/work/sof/sof/workspace/sof/src/audio/module_adapter/module_adapter_ipc4.c:65:4: error: expected expression 300 const struct ipc4_module_init_ext_obj_dp_data *dp_data =

The only thing I can think of doing is adding extra { } -block after "case IPC4_MOD_INIT_DATA_ID_DP_DATA:". Let's hope for the best, but I think there is something wrong with the compiler.

@jsarha a variable declaration cannot be the first statement after case ...:

jsarha · 2025-07-04T10:18:30Z

Added { } -block around "case IPC4_MOD_INIT_DATA_ID_DP_DATA:" :

https://github.com/jsarha/sof/blob/b7e2baef6aef3190ca143d0c5f031a55eca6b17b/src/audio/module_adapter/module_adapter_ipc4.c#L65

Add module_ext_init_decode() for struct ipc4_module_init_ext_init payload decoding. The function goes decodes ext_init and following object array payload. The only recognized object so far is struct sof_ipc4_mod_init_ext_dp_memory_data. The possibly found stack and heap size requirements are copied to struct module_config, but no other functionality is added. This first version ignores rtos_domain and gna_used flags, and fails if their associated data is found in the message payload. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

…utes Define domain_id, stack_bytes_requirement and heap_bytes_requirement widget attributes. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

…P SRC Adds domain_id, stack and heap_size_requirements values to SRCs widgets if SRC's scheduling domain is DP. At this phase the values are there only for testing purposes and they do not represent SRC modules actual requirements. The values are sent to FW using ipc4 module init message's ext_init payload and its object array. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

jsarha · 2025-07-05T20:57:40Z

Changed module_ext_init_decode() size parameter from uint32_t to size_t to fix a static analysis warning.

jsarha mentioned this pull request Jun 17, 2025

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

Merged

lgirdwood reviewed Jun 18, 2025

View reviewed changes

jsarha marked this pull request as ready for review June 19, 2025 07:13

jsarha requested review from RanderWang, dbaluta, jxstelter, kv2019i, lbetlej, marcinszkudlinski, mmaka1, pblaszko, plbossart and ranj063 as code owners June 19, 2025 07:13

jsarha force-pushed the comp_heap_and_stack_size_limits branch from 36651d9 to 0f9308a Compare June 24, 2025 22:03

lgirdwood approved these changes Jun 25, 2025

View reviewed changes

lyakh reviewed Jun 26, 2025

View reviewed changes

jsarha force-pushed the comp_heap_and_stack_size_limits branch from 0f9308a to 5d3c13e Compare June 26, 2025 12:36

lgirdwood approved these changes Jun 27, 2025

View reviewed changes

kv2019i requested changes Jul 1, 2025

View reviewed changes

jsarha commented Jul 1, 2025

View reviewed changes

jsarha force-pushed the comp_heap_and_stack_size_limits branch from 5d3c13e to c87722b Compare July 1, 2025 21:58

kv2019i approved these changes Jul 2, 2025

View reviewed changes

lgirdwood approved these changes Jul 2, 2025

View reviewed changes

Jyri Sarha added 3 commits July 2, 2025 15:13

module/base: Add domain id, stack- and heap bytes members to module_c…

355dcf0

…onfig Add domain_id, stack_bytes and heap_bytes members to module_config. These may be be filled from ext_init payload. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

jsarha force-pushed the comp_heap_and_stack_size_limits branch from c87722b to b020bd2 Compare July 2, 2025 13:28

ranj063 approved these changes Jul 3, 2025

View reviewed changes

jsarha force-pushed the comp_heap_and_stack_size_limits branch from b020bd2 to b7e2bae Compare July 4, 2025 10:05

Jyri Sarha added 3 commits July 5, 2025 23:55

topology2: domain_id stack- and heap_bytes_requirements widget attrib…

aeb63bb

…utes Define domain_id, stack_bytes_requirement and heap_bytes_requirement widget attributes. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>

jsarha force-pushed the comp_heap_and_stack_size_limits branch from b7e2bae to 3b67e6a Compare July 5, 2025 20:55

lgirdwood approved these changes Jul 7, 2025

View reviewed changes

lgirdwood merged commit b3b10a0 into thesofproject:main Jul 7, 2025
37 of 44 checks passed

Add object array abstraction to ipc4_module_init_ext_init and use it to pass stack and heap size for DP modules #10064

Add object array abstraction to ipc4_module_init_ext_init and use it to pass stack and heap size for DP modules #10064

Uh oh!

Conversation

jsarha commented Jun 17, 2025

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsarha commented Jun 19, 2025

Uh oh!

lgirdwood commented Jun 25, 2025

Uh oh!

lyakh Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

jsarha commented Jun 27, 2025

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kv2019i Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

jsarha Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jsarha Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jul 2, 2025

Uh oh!

jsarha commented Jul 2, 2025

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

wszypelt commented Jul 4, 2025

Uh oh!

jsarha commented Jul 4, 2025

Uh oh!

lyakh commented Jul 4, 2025

Uh oh!

jsarha commented Jul 4, 2025

Uh oh!

jsarha commented Jul 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jsarha Jul 1, 2025 •

edited

Loading

jsarha Jul 1, 2025 •

edited

Loading