library manager: initial implementation by pjdobrowolski · Pull Request #5796 · thesofproject/sof

pjdobrowolski · 2022-05-10T10:46:35Z

Library manager is part of loading external libraries responsible of
handling data and placing it in proper order and places.

After receiving IPC4 load library command

reads manifest
after module_id choses module to load and verify it (WIP)
loads manifest
allocs propper memory size
transfer module from IMR to HPSRAM (WIP)

Signed-off-by: Stelter, Jaroslaw jaroslaw.stelter@intel.com
Signed-off-by: Dobrowolski, PawelX pawelx.dobrowolski@intel.com

sofci · 2022-05-10T10:48:04Z

Can one of the admins verify this patch?

reply test this please to run this test once

pjdobrowolski · 2022-05-10T10:51:44Z

test this please

gkbldcig · 2022-05-10T11:18:55Z

Can one of the admins verify this patch?

src/include/sof/library_manager.h

src/library_manager/library_manager.c

marc-hb · 2022-05-10T19:16:08Z

test this please

lgirdwood · 2022-05-11T13:39:00Z

@pjdobrowolski pls check your inbox, invite sent to autorun the CI.

lgirdwood

Some questions

How are we testing this today ? and how should we test in CI
Does this version support the dynamic linking or just the dynamic loading today ?

src/include/sof/library_manager.h

src/library_manager/library_manager.c

dbaluta · 2022-05-11T14:34:09Z

@pjdobrowolski @lgirdwood aside from having this quick merged what stops us from implementing this feature in zephyr?

marc-hb · 2022-05-11T17:14:45Z

Most checkpatch warnings at https://sof-ci.01.org/sofpr/PR5796/build327/checkpatch/ seem valid (and easy to fix)

jxstelter · 2022-05-12T07:19:10Z

Some questions

How are we testing this today ? and how should we test in CI

This PR is only small part responsible for loading library binary. More changes are required to have something testable.

Does this version support the dynamic linking or just the dynamic loading today ?

We support now dynamic loading of library containing 3rd party module binary. Currently available libraries in IADK (Windows close source FW) are not supporting dynamic linking, however it could be supported in the future.

marc-hb

More changes are required to have something testable.

Maybe that's why it's still a draft?

src/library_manager/Kconfig

src/library_manager/library_manager.c

src/include/sof/library_manager.h

src/library_manager/library_manager.c

src/library_manager/Kconfig

src/library_manager/lib_manager.c

src/library_manager/lib_notification.c

src/include/sof/lib_manager.h

src/library_manager/lib_manager.c

lgirdwood · 2022-08-17T13:43:28Z

@jxstelter fyi since @pjdobrowolski is out.

src/library_manager/lib_manager.c

ujfalusi

