-
Notifications
You must be signed in to change notification settings - Fork 349
ipc: rework Zephyr IPC interfaces #10089
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,6 @@ CONFIG_SOF_LOG_LEVEL_INF=n | |
| CONFIG_SOF_LOG_LEVEL_OFF=y | ||
| CONFIG_ZEPHYR_LOG=n | ||
|
|
||
| CONFIG_INTEL_ADSP_IPC=y | ||
|
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please also remove 2 empty lines here |
||
| # Temporary disabled options | ||
|
|
@@ -45,3 +44,6 @@ CONFIG_PM_PREWAKEUP_CONV_MODE_CEIL=y | |
| CONFIG_COMP_KPB=n | ||
|
|
||
| CONFIG_USERSPACE=y | ||
|
|
||
| # Use the new IPC message service in Zephyr | ||
| CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,7 +13,6 @@ | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #include <zephyr/kernel.h> | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #include <intel_adsp_ipc.h> | ||||||||||||||||||||||||||||||||||||||||
| #include <sof/ipc/common.h> | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #include <sof/ipc/schedule.h> | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -45,6 +44,11 @@ | |||||||||||||||||||||||||||||||||||||||
| #include <stddef.h> | ||||||||||||||||||||||||||||||||||||||||
| #include <stdint.h> | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #ifdef CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE | ||||||||||||||||||||||||||||||||||||||||
| #include <intel_adsp_ipc.h> | ||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||
| #include <zephyr/ipc/backends/intel_adsp_host_ipc.h> | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| SOF_DEFINE_REG_UUID(zipc_task); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| LOG_MODULE_DECLARE(ipc, CONFIG_SOF_LOG_LEVEL); | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -59,33 +63,97 @@ LOG_MODULE_DECLARE(ipc, CONFIG_SOF_LOG_LEVEL); | |||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| static uint32_t g_last_data, g_last_ext_data; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| struct k_sem *wait_ack_sem; | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||
| * @brief cAVS IPC Message Handler Callback function. | ||||||||||||||||||||||||||||||||||||||||
| * @brief cAVS IPC Message Received Callback function. | ||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||
| * See @ref (*intel_adsp_ipc_handler_t) for function signature description. | ||||||||||||||||||||||||||||||||||||||||
| * @return false so BUSY on the other side will not be cleared immediately but | ||||||||||||||||||||||||||||||||||||||||
| * @return -1 so BUSY on the other side will not be cleared immediately but | ||||||||||||||||||||||||||||||||||||||||
| * will remain set until message would have been processed by scheduled task, i.e. | ||||||||||||||||||||||||||||||||||||||||
| * until ipc_platform_complete_cmd() call. | ||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| static bool message_handler(const struct device *dev, void *arg, uint32_t data, uint32_t ext_data) | ||||||||||||||||||||||||||||||||||||||||
| static void ipc_receive_cb(const void *data, size_t cb_type, void *priv) | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| struct ipc *ipc = (struct ipc *)arg; | ||||||||||||||||||||||||||||||||||||||||
| struct intel_adsp_ipc_ept_priv_data *priv_data = priv; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| switch (cb_type) { | ||||||||||||||||||||||||||||||||||||||||
| case INTEL_ADSP_IPC_CB_MSG: { | ||||||||||||||||||||||||||||||||||||||||
| const struct intel_adsp_ipc_msg *msg = data; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| struct ipc *ipc = priv_data->priv; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| k_spinlock_key_t key; | ||||||||||||||||||||||||||||||||||||||||
| k_spinlock_key_t key; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| key = k_spin_lock(&ipc->lock); | ||||||||||||||||||||||||||||||||||||||||
| key = k_spin_lock(&ipc->lock); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| g_last_data = data; | ||||||||||||||||||||||||||||||||||||||||
| g_last_ext_data = ext_data; | ||||||||||||||||||||||||||||||||||||||||
| g_last_data = msg->data; | ||||||||||||||||||||||||||||||||||||||||
| g_last_ext_data = msg->ext_data; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #if CONFIG_DEBUG_IPC_COUNTERS | ||||||||||||||||||||||||||||||||||||||||
| increment_ipc_received_counter(); | ||||||||||||||||||||||||||||||||||||||||
| increment_ipc_received_counter(); | ||||||||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||||||||
| ipc_schedule_process(ipc); | ||||||||||||||||||||||||||||||||||||||||
| ipc_schedule_process(ipc); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| k_spin_unlock(&ipc->lock, key); | ||||||||||||||||||||||||||||||||||||||||
| k_spin_unlock(&ipc->lock, key); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||
| /* Not done so IPC is still "busy" */ | ||||||||||||||||||||||||||||||||||||||||
| priv_data->cb_ret = -EBUSY; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| case INTEL_ADSP_IPC_CB_DONE: | ||||||||||||||||||||||||||||||||||||||||
| if (wait_ack_sem) | ||||||||||||||||||||||||||||||||||||||||
| k_sem_give(wait_ack_sem); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| priv_data->cb_ret = INTEL_ADSP_IPC_CB_RET_OKAY; | ||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||
| priv_data->cb_ret = -EINVAL; | ||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| static struct ipc_ept ipc_ept; | ||||||||||||||||||||||||||||||||||||||||
| static struct intel_adsp_ipc_ept_priv_data ipc_ept_priv_data; | ||||||||||||||||||||||||||||||||||||||||
| static struct ipc_ept_cfg ipc_ept_cfg = { | ||||||||||||||||||||||||||||||||||||||||
| .name = "sof_ipc", | ||||||||||||||||||||||||||||||||||||||||
| .cb = { | ||||||||||||||||||||||||||||||||||||||||
| .received = ipc_receive_cb, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| .priv = &ipc_ept_priv_data, | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| static void ipc_ept_init(struct ipc *ipc) | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| const struct device *ipc_dev = INTEL_ADSP_IPC_HOST_DEV; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| ipc_ept_priv_data.priv = ipc; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| ipc_service_register_endpoint(ipc_dev, &ipc_ept, &ipc_ept_cfg); | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+126
to
+132
|
||||||||||||||||||||||||||||||||||||||||
| static void ipc_ept_init(struct ipc *ipc) | |
| { | |
| const struct device *ipc_dev = INTEL_ADSP_IPC_HOST_DEV; | |
| ipc_ept_priv_data.priv = ipc; | |
| ipc_service_register_endpoint(ipc_dev, &ipc_ept, &ipc_ept_cfg); | |
| static int ipc_ept_init(struct ipc *ipc) | |
| { | |
| const struct device *ipc_dev = INTEL_ADSP_IPC_HOST_DEV; | |
| ipc_ept_priv_data.priv = ipc; | |
| int ret = ipc_service_register_endpoint(ipc_dev, &ipc_ept, &ipc_ept_cfg); | |
| if (ret) { | |
| LOG_ERR("Failed to register IPC endpoint: %d", ret); | |
| return ret; | |
| } | |
| return 0; |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of ipc_service_send() is not checked in ipc_complete(). If sending the completion signal fails, the host will not be notified that IPC processing is complete, potentially causing the system to hang. Add error handling to check the return value and log an error.
| ipc_service_send(&ipc_ept, NULL, INTEL_ADSP_IPC_SEND_DONE); | |
| int ret = ipc_service_send(&ipc_ept, NULL, INTEL_ADSP_IPC_SEND_DONE); | |
| if (ret) { | |
| LOG_ERR("ipc_service_send(INTEL_ADSP_IPC_SEND_DONE) failed: %d", ret); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh... this looks strange - calling "send" to check the status...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh Yeah, some of this is a bit awkward, but let's not blame the messenger for all of it. This was not our preferred API in Zephyr, but this (the IPC service interface) is what we could get merged now and it's the Zephyr main API we need to ues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to introduce a new API with a query type call, but that new API was rejected in the arch meeting. So we are stuck with the existing IPC API which only has a send function.
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of ipc_service_send() is not checked in ipc_send_message_emergency(). In an emergency context (such as watchdog timeout), it's critical to know if the message was sent successfully. Consider checking the return value and logging an error, or changing the function signature to return an int to allow callers to handle failures.
| void ipc_send_message_emergency(uint32_t data, uint32_t ext_data) | |
| { | |
| struct intel_adsp_ipc_msg msg = {.data = data, .ext_data = ext_data}; | |
| ipc_service_send(&ipc_ept, &msg, INTEL_ADSP_IPC_SEND_MSG_EMERGENCY); | |
| int ipc_send_message_emergency(uint32_t data, uint32_t ext_data) | |
| { | |
| struct intel_adsp_ipc_msg msg = {.data = data, .ext_data = ext_data}; | |
| int ret = ipc_service_send(&ipc_ept, &msg, INTEL_ADSP_IPC_SEND_MSG_EMERGENCY); | |
| if (ret) { | |
| LOG_ERR("Emergency IPC send failed: %d", ret); | |
| } | |
| return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact, can we not just use Zephyr functions directly here? Would make reading easier by saving a level of indirection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function also sets a pointer inside the private data to the IPC struct. We can expand both in the two places calling ipc_ept_init(). It's that if we ever have to change the init sequence, we don't have to do it at two places... and to avoid changing one and not the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this break the "old" API? Shell it then be removed completely or do we need #ifdefs in these calls too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that once this is merged, the old API will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that once this is merged, the old API will be removed.
@dcpleung sure, understand, but isn't there a window when this is merged and the old is still there (and used)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh We have no need to keep this around. E.g. once we merge this, we can immediately submit a PR to remove the old API from Zephyr. Once that is merged, we can clean up SOF. This is really just a quick transition and I appreciate Daniel for doing the transition APIs, so this didn't require a flag day to take into SOF.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,8 +34,7 @@ static void watchdog_primary_core_action_on_timeout(void) | |
|
|
||
| /* Send Watchdog Timeout IPC notification */ | ||
| ipc4_notification_watchdog_init(¬if, cpu_get_id(), true); | ||
| intel_adsp_ipc_send_message_emergency(INTEL_ADSP_IPC_HOST_DEV, | ||
| notif.primary.dat, notif.extension.dat); | ||
| (void)ipc_send_message_emergency(notif.primary.dat, notif.extension.dat); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice - we do have low and high priority messages !
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't a new functionality. Its been around for years. But its not about low/high priority. Its a separate emergency ipc sending mechanism that ignores the current communication state and message queue. |
||
| } | ||
|
|
||
| static void watchdog_secondary_core_action_on_timeout(void) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? The type is already set where this option is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is superfluos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zephyr no longer has this option. So this is where this kconfig is defined, and thus needing a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in a .defconfig?.. Hm, not a blocker, but appears strange to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would eventually go away too. It was used to enable the IPC driver in Zephyr. Now that the driver is automatically included depending on device tree, so that is no longer the case. There are couple places in SOF using this kconfig and I am not sure about the impact of removing them. So keep it for now.