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

[BUG] Platform-specific IPC4 code in platform-independent source files #7549

Closed
andyross opened this issue May 2, 2023 · 6 comments
Closed
Assignees
Labels
bug Something isn't working as expected
Milestone

Comments

@andyross
Copy link
Contributor

andyross commented May 2, 2023

Recent IPC4 changes have been introducing Intel DSP details into the generic SOF source layer.

The src/audio/base_fw.c file (despite the name, this is an IPC4-only source file) is implementing a protocol that seems directly related to Intel hardware details. In particular the handling in basefw_mem_state_info() is doing direct register reads to retrieve hardware state. It seems like there's a facility in place to structure the output of this function via "tuple" key/data pairs, but there's no documentation on the format it nor indirection API available for platforms.

Similarly the component driver module handling in ipc4/helper.c:ipc4_get_comp_drv() assumes that there is a Intel rimage-generated manifest format in memory from which to load modules. There is a "LIBRARY_MANAGER" feature that seems to be intended to replace this, I think? As of right now neither mechanism is in use in the main tree.

All hardware-specific features like this need to be provided behind an API that allows for other platforms to redefine it as needed, and ideally be defined/documented in a way that makes as few assumptions as possible about the underlying hardware.

See also comments and commit messages is PR #7531

@andyross andyross added the bug Something isn't working as expected label May 2, 2023
@paulstelian97
Copy link
Collaborator

Some documentation on what this required component is doing is also useful for porting other platforms (we want to switch to IPC4 at NXP, and in my quick look this was one of the places where I stopped, not knowing how to progress further). Why is it even required in the first place no matter what?

@lgirdwood lgirdwood added this to the v2.7 milestone Jul 4, 2023
@mengdonglin mengdonglin modified the milestones: v2.7, v2.8 Aug 16, 2023
@kv2019i
Copy link
Collaborator

kv2019i commented Oct 26, 2023

Tracking the basefw part in #8391

@kv2019i
Copy link
Collaborator

kv2019i commented Nov 20, 2023

No assigned person and one week to go before branch point, kicking to v2.9.

@kv2019i kv2019i modified the milestones: v2.8, v2.9 Nov 20, 2023
@lgirdwood
Copy link
Member

@andyross @cujomalainey we will tackle this in Q1, there is ongoing work already to split IPC logic out of modules as 1st step so we have module-common.c, module-ipc3.c and module-ipc4.c. We can then move this into other places so that common codebase is generic.

@kv2019i
Copy link
Collaborator

kv2019i commented Mar 4, 2024

Stable-v2.9 branched, this didn't make the cut, bumping to 2.10.

@kv2019i
Copy link
Collaborator

kv2019i commented Apr 24, 2024

With #9060 merged, we can close this. All vendor specific code has been now removed. Only exception is SOF_TELEMETRY, but that is behind separate ifdefs.

@kv2019i kv2019i closed this as completed Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

6 participants