@pjdobrowolski, do you have any library I can try to load with my Linux side PR (thesofproject/linux#3826)?

src/library_manager/lib_manager.c

pjdobrowolski · 2022-08-29T12:02:30Z

@pjdobrowolski, do you have any library I can try to load with my Linux side PR (thesofproject/linux#3826)?

You can build one yourself using https://github.com/thesofproject/converged-sof-modules

FYI: I came back from 2 week long sick leave and you guys flooded me with changes which should be requested almost 2 months ago. @dbaluta @ujfalusi @lgirdwood @lyakh I understand that next few days I will spend on resolving that requests. And I thought that sinusitis was bad...

src/include/sof/lib_manager.h

dbaluta · 2022-08-29T12:53:05Z

FYI: I came back from 2 week long sick leave and you guys flooded me with changes which should be requested almost 2 months ago. @dbaluta @ujfalusi @lgirdwood @lyakh I understand that next few days I will spend on resolving that requests. And I thought that sinusitis was bad...

@pjdobrowolski don't worry you will get used with us ;). glad that you are better now.

In order to get your patches faster accepted open source community has some guidelines:

split your patches into simple logical units.
make your code clean. follow naming conventions. Run ./scripts/checkpatch.pl over your patch.
avoid having more then 3-4 level of indentation in the code.
when introducing new features in a series: please add the bugfixes first and the patches introducing new code later.

I will be on vacation until next week so you I'm off your back for a while :). But I'll be back.

lgirdwood · 2022-08-29T15:42:26Z

rerun CI, license server

License checkout failed: Cannot connect to license server system.
 The license server manager (lmgrd) has not been started yet,
 the wrong port@host or license file is being used, or the
 port or hostname in the license file has been changed.
Feature:       XTENSA_XCC_TIE
Server name:   fmylic7005.fm.intel.com
License path:  84300@xtensa01p.elic.intel.com:/srv/home/jenkins/xcc/install/tools/RG-2017.8-linux/XtensaTools/Tools/lic/license.dat:
FLEXnet Licensing error:-15,570.  System Error: 115 "Operation now in progress"
For further information, refer to the FLEXnet Licensing documentation,
available at "www.macrovision.com".
make[3]: *** [CMakeFiles/sof.dir/build.make:1248: CMakeFiles/sof.dir/src/math/sqrt_int16.c.o] Error 2
make[3]: *** Waiting for unfinished jobs....
License checkout failed: No such feature exists.

lgirdwood · 2022-08-29T15:42:33Z

SOFCI TEST

pjdobrowolski · 2022-08-30T06:11:35Z

@dbaluta Are there any guidelines like that for reviewers or just "hit and run"?

src/include/sof/lib_manager.h

lgirdwood · 2022-08-30T08:23:25Z

@pjdobrowolski can you force push again, I've just merged the build fix for IPC4 and newer CC. Thanks

lgirdwood · 2022-08-30T13:52:32Z

SOFCI TEST

src/library_manager/lib_notification.c

lyakh · 2022-08-31T09:00:11Z

src/library_manager/lib_notification.c

your .lib_notif_pool is somewhat... unusual. Usually you'd just have a list head in the higher level object (in ext_lib in this case) to which you then link elements as you allocate them. Instead you have a pointer to lib_notif_pool there and once you allocate the first element and attach it there, you then link further allocated elements to the first one. That makes the list head instance in that object both - a list head and a list element. list_empty() now means not 0 elements, but 1... All that can be made to work of course but just is a bit unusual and one has to remember about this when working with this code.

So what would you suggest. That might be refactored for sure but I think that main goal is to port that feature from legacy and integrate. Many mechanisms used here are borrowed from older firmware and for sure they can be improved. We can do that later if it isn't so wrong.

src/library_manager/lib_notification.c

src/include/ipc4/module.h

src/ipc/ipc4/helper.c

pjdobrowolski · 2022-08-31T10:00:56Z

SOFCI TEST

lgirdwood · 2022-08-31T10:40:23Z

@lrudyX @wszypelt can you check CI, not expecting this to fail where it did. Looks like a copy issue ? Thanks.

src/library_manager/lib_notification.c

src/library_manager/lib_manager.c

wszypelt · 2022-08-31T11:09:32Z

@lgirdwood yea...
TGL was disabled, I fixed it and ran the tests again.

Library manager is part of loading external libraries responsible of handling data and placing it in proper order and places. This feature is using module adapter API for loadable native and external libraries. After receiving IPC4 load library command - reads manifest - after module_id choses module to load and verify it (WIP) - loads manifest - allocs proper memory size - transfer module from external MEMORY to HPSRAM (WIP) Signed-off-by: Stelter, Jaroslaw <jaroslaw.stelter@intel.com> Signed-off-by: Dobrowolski, PawelX <pawelx.dobrowolski@intel.com>

Commit adds ipc which contains information need to load external libraries from HOST and required actions to make it possible using library manager functionality. Signed-off-by: Stelter, Jaroslaw <jaroslaw.stelter@intel.com> Signed-off-by: Dobrowolski, PawelX <pawelx.dobrowolski@intel.com>

Adding function responsible for loading library action after IPC4 message. Signed-off-by: Stelter, Jaroslaw <jaroslaw.stelter@intel.com> Signed-off-by: Dobrowolski, PawelX <pawelx.dobrowolski@intel.com>

Adding for ipc4_get_comp_drv check if module is loadable and for active library manager functionality register it dynamically. Signed-off-by: Stelter, Jaroslaw <jaroslaw.stelter@intel.com> Signed-off-by: Dobrowolski, PawelX <pawelx.dobrowolski@intel.com>

…red. System service functionality used by 3rd party modules expects that IPC data buffer will be filled with data during notification preparation. To avoid additional data copy, provide message buffer directly to module. This requires that ipc_msg_send() function will not fail if source data pointer will be equal to destination one. Signed-off-by: Stelter, Jaroslaw <jaroslaw.stelter@intel.com> Signed-off-by: Dobrowolski, PawelX <pawelx.dobrowolski@intel.com>

pjdobrowolski · 2022-08-31T11:46:42Z

rebased, resolved, ready to merge?

lgirdwood · 2022-08-31T12:44:30Z

rebased, resolved, ready to merge?

Good to merge as soon as CI finishes.

wszypelt · 2022-08-31T12:59:11Z

SOFCI TEST

lyakh · 2022-09-01T05:53:31Z

src/library_manager/lib_notification.c

+		msg = ipc_msg_init(header, SRAM_OUTBOX_SIZE);
+		if (!msg)
+			rfree(msg_pool_elem);
+			return NULL;


You need braces here

So reopen PR and will fix it.

dbaluta reviewed May 10, 2022

View reviewed changes

src/include/sof/library_manager.h Outdated Show resolved Hide resolved

dbaluta reviewed May 10, 2022

View reviewed changes

src/include/sof/library_manager.h Outdated Show resolved Hide resolved

dbaluta reviewed May 10, 2022

View reviewed changes

src/include/sof/library_manager.h Outdated Show resolved Hide resolved

dbaluta reviewed May 10, 2022

View reviewed changes

src/library_manager/library_manager.c Outdated Show resolved Hide resolved

pjdobrowolski force-pushed the library_manager branch from 5971bea to 24832db Compare May 11, 2022 13:41

lgirdwood reviewed May 11, 2022

View reviewed changes

pjdobrowolski force-pushed the library_manager branch 2 times, most recently from ea4d814 to 8865e18 Compare May 11, 2022 14:30

pjdobrowolski force-pushed the library_manager branch from 8865e18 to 2c72666 Compare May 11, 2022 14:37

jxstelter closed this May 12, 2022

pjdobrowolski reopened this May 12, 2022

marc-hb reviewed May 12, 2022

View reviewed changes

src/library_manager/Kconfig Outdated Show resolved Hide resolved

src/library_manager/library_manager.c Outdated Show resolved Hide resolved

dbaluta reviewed May 13, 2022

View reviewed changes

src/include/sof/library_manager.h Outdated Show resolved Hide resolved

src/include/sof/library_manager.h Outdated Show resolved Hide resolved

src/include/sof/library_manager.h Outdated Show resolved Hide resolved

src/library_manager/library_manager.c Outdated Show resolved Hide resolved

jxstelter requested a review from mwasko May 13, 2022 10:25

pjdobrowolski force-pushed the library_manager branch 5 times, most recently from ccec0d4 to e440d45 Compare May 17, 2022 13:05

ranj063 reviewed May 17, 2022

View reviewed changes

dbaluta reviewed May 18, 2022

View reviewed changes

pjdobrowolski force-pushed the library_manager branch from e440d45 to 377bee8 Compare May 18, 2022 12:32

lyakh reviewed Aug 29, 2022

View reviewed changes

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

ujfalusi reviewed Aug 29, 2022

View reviewed changes

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

src/library_manager/lib_manager.c Outdated Show resolved Hide resolved

ujfalusi reviewed Aug 29, 2022

View reviewed changes

src/include/sof/lib_manager.h Outdated Show resolved Hide resolved

ujfalusi reviewed Aug 29, 2022

View reviewed changes

src/include/sof/lib_manager.h Outdated Show resolved Hide resolved

ujfalusi reviewed Aug 30, 2022

View reviewed changes

src/include/sof/lib_manager.h Outdated Show resolved Hide resolved

lyakh reviewed Aug 31, 2022

View reviewed changes

marcinszkudlinski approved these changes Aug 31, 2022

View reviewed changes

lyakh reviewed Aug 31, 2022

View reviewed changes

lgirdwood approved these changes Aug 31, 2022

View reviewed changes

pjdobrowolski added 5 commits August 31, 2022 13:35

ipc4: library manager: handling load library function

e596280

Adding function responsible for loading library action after IPC4 message. Signed-off-by: Stelter, Jaroslaw <jaroslaw.stelter@intel.com> Signed-off-by: Dobrowolski, PawelX <pawelx.dobrowolski@intel.com>

lyakh reviewed Sep 1, 2022

View reviewed changes

marc-hb mentioned this pull request Sep 1, 2022

[SKIP CI] Minor docker-run.sh fixes #6238

Merged

Conversation

pjdobrowolski commented May 10, 2022

Uh oh!

sofci commented May 10, 2022

Uh oh!

pjdobrowolski commented May 10, 2022

Uh oh!

gkbldcig commented May 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-hb commented May 10, 2022

Uh oh!

lgirdwood commented May 11, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbaluta commented May 11, 2022

Uh oh!

marc-hb commented May 11, 2022

Uh oh!

jxstelter commented May 12, 2022 • edited by marc-hb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Aug 17, 2022

Uh oh!

Uh oh!

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pjdobrowolski commented Aug 29, 2022

Uh oh!

Uh oh!

Uh oh!

dbaluta commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Aug 29, 2022

Uh oh!

lgirdwood commented Aug 29, 2022

Uh oh!

jxstelter commented May 12, 2022 •

edited by marc-hb

Loading

dbaluta commented Aug 29, 2022 •

edited

Loading