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

[DNM][POC] asoc: sof: add a hard-coded array of loadable extensions #5156

Draft
wants to merge 1 commit into
base: topic/sof-dev
Choose a base branch
from

Conversation

lyakh
Copy link

@lyakh lyakh commented Aug 23, 2024

The driver currently loads drivers that are missing in the firmware base image but are present in the topology. It might also be necessary to load auxiliary loadable objects. Use a hard-coded array for that.

I'm not proposing to merge this approach, but we need something to load such auxiliary modules.

The driver currently loads drivers that are missing in the firmware
base image but are present in the topology. It might also be
necessary to load auxiliary loadable objects. Use a hard-coded
array for that.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
static const guid_t preload_uuid[] = {
GUID_INIT(0x93446E12, 0x1864, 0x4E04, 0xAF, 0xE0, 0x3B, 0x1D, 0x77, 0x8F, 0xFB, 0x79),
};
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what 'auxiliary loadable objects' are, but I don't think we can hard-code such GUIDs either. Those are likely vendor and implementation-specific, it's hard to imagine why they would need to be listed in the common parts of SOF.

Is it not possible to extend the topology to make references to such objects and load them in a more transparent way?

Also the commit title prefix should be ASoC: SOF:

Copy link
Author

Choose a reason for hiding this comment

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

This is just a PoC, not even an RFC. I just needed a way to load those dependency modules to test my firmware (SOF and Zephyr) code. @plbossart @ujfalusi Please suggest a suitable way for doing this, my preference so far would be dependencies in manifests. Something like

.module = {
    .name = "EQFIR",
    .uuid = {UUID_EQFIR},
    .n_dependencies = 1,
    .dependencies = {{UUID_FIR}}
    ...
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we are not going to hardcode UUIDs in kernel, it is not going to scale and just introduce unmanageable dependency mess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lyakh, we can only load libraries into firmware, a library can contain several modules and all of them is sent to firmware for loading.
You should be bundling the dependent modules together in a library package and the firmware will load each modules from that library.

This is already supported by kernel and afaik the firmware's library manager code also handles this.

Copy link
Author

Choose a reason for hiding this comment

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

No, we are not going to hardcode UUIDs in kernel, it is not going to scale and just introduce unmanageable dependency mess.

@ujfalusi correct, that's why I proposed to add dependencies in module manifests, i.e. in the firmware

Copy link
Author

Choose a reason for hiding this comment

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

@lyakh, we can only load libraries into firmware, a library can contain several modules and all of them is sent to firmware for loading. You should be bundling the dependent modules together in a library package and the firmware will load each modules from that library.

This is already supported by kernel and afaik the firmware's library manager code also handles this.

@ujfalusi yes, this is also a possibility, but I like it less because:

  1. it's currently unsupported by LLEXT modules. It can be added, but so far we've used a simpler 1 library -> 1 module -> 1 ELF object scheme. Changing that would involve adding complexity to cmake, Python scripts, rimage, llext manager...
  2. what do you do if multiple modules depend on the same auxiliary ones? Include them in multiple libraries? Then you have to send them all via IPC (increased IPC traffic) and first store them in DSP IMR memory, until you figure out, that you already have some of them, then what? Keep those copies, wasting memory or try to delete them, freeing parts of that memory (wouldn't be easy or elegant)?

Copy link
Author

Choose a reason for hiding this comment

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

@lyakh, before adding new features to firmware and kernel, can we at least try to use the already implemented and standard way of handling multiple modules per library?

it is NOT implemented for LLEXT

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lyakh, before adding new features to firmware and kernel, can we at least try to use the already implemented and standard way of handling multiple modules per library?

it is NOT implemented for LLEXT

The module dependency handling is NOT implemented in kernel (nor there is any design document to follow on it) ;)

Copy link
Author

Choose a reason for hiding this comment

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

The module dependency handling is NOT implemented in kernel (nor there is any design document to follow on it) ;)

@ujfalusi exactly, so we have a choice - either adding library loading support to LLEXT with the drawbacks that I've described above, or we add dependency loading support to the SOF kernel driver. Also note, that the multi-module library API wasn't designed to handle multiple copies of modules (AFAIK), in fact it might even check and complain / bail out

Copy link
Member

Choose a reason for hiding this comment

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

May I suggest a one pager documentation on what an LLEXT module is, what it brings and how it should be managed?
It seems we are building layers upon layers without a clear vision of what the entire edifice will look like or what purpose it serves.

@ranj063
Copy link
Collaborator

ranj063 commented Aug 27, 2024

The driver currently loads drivers that are missing in the firmware base image but are present in the topology. It might also be necessary to load auxiliary loadable objects. Use a hard-coded array for that.

I'm not proposing to merge this approach, but we need something to load such auxiliary modules.

@lyakh what are these aux loadable objects exactly? Am I understanding this correct that you're talking about modules that depend on other modules? If so, why can't they also be specified in topology?

@lyakh
Copy link
Author

lyakh commented Aug 28, 2024

Replying to @plbossart and @ranj063 - one day I should finalise thesofproject/sof-docs#493 . In short - currently we can make Module Adapter instances as loadable LLEXT, with added dependency support we will also be able to dynamically load "auxiliary" code, e.g. Maths libraries like FIR, needed for eq_fir and one or two other component drivers, but if your topology contains such pipelines but they aren't in use ATM, that code can stay in IMR without being copied to SRAM.

@plbossart
Copy link
Member

plbossart commented Aug 28, 2024

"if your topology contains such pipelines but they aren't in use ATM, that code can stay in IMR without being copied to SRAM."

What's the benefit? Can this memory be reused for the heap then?

There's also the added risk/benefit question. New degrees of freedom are great until they start raising new validation problems with dynamic allocation failing randomly at some point... Exhibit A was the introduction of dynamic pipelines.

Edit: an additional question is the increased latency to enable a new pipeline that wasn't used before. This would move some of the latency from the boot or resume time to the pipeline start proper. Not necessarily good in all cases.

@lyakh
Copy link
Author

lyakh commented Aug 29, 2024

"if your topology contains such pipelines but they aren't in use ATM, that code can stay in IMR without being copied to SRAM."

What's the benefit? Can this memory be reused for the heap then?

Potentially - yes, ATM it can at least be reused for other modules. Additionally unused SRAM banks can be powered down.

There's also the added risk/benefit question. New degrees of freedom are great until they start raising new validation problems with dynamic allocation failing randomly at some point... Exhibit A was the introduction of dynamic pipelines.

Sure, in general more code / features usually means higher maintenance costs.

Edit: an additional question is the increased latency to enable a new pipeline that wasn't used before. This would move some of the latency from the boot or resume time to the pipeline start proper. Not necessarily good in all cases.

That's true as well. And even dynamic pipelines themselves, as you mentioned, add to this latency too. Maybe we should consider adding a feature to lock some pipelines in SRAM. But maybe the user-space should just keep them open in such cases.

@plbossart
Copy link
Member

Seems like we need a whole new level of infrastructure across firmware and host driver to power-managed banks, lock modules, deal with dependencies, manage fragmentation, etc.

This isn't a simple addition to the list of features... And this begs the question: what happens if we don't support this capability?

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

Successfully merging this pull request may close these issues.

4 participants