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

[FEATURE] Move extended manifest description from toml to elf section #7304

Closed
softwarecki opened this issue Mar 20, 2023 · 7 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@softwarecki
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

In the toml files we have a description of the platform hardware and a description of the modules (extended manifest) that are embedded in the firmware. I would like to separate the two and conduct some improvements.

With toml files I see the following problems:

  • No support for file inclusion. We have a separate toml file for each platform. If a module is used by multiple platforms, we have an identical description of the module's capabilities/configuration duplicated in multiple files.
  • Lack of support for labels. At this moment, some fields of the module's configuration is represented as hexadecimal bit masks that are completely unreadable by humans.
  • Enabling/disabling building of a given module requires correcting toml files.

Describe the solution you'd like
My idea to solve these problems is to move the information from the toml files to the source code of the modules. While leaving in them only some basic description of the platform and its address space.
We can prepare a structure with the information necessary to generate the extended manifest and put it in a separate section of the elf file. It would be read by rimage (or any other tool in the future). In this way, we already provide some informations to rimage now. This is the module manifest (struct sof_man_module_manifest) in the ".module" section and the extended manifest (duplicated name, this is different informations than in toml files), (struct ext_man_header) section ".fw_metadata". Module manifest for all modules we can easily move to source code, because its reading is already implemented in rimage. The ext_man_header structure was designed to be extensible, so it could be used to pass the module's pin configuration, etc. to rimage. The advantage of this approach is that rimage (or separate tool) would build all manifests using only a compilation artifacts. Adding/removing a module would not involve updating multiple toml files for different platforms.
When creating a loadable library, we eliminate the risk of taking a toml file that does not match the compiled module. The module metadata would be included in the module file.

Describe alternatives you've considered
Alternatively, we should extend toml file support in rimage with:

  • Toml file inclusion. Move module descriptions into separate files and include them in platform files.
  • Textual value labels to make the configuration more readable.
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 20, 2023

Toml file inclusion. Move module descriptions into separate files and include them in platform files.

I wonder about a less flexible but hopefully cheaper/simpler/quicker change: clean-up the rimage code and separate extended manifest generation from .ri generation. In other words, invoke rimage twice in the build system: once for each distinct task.

This could help keep these two different jobs relatively distinct and maybe they could point at different .toml files?

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 21, 2023

In the toml files we have a description of the platform hardware and a description of the modules (extended manifest) that are embedded in the firmware. I would like to separate the two and conduct some improvements.

Another important reason to keep the two clearly separated: Zephyr has a very extensive test suite that runs on the ADSP hardware without SOF. This requires the former but not the latter.

This was just mentioned in #7270, cc: @aiChaoSONG

@RanderWang
Copy link
Collaborator

We need to strip the module extended config in elf file to pack the released fw binary, or the fw size will be increased.

Now we are familiar with the module position in extended config that mixin index = 2, mixout 3, copier 4, peak volume 5, how to keep this sequence ?

I can image one similar question: add comments in your code or give a description in a manual.

marc-hb added a commit to marc-hb/rimage that referenced this issue Apr 18, 2023
The deleted "install" target copied only the rimage executable and
left the config files behind. This gave the wrong impression that the
executable is useful without config files.

"Installing" also gave the wrong impression that rimage versions are
somewhat stable and relatively independent of SOF versions: they're
not. In fact there is no such thing as an rimage "version": everyone
should always use the exact rimage _git commit_ from the west manifest
or git submodule. There are no rimage "releases" and no semantic
versioning or anything like it, rimage is effectively part of the SOF
source and build and run directly from its build directory by
practically every SOF developer and SOF CI thanks to

  sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ...
  sof/scripts/xtensa-build-zephyr.py#def build_rimage()
  sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ...

etc.

Providing an "install" target greatly increases the chances of
different people and CIs using different rimage versions which is the
last thing we want considering the many significant rimage changes
happening right now, examples:

- zephyrproject-rtos/zephyr#56099
- zephyrproject-rtos/zephyr#54700
- thesofproject/sof#7270
- thesofproject/sof#7304
- thesofproject/sof#7320
- thesofproject#155

While it's useful for multiple files (e.g.: config files), a CMake
target was always overkill to copy a single file. Someone or some
script who really wants to copy the rimage binary to some other place
that the build directory for some (discouraged) reason _can still do
so_ with a much more basic, simpler and more transparent file copy
command.

Finally, the default "bin" DESTINATION required root access which
is otherwise totally unncessary to build SOF.

Signed-off-by: Marc Herbert <[email protected]>
marc-hb added a commit to marc-hb/rimage that referenced this issue Apr 18, 2023
The deleted "install" target copied only the rimage executable and
left the config files behind. This gave the wrong impression that the
executable is useful without config files.

"Installing" also gave the wrong impression that rimage versions are
somewhat stable and relatively independent of SOF versions: they're
not. In fact there is no such thing as an rimage "version": everyone
should always use the exact rimage _git commit_ from the west manifest
or git submodule. There are no rimage "releases" and no semantic
versioning or anything like it, rimage is effectively part of the SOF
source and build and run directly from its build directory by
practically every SOF developer and SOF CI thanks to

  sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ...
  sof/scripts/xtensa-build-zephyr.py#def build_rimage()
  sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ...

etc.

Providing an "install" target greatly increases the chances of
different people and CIs using different rimage versions which is the
last thing we want considering the many significant rimage changes
happening right now, examples:

- zephyrproject-rtos/zephyr#56099
- zephyrproject-rtos/zephyr#54700
- thesofproject/sof#7270
- thesofproject/sof#7304
- thesofproject/sof#7320
- thesofproject#155

While it's useful for multiple files (e.g.: config files), a CMake
target was always overkill to copy a single file. Someone or some
script who really wants to copy the rimage binary to some other place
that the build directory for some (discouraged) reason _can still do
so_ with a much more basic, simpler and more transparent file copy
command.

Finally, the default "bin" DESTINATION required root access which
is otherwise totally unncessary to build SOF.

Signed-off-by: Marc Herbert <[email protected]>
kv2019i pushed a commit to thesofproject/rimage that referenced this issue Apr 26, 2023
The deleted "install" target copied only the rimage executable and
left the config files behind. This gave the wrong impression that the
executable is useful without config files.

"Installing" also gave the wrong impression that rimage versions are
somewhat stable and relatively independent of SOF versions: they're
not. In fact there is no such thing as an rimage "version": everyone
should always use the exact rimage _git commit_ from the west manifest
or git submodule. There are no rimage "releases" and no semantic
versioning or anything like it, rimage is effectively part of the SOF
source and build and run directly from its build directory by
practically every SOF developer and SOF CI thanks to

  sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ...
  sof/scripts/xtensa-build-zephyr.py#def build_rimage()
  sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ...

etc.

Providing an "install" target greatly increases the chances of
different people and CIs using different rimage versions which is the
last thing we want considering the many significant rimage changes
happening right now, examples:

- zephyrproject-rtos/zephyr#56099
- zephyrproject-rtos/zephyr#54700
- thesofproject/sof#7270
- thesofproject/sof#7304
- thesofproject/sof#7320
- #155

While it's useful for multiple files (e.g.: config files), a CMake
target was always overkill to copy a single file. Someone or some
script who really wants to copy the rimage binary to some other place
that the build directory for some (discouraged) reason _can still do
so_ with a much more basic, simpler and more transparent file copy
command.

Finally, the default "bin" DESTINATION required root access which
is otherwise totally unncessary to build SOF.

Signed-off-by: Marc Herbert <[email protected]>
@lgirdwood lgirdwood added this to the v2.7 milestone Jul 4, 2023
@alex-cri alex-cri modified the milestones: v2.7, v2.8 Aug 22, 2023
@kv2019i
Copy link
Collaborator

kv2019i commented Sep 25, 2023

@softwarecki How would you envision this to tackle the IPC4 performance budgets (KCPS)? These are per-module (e.g. you need a value for "volume" but also per-platform "cycles-per-second" budget is X for one platform and Y for another platform (due to different hw or different default compiler used), even if same volume source code is built for both.

Now we tackle this by having separate toml files for platforms platX and platY.

PS #8243 and the discussion is related to this feature request.

@plbossart
Copy link
Member

This is a very interesting point @kv2019i

The modules are identified by their UUID, but nothing requires or guarantees that a module with a given UUID has the same performance requirements across multiple platforms. Different compiler options and hardware WILL result in different avg/peak requirements. Even memory could be different if it was tracked.

Worse, the module itself could have different internal algorithmic switches with fancy solutions on some higher-end platforms and less fancy ones on cost-reduced platforms.

@softwarecki
Copy link
Collaborator Author

@RanderWang

Now we are familiar with the module position in extended config that mixin index = 2, mixout 3, copier 4, peak volume 5, how to keep this sequence ?

We can keep sequence of modules in the manifest by using an array of pointers to individual manifests.

@kv2019i

How would you envision this to tackle the IPC4 performance budgets (KCPS)?

We can move platform-dependent information to a separate files placed in per-platform directories. Then, depending on the platform, we include files from the selected directory.

I quickly prepared a general sketch of the solution:

struct module_manifest_config {
	int PAR_0;
	int PAR_1;
	int PAR_2;
	int PAR_3;
	int IS_BYTES;
	int CPS;
	int IBS;
	int OBS;
	int MOD_FLAGS;
	int CPC;
	int OBLS;
};

struct module_manifest {
	char name[15];
	int affinity_mask;
	int instance_count;
	int domain_types;
	int load_type;
	int module_type;
	int auto_start;
	struct module_manifest_config *mod_cfg;
};

/* manifest/mtl/updwnmix_config.h */
struct module_manifest_config updwnmix_config[] = {
	{ 0, 0, 0, 0, 216, 706000, 12, 16, 0, 0, 0 },
	{ 1, 0, 0, 0, 216, 1271000, 8, 8, 0, 0, 0 },
	{ 2, 0, 0, 0, 216, 1839000, 89, 118, 0, 0, 0 },
	{ 3, 0, 0, 0, 216, 2435000, 48, 64, 0, 0, 0 },
	{ 4, 0, 0, 0, 216, 3343000, 192, 192, 0, 0, 0 },
	{ 5, 0, 0, 0, 216, 3961000, 177, 177, 0, 0, 0 },
	{ 6, 0, 0, 0, 216, 4238000, 192, 256, 0, 0, 0 },
	{ 7, 0, 0, 0, 216, 6691000, 192, 256, 0, 0, 0  }
};

/* manifest/updwnmix.h */

#include <updwnmix_config.h>
struct module_manifest updwnmix = {
	.name = "UPDWMIX",
	.affinity_mask = 0x1,
	.instance_count = 15,
	.domain_types = 0,
	.load_type = 0,
	.module_type = 5,
	.auto_start = 0,
	.mod_cfg = updwnmix_config
}


/* manifest.h */
/* includeDirectory = manifest/<platform>/ */
/* includeDirectory = manifest/ */
#include <down_mix.h>
#include <copier.h>
#include <up_down_mix.h>

struct module_manifest *modules[] = {
	&copier, &down_mix, &up_down_mix
}

@lgirdwood
Copy link
Member

@softwarecki its going to be less effort to keep the toml and pre-process it. i.e. we dont have to add any new tooling or logic.

  1. toml is split per module, platform and feature (like cse data).
  2. toml can include other toml, C headers and Kconfig.
  3. pre-processing means any one can update (we dont need Cadence CC to change extended manifest)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants