thesofproject
ams: Add initial AMS implementation
Add Asynchronous Messaging Service. This is used to communicate between modules in ACE.
Might still need some tinkering: verified single-core scenario and routing logic, though multi core behavior was not tested in-depth.
Signed-off-by: Krzysztof Frydryk [email protected]
Add Asynchronous Messaging Service. This is used to communicate between modules in ACE.
Can you provide examples of what this would be used for?
@plbossart AMS is basically notifications on steroids. It allows broadcasting notifications from one producer to multiple consumers, as well as targeting single consumers. Producers/consumers can be registered runtime. It should also allow for unified notifications to be sent to host through host gateway (not yet implemented). AFAIK for ACE platforms it is desired to send all module events through asynchronous messages.
@plbossart AMS is basically notifications on steroids. It allows broadcasting notifications from one producer to multiple consumers, as well as targeting single consumers. Producers/consumers can be registered runtime. It should also allow for unified notifications to be sent to host through host gateway (not yet implemented). AFAIK for ACE platforms it is desired to send all module events through asynchronous messages.
Which of these can the current notifier not do?
@lyakh AMS is a required part of ACE feature parity. It needs to be compatible with loadable modules and whatever is happening behind the host gateway.
@lgirdwood Async messages between different cores are sent through IDC/IPC. I don't think we have to care about what's behind the sof abstraction. As long as we can send a message and add case to handle ams idc we should be ok. IPC part is not yet implemented, though should be pretty similar.
@lgirdwood Async messages between different cores are sent through IDC/IPC. I don't think we have to care about what's behind the sof abstraction. As long as we can send a message and add case to handle ams idc we should be ok. IPC part is not yet implemented, though should be pretty similar.
ok, as long as we are using the Zephyr interface for IDC/IPC then I'm good.
@plbossart AMS is basically notifications on steroids. It allows broadcasting notifications from one producer to multiple consumers, as well as targeting single consumers. Producers/consumers can be registered runtime. It should also allow for unified notifications to be sent to host through host gateway (not yet implemented). AFAIK for ACE platforms it is desired to send all module events through asynchronous messages.
Which of these can the current notifier not do?
IIUC, AMS can also message host. Yeah, we cant have 2 methods of messaging between components or to host upstream. We need to update the notifier users to AMS AND IPC events users prior to v2.5. i.e. we merge those 2 existing APIs into AMS.
Now you can also see sparse complaints about address space violations https://github.com/thesofproject/sof/actions/runs/3282088398/jobs/5405006166#step:4:5259 :
/zep_workspace/sof/src/lib/ams.c:60:38: warning: incorrect type in argument 1 (different address spaces)
/zep_workspace/sof/src/lib/ams.c:60:38: expected void const *m1
/zep_workspace/sof/src/lib/ams.c:60:38: got unsigned char __cache *
/zep_workspace/sof/src/lib/ams.c:69:53: warning: incorrect type in argument 1 (different address spaces)
/zep_workspace/sof/src/lib/ams.c:69:53: expected void *dest
/zep_workspace/sof/src/lib/ams.c:69:53: got unsigned char __cache *
/zep_workspace/sof/src/lib/ams.c:109:5: warning: symbol 'ams_find_uuid_index_by_msg_type_id' was not declared. Should it be static?
/zep_workspace/sof/src/lib/ams.c:285:57: warning: incorrect type in argument 1 (different address spaces)
/zep_workspace/sof/src/lib/ams.c:285:57: expected void *dest
/zep_workspace/sof/src/lib/ams.c:285:57: got unsigned char __cache *
/zep_workspace/sof/src/lib/ams.c:535:63: warning: incorrect type in argument 3 (different address spaces)
/zep_workspace/sof/src/lib/ams.c:535:63: expected void const *src
/zep_workspace/sof/src/lib/ams.c:535:63: got unsigned char [usertype] __cache ( * )[16]
/zep_workspace/sof/src/lib/ams.c:539:33: warning: dubious: x & !y
/zep_workspace/sof/src/lib/ams.c:539:33: warning: Variable length array is used.
/zep_workspace/sof/src/lib/ams.c:539:33: warning: Variable length array is used.
/zep_workspace/sof/src/lib/ams.c:582:63: warning: incorrect type in argument 3 (different address spaces)
/zep_workspace/sof/src/lib/ams.c:582:63: expected void const *src
/zep_workspace/sof/src/lib/ams.c:582:63: got unsigned char [usertype] __cache ( * )[16]
BTW, cache_to_uncache() and uncache_to_cache() are also wrong on MTL:
/zep_workspace/sof/zephyr/../src/include/sof/coherent.h:346:54: warning: incorrect type in initializer (different address spaces)
/zep_workspace/sof/zephyr/../src/include/sof/coherent.h:346:54: expected struct coherent __cache *cc
/zep_workspace/sof/zephyr/../src/include/sof/coherent.h:346:54: got struct coherent *
/zep_workspace/sof/zephyr/../src/include/sof/coherent.h:361:39: warning: incorrect type in initializer (different address spaces)
/zep_workspace/sof/zephyr/../src/include/sof/coherent.h:361:39: expected struct coherent *uc
/zep_workspace/sof/zephyr/../src/include/sof/coherent.h:361:39: got struct coherent __cache *
should be fixed. Please, check cAVS
Further work on asynchronous messages is being halted atm. Secondary cores should be enabled on mtl soon and work will be resumed than.
I'm resurrecting this PR. Several changes were made, AMS transfers on same core and between primary/secondary cores were tested and seem to be working. AFAIK AMS tasks should be edf, but currently on MTL only ll tasks can be run on secondary cores - hence they're used at the moment. I'll add doxygen docs to this PR soon, as for @lgirdwood request.
Btw, @kfrydryx I didn't see this linked anywhere yet but we do have this https://thesofproject.github.io/latest/architectures/firmware/sof-zephyr/mpp_layer/async_messaging.html . Probably should be as a link in the pull request description. I think this would help to understand context. FYI @plbossart @lyakh @dbaluta who have asked about introduction details. (And no, I didn't know we had such documentation already up at sof-docs either )
Btw, @kfrydryx I didn't see this linked anywhere yet but we do have this https://thesofproject.github.io/latest/architectures/firmware/sof-zephyr/mpp_layer/async_messaging.html . Probably should be as a link in the pull request description. I think this would help to understand context. FYI @plbossart @lyakh @dbaluta who have asked about introduction details. (And no, I didn't know we had such documentation already up at sof-docs either )
Thanks, @kv2019i that helps. @kfrydryx how about just using semaphores and not messing around with IDC? We are running on top of a rather advanced multi-threading multi-CPU capable OS, why not use it. Can we register consumers not with callbacks but with a semaphore, so that when producers signal consumers, they just signal those semaphores thus making it truly asynchronous. BTW, Zephyr even has sockets, maybe just use those? They even have socketpairs. Maybe we can really use an existing tested and verified API instead of adding another one. They also have kernel/mailbox.c and they have a zbus for message passing... And possibly more IPC APIs.
@kfrydryx @kv2019i @lyakh The documentation [1] states this "The service exposes an external interface to the host". This seems like an ABI change, what exactly is exposed to the host driver and what can be configured?
[1] https://thesofproject.github.io/latest/architectures/firmware/sof-zephyr/mpp_layer/async_messaging.html .
@plbossart This will be coming later in the form of Host Async Message Gateway. That's basically a module responsible for forwarding Asynchronous Messages to the host and the other way round.
Lets make sure our direction keeps the external API for compatibility (new APIs could be allowed) and integrate to backends in Zephyr (to save memory and reduce support effort),
Rebased due to merge conflicts. PR with usage is in draft: https://github.com/thesofproject/sof/pull/7158
@plbossart, @lyakh please unblock the PR or list the technical issues that still need to be addressed before merge. As agreed, the improved AMS design can be implemented incrementally over time base on the proposed solution.
This PR still feels rushed and could use a number of review cycles to clearly explain some of its details.
@plbossart this PR is in review since September 2022, @kfrydryx is answering questions for months now. If there is still something unclear I am open to have a call and clarify the details. There was also architecture documentation chapter added to sof-docs.
I am not not going to reject but not going to approve either. This is not the way a publicly-visible PR should be handled.
I would never accept a PR like this if it was at the kernel level.
That is why I asked for a list of technical review comments that has not been yet addressed according to you. I want to have a clear line of sight what we are missing to move forward.
@plbossart, @lyakh please unblock the PR or list the technical issues that still need to be addressed before merge. As agreed, the improved AMS design can be implemented incrementally over time base on the proposed solution.
@mwasko I've dismissed my request for change. I still don't think this is a good addition to the SOF code base. Both because of the implementation itself and of the chosen approach like the use of IDC. For that reason, that I think it should be rewritten on top of proper RTOS APIs I'd rather not spend more time reviewing this version.