Audio: basefw: Implement ipc4 modules info get #9032

gbernatxintel · 2024-04-11T11:04:33Z

Add support to handle IPC4_MODULES_INFO_GET message.
This make it possible to get information about the current loaded modules.

lyakh · 2024-04-12T07:32:58Z

src/audio/base_fw.c

@@ -497,6 +520,7 @@ static int basefw_get_large_config(struct comp_dev *dev,
 case IPC4_DSP_RESOURCE_STATE:
 case IPC4_NOTIFICATION_MASK:
 case IPC4_MODULES_INFO_GET:
+ return basefw_modules_info_get(data_offset, data);


this handler will also handle IPC4_DSP_RESOURCE_STATE and IPC4_NOTIFICATION_MASK?

lyakh · 2024-04-12T07:33:08Z

src/audio/base_fw.c

+ struct sof_man_module *module_entry =
+ (struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(idx));
+ memcpy(&(module_info->modules[idx]), module_entry, sizeof(struct sof_man_module));
+ }


indentation

lyakh · 2024-04-12T07:33:20Z

src/audio/base_fw.c

+
+ module_info->modules_count = desc->header.num_module_entries;
+
+ for(int idx = 0; idx < module_info->modules_count; ++idx){


space before {

lyakh · 2024-04-12T07:33:49Z

src/audio/base_fw.c

+ for(int idx = 0; idx < module_info->modules_count; ++idx){
+ struct sof_man_module *module_entry =
+ (struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(idx));
+ memcpy(&(module_info->modules[idx]), module_entry, sizeof(struct sof_man_module));


sizeof(module_info->modules[idx]) as the last parameter

lyakh · 2024-04-12T07:36:42Z

src/audio/base_fw.c

+ memcpy(&(module_info->modules[idx]), module_entry, sizeof(struct sof_man_module));
+ }
+
+ *data_offset = sizeof(uint32_t) + module_info->modules_count * sizeof(struct sof_man_module);


I'd do sizeof(module_info->modules_count) + module_info->modules_count * sizeof(module_info->modules[0]) - makes it clearer what is calculated IMHO

kv2019i

Some review comments inline.

kv2019i · 2024-04-12T10:45:09Z

src/audio/base_fw.c

+{
+ struct ipc4_modules_info *const module_info = (struct ipc4_modules_info *)data;
+
+ struct sof_man_fw_desc *desc = (struct sof_man_fw_desc *)IMR_BOOT_LDR_MANIFEST_BASE;


This is a bit platform specific. The work to add a platform layer for IPC4 basefw is still in progress, but at a minimum, I think you need to ifdef this with "#ifdef RIMAGE_MANIFEST" like done in ipc4/helper.c.

kv2019i · 2024-04-12T10:48:21Z

src/include/ipc/header.h

@@ -329,7 +329,7 @@ struct ipc_cmd_hdr;
 #define SOF_IPC_MESSAGE_ID(x) ((x) & 0xffff)

 /** Maximum message size for mailbox Tx/Rx */
-#define SOF_IPC_MSG_MAX_SIZE 384


I think this needs a separate commit and rationale for the increase. This affects all IPC3 implementations as well and increases memory use as this is directly used to allocate memory (which so far has been 384 bytes).

Or is this something only for IPC4. Then tihs could be changed only if built for IPC4.

@gbernatxintel We can play safe here. e.g.

#if IPC4 #define SIZE 0x1000 #else #define SIZE 384 #endif

IPC fuzz failed only for IPC3 so please try this @gbernatxintel

@ujfalusi @plbossart This SOF_IPC_MSG_MAX_SIZE is only used in IPC3 code, so if this change is ifdeffed like this, I don't think we need to bump the ABI ver

@kv2019i, the kernel uses different defines for the message size (in mailbox):
include/sound/sof/header.h:#define SOF_IPC_MSG_MAX_SIZE 384
include/sound/sof/ipc4/header.h:#define SOF_IPC4_MSG_MAX_SIZE 4096

sound/soc/sof/ipc3.c: sdev->ipc->max_payload_size = SOF_IPC_MSG_MAX_SIZE;
sound/soc/sof/ipc4.c: sdev->ipc->max_payload_size = SOF_IPC4_MSG_MAX_SIZE;

lgirdwood · 2024-04-18T08:57:59Z

src/include/ipc/header.h

@@ -329,8 +329,11 @@ struct ipc_cmd_hdr;
 #define SOF_IPC_MESSAGE_ID(x) ((x) & 0xffff)

 /** Maximum message size for mailbox Tx/Rx */
+#if IPC4


Can you check the Kconfig symbol for IPC4, it should be something like CONFIG_IPC4

@gbernatxintel use #ifdef CONFIG_IPC_MAJOR_4

lgirdwood · 2024-04-18T09:30:37Z

src/include/ipc4/base_fw.h

@@ -17,6 +17,10 @@
 #ifndef __SOF_IPC4_BASE_FW_H__
 #define __SOF_IPC4_BASE_FW_H__

+#ifdef CONFIG_INTEL


Can we make this a new Kconfig as manifest data is useful and generic that could be used by anyone. e.g. KCONFIG_IPC4_MANIFEST_MODULES we can have other ipc4 manifest kconfisg too as I would imagine a lot of vendors would like to export data.

gbernatxintel · 2024-04-24T09:25:59Z

Update
Moved module_info_get functions to platform specs file, as there were problems with ipc fuzz testing. In general, the problem was that I use a structure that is defined in rimage/manifest.h. This file occurs on the intel platform, so to avoid the use of ifdef the whole thing was moved to the platform file.

@marc-hb Could you take a look at CI, is there a compilation problem?

kv2019i

#9060 was merged, so you'll need to rebase this.

kv2019i · 2024-04-24T11:12:10Z

src/include/ipc/header.h

@@ -329,7 +329,7 @@ struct ipc_cmd_hdr;
 #define SOF_IPC_MESSAGE_ID(x) ((x) & 0xffff)

 /** Maximum message size for mailbox Tx/Rx */
-#define SOF_IPC_MSG_MAX_SIZE 384


@ujfalusi @plbossart This SOF_IPC_MSG_MAX_SIZE is only used in IPC3 code, so if this change is ifdeffed like this, I don't think we need to bump the ABI ver

kv2019i · 2024-04-24T11:17:19Z

src/include/kernel/abi.h

@@ -29,7 +29,7 @@

 /** \brief SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 29
+#define SOF_ABI_MINOR 30


So this is not needed as this ABI versioning applies to IPC3 builds.

We still don't have infra to describe IPC4 infra. As this has been pending for long, let me file a ticket -> #9080

marc-hb · 2024-04-24T21:00:48Z

@marc-hb Could you take a look at CI, is there a compilation problem?

Next time please share the link and error message, more specifically:

https://github.com/thesofproject/sof/actions/runs/8814185894/job/24193491363?pr=9032

E: Failed to fetch https://packages.microsoft.com/ubuntu/22.04/prod/dists/jammy/InRelease Clearsigned file isn't valid, got 'NOSPLIT' (does the network require authentication?)

It's either very hard or impossible (depending on which check) to find results again after a force-push.

Back to the actual issue: yes this looks like a Github glitch. Since you need to resolve git conflicts anyway, it will hopefully be fixed next time.

BTW: https://www.githubstatus.com/

marc-hb · 2024-04-25T21:09:24Z

src/audio/base_fw.c

@@ -461,7 +466,9 @@ static int basefw_get_large_config(struct comp_dev *dev,
 /* TODO: add more support */
 case IPC4_DSP_RESOURCE_STATE:
 case IPC4_NOTIFICATION_MASK:
+ break;


Why is this not even printing a warning?

You seem to be splitting the "TODO", unimplemented section in the middle. This means the second part is losing its "TODO" comment and silently doing nothing with zero comment and zero warning. So now it looks even less intentional and even more like a bug.

move out the IPC4_MODULES_INFO_GET case from the TODO list.

done, currently all implemented cases are before the TODO comment

ujfalusi · 2024-04-26T06:04:52Z

src/include/ipc/header.h

@@ -329,7 +329,7 @@ struct ipc_cmd_hdr;
 #define SOF_IPC_MESSAGE_ID(x) ((x) & 0xffff)

 /** Maximum message size for mailbox Tx/Rx */
-#define SOF_IPC_MSG_MAX_SIZE 384


@kv2019i, the kernel uses different defines for the message size (in mailbox):
include/sound/sof/header.h:#define SOF_IPC_MSG_MAX_SIZE 384
include/sound/sof/ipc4/header.h:#define SOF_IPC4_MSG_MAX_SIZE 4096

sound/soc/sof/ipc3.c: sdev->ipc->max_payload_size = SOF_IPC_MSG_MAX_SIZE;
sound/soc/sof/ipc4.c: sdev->ipc->max_payload_size = SOF_IPC4_MSG_MAX_SIZE;

ujfalusi · 2024-04-26T06:06:53Z

src/audio/base_fw.c

@@ -461,7 +466,9 @@ static int basefw_get_large_config(struct comp_dev *dev,
 /* TODO: add more support */
 case IPC4_DSP_RESOURCE_STATE:
 case IPC4_NOTIFICATION_MASK:
+ break;


move out the IPC4_MODULES_INFO_GET case from the TODO list.

ujfalusi · 2024-04-26T06:09:59Z

src/audio/base_fw.c

@@ -461,7 +466,9 @@ static int basefw_get_large_config(struct comp_dev *dev,
 /* TODO: add more support */
 case IPC4_DSP_RESOURCE_STATE:
 case IPC4_NOTIFICATION_MASK:
+ break;
 case IPC4_MODULES_INFO_GET:


The commit message can use some update.
"Added support for ipc4 query no 9."
->
"Add support to handle IPC4_MODULES_INFO_GET message."

ujfalusi · 2024-04-26T06:17:01Z

src/audio/base_fw.c

@@ -334,6 +334,11 @@ static int basefw_libraries_info_get(uint32_t *data_offset, char *data)
 return 0;
 }

+static int basefw_modules_info_get(uint32_t *data_offset, char *data)
+{
+ return platform_basefw_modules_info_get(data_offset, data);


why it is offloaded to platform function?
What is the difference between the module info and library info (basefw_libraries_info_get()) to demand this handling?

I have generally separated the function platform_basefw_modules_info_get(...) instead of implementing everything in base_fw.c because I use the ipc4_modules_info structure which uses sof_man_module.
sof_man_module is defined in rimage/manifest.h

I had a problem with the tests because on non-Intel platforms there is no manifest.h file and the tests did not pass. Besides, this modular approach follows the convention that we create code in base_fw_platform.c

With regard to the differences between basefw_modules_info_get and basefw_libraries_info_get

This basefw_libraries_info_get returns the currently loaded library and other structure.
basefw_modules_info_get returns which modules are loaded and which are not, along with information about them.

In short, both commands return a different data type, different structure. In preparing this PR I looked at previous code and tests that need specific information.

lgirdwood

@kv2019i @lyakh good for you now ?

lyakh

one error to correct

lyakh · 2024-04-30T05:52:27Z

src/platform/intel/ace/lib/base_fw_platform.c

+ module_entry, sizeof(struct sof_man_module));
+ }
+
+ *data_offset = sizeof(module_info) +


you meant sizeof(*module_info)

Good point, fixed

Maximym ipc4 message size changed to 4kB. This is the size of a page. Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>

Add support to handle IPC4_MODULES_INFO_GET message. This make it possible to get information about the current loaded modules. Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>

lgirdwood · 2024-04-30T15:09:02Z

@lyakh good now ?

gbernatxintel requested review from RanderWang, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners April 11, 2024 11:04

lyakh requested changes Apr 12, 2024

View reviewed changes

kv2019i requested changes Apr 12, 2024

View reviewed changes

gbernatxintel force-pushed the gb_modules_info_get branch from 68a51da to 11cff4b Compare April 15, 2024 13:56

gbernatxintel marked this pull request as draft April 16, 2024 08:04

gbernatxintel force-pushed the gb_modules_info_get branch 7 times, most recently from 4744f21 to 7f3f141 Compare April 17, 2024 10:38

gbernatxintel marked this pull request as ready for review April 18, 2024 07:44

gbernatxintel requested a review from a team as a code owner April 18, 2024 07:44

lgirdwood reviewed Apr 18, 2024

View reviewed changes

gbernatxintel marked this pull request as draft April 18, 2024 09:34

gbernatxintel force-pushed the gb_modules_info_get branch 6 times, most recently from 1111c34 to a710f66 Compare April 24, 2024 09:19

gbernatxintel marked this pull request as ready for review April 24, 2024 09:26

kv2019i mentioned this pull request Apr 24, 2024

[FEATURE] Add ABI versioning to IPC4 interface #9080

Open

kv2019i reviewed Apr 24, 2024

View reviewed changes

gbernatxintel force-pushed the gb_modules_info_get branch from a710f66 to ffcbe66 Compare April 25, 2024 09:01

gbernatxintel marked this pull request as draft April 25, 2024 09:02

gbernatxintel force-pushed the gb_modules_info_get branch from ffcbe66 to a625185 Compare April 25, 2024 09:45

marc-hb reviewed Apr 25, 2024

View reviewed changes

ujfalusi reviewed Apr 26, 2024

View reviewed changes

gbernatxintel force-pushed the gb_modules_info_get branch from a625185 to e61dbc6 Compare April 26, 2024 13:40

gbernatxintel marked this pull request as ready for review April 29, 2024 09:11

lgirdwood reviewed Apr 29, 2024

View reviewed changes

kv2019i approved these changes Apr 29, 2024

View reviewed changes

lyakh reviewed Apr 30, 2024

View reviewed changes

gbernatxintel added 2 commits April 30, 2024 15:04

ipc: Change ipc4 message max size

d02a74a

Maximym ipc4 message size changed to 4kB. This is the size of a page. Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>

Audio: basefw: Implement ipc4 modules info get

c429510

Add support to handle IPC4_MODULES_INFO_GET message. This make it possible to get information about the current loaded modules. Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>

gbernatxintel force-pushed the gb_modules_info_get branch from e61dbc6 to c429510 Compare April 30, 2024 13:05

lgirdwood approved these changes Apr 30, 2024

View reviewed changes

lyakh approved these changes May 2, 2024

View reviewed changes

kv2019i merged commit 25423f5 into thesofproject:main May 2, 2024
42 of 46 checks passed


		module_info->modules_count = desc->header.num_module_entries;

		for(int idx = 0; idx < module_info->modules_count; ++idx){

Audio: basefw: Implement ipc4 modules info get #9032

Audio: basefw: Implement ipc4 modules info get #9032

Conversation

gbernatxintel commented Apr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i 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

gbernatxintel commented Apr 24, 2024

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Apr 24, 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

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

lyakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Apr 30, 2024

gbernatxintel commented Apr 11, 2024 •

edited

Loading