Skip to content

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

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

Closed
lyakh wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2164,6 +2164,14 @@ static int sof_set_widget_pipeline(struct snd_sof_dev *sdev, struct snd_sof_pipe
return 0;
}

#ifdef CONFIG_SND_SOC_SOF_IPC4
#include "ipc4-priv.h"

static const guid_t preload_uuid[] = {
GUID_INIT(0x93446E12, 0x1864, 0x4E04, 0xAF, 0xE0, 0x3B, 0x1D, 0x77, 0x8F, 0xFB, 0x79),
};
#endif

@plbossartplbossartAug 26, 2024

Copy link
Copy Markdown
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:

@lyakhlyakhAug 27, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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}}
...
};

@ujfalusiujfalusiAug 27, 2024

Copy link
Copy Markdown
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.

@ujfalusiujfalusiAug 27, 2024

Copy link
Copy Markdown
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.

@lyakhlyakhAug 27, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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

@lyakhlyakhAug 27, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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)?

@lyakhlyakhAug 27, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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

@ujfalusiujfalusiAug 27, 2024

Copy link
Copy Markdown
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) ;)

@lyakhlyakhAug 27, 2024

Copy link
Copy Markdown
CollaboratorAuthor

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

@plbossartplbossartAug 27, 2024

Copy link
Copy Markdown
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.


/* completion - called at completion of firmware loading */
static int sof_complete(struct snd_soc_component *scomp)
{
Expand All@@ -2172,6 +2180,7 @@ static int sof_complete(struct snd_soc_component *scomp)
const struct sof_ipc_tplg_widget_ops *widget_ops;
struct snd_sof_control *scontrol;
struct snd_sof_pipeline *spipe;
unsigned int i;
int ret;

widget_ops = tplg_ops ? tplg_ops->widget : NULL;
Expand All@@ -2187,6 +2196,13 @@ static int sof_complete(struct snd_soc_component *scomp)
}
}

#ifdef CONFIG_SND_SOC_SOF_IPC4
/* Pre-load libraries */
for (i = 0; i < ARRAY_SIZE(preload_uuid); i++)
if (!sof_ipc4_find_module_by_uuid(sdev, preload_uuid + i))
return -ENODEV;
#endif

/* set up the IPC structures for the pipeline widgets */
list_for_each_entry(spipe, &sdev->pipeline_list, list) {
struct snd_sof_widget *pipe_widget = spipe->pipe_widget;
Expand Down