Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions app/boards/intel_adsp/Kconfig.defconfig
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ config DAI_INTEL_SSP
default y

config INTEL_ADSP_IPC
bool
Copy link
Collaborator

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?

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

default y
depends on IPC_SERVICE_BACKEND_INTEL_ADSP_HOST_IPC

config INTEL_ADSP_TIMER
default y
Expand Down
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace15_mtpm.conf
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,6 @@ CONFIG_LOG_BACKEND_SOF_PROBE=n
CONFIG_LOG_OUTPUT_FORMAT_LINUX_TIMESTAMP=y
CONFIG_WINSTREAM_CONSOLE=n
CONFIG_LOG_FLUSH_SLEEP_US=5000

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace20_lnl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,6 @@ CONFIG_LOG_BACKEND_ADSP=n
CONFIG_LOG_OUTPUT_FORMAT_LINUX_TIMESTAMP=y
CONFIG_WINSTREAM_CONSOLE=n
CONFIG_LOG_FLUSH_SLEEP_US=5000

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace30_ptl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,6 @@ CONFIG_LOG_BACKEND_ADSP=n
CONFIG_LOG_FLUSH_SLEEP_US=5000
CONFIG_LOG_OUTPUT_FORMAT_LINUX_TIMESTAMP=y
CONFIG_WINSTREAM_CONSOLE=n

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
4 changes: 3 additions & 1 deletion app/boards/intel_adsp_ace30_ptl_sim.conf
Original file line number Diff line number Diff line change
Expand Up @@ -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


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also remove 2 empty lines here

# Temporary disabled options
Expand All @@ -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
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace30_wcl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@ CONFIG_LOG_BACKEND_ADSP=n
CONFIG_LOG_FLUSH_SLEEP_US=5000
CONFIG_LOG_OUTPUT_FORMAT_LINUX_TIMESTAMP=y
CONFIG_WINSTREAM_CONSOLE=n

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace30_wcl_sim.conf
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,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
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace40_nvl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,6 @@ CONFIG_PM_DEVICE_RUNTIME_ASYNC=n
# Zephyr / logging
CONFIG_LOG_BACKEND_ADSP=n
CONFIG_WINSTREAM_CONSOLE=n

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_ace40_nvls.conf
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,6 @@ CONFIG_PM_DEVICE_RUNTIME_ASYNC=n
# Zephyr / logging
CONFIG_LOG_BACKEND_ADSP=n
CONFIG_WINSTREAM_CONSOLE=n

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_cavs25.conf
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,6 @@ CONFIG_LOG_FUNC_NAME_PREFIX_DBG=y
CONFIG_LOG_OUTPUT_FORMAT_LINUX_TIMESTAMP=y
CONFIG_LOG_TIMESTAMP_64BIT=y
CONFIG_WINSTREAM_CONSOLE=n

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
3 changes: 3 additions & 0 deletions app/boards/intel_adsp_cavs25_tgph.conf
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,6 @@ CONFIG_LOG_FUNC_NAME_PREFIX_DBG=y
CONFIG_LOG_OUTPUT_FORMAT_LINUX_TIMESTAMP=y
CONFIG_LOG_TIMESTAMP_64BIT=y
CONFIG_WINSTREAM_CONSOLE=n

# Use the new IPC message service in Zephyr
CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE=n
8 changes: 8 additions & 0 deletions src/include/sof/ipc/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -250,4 +250,12 @@ void ipc_complete_cmd(struct ipc *ipc);
/* GDB stub: should enter GDB after completing the IPC processing */
extern bool ipc_enter_gdb;

/**
* \brief Send emergency IPC message.
*
* @param[in] data IPC data to be sent.
* @param[in] ext_data Extended data to be sent.
*/
void ipc_send_message_emergency(uint32_t data, uint32_t ext_data);

#endif /* __SOF_DRIVERS_IPC_H__ */
125 changes: 102 additions & 23 deletions src/ipc/ipc-zephyr.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

#include <zephyr/kernel.h>

#include <intel_adsp_ipc.h>
#include <sof/ipc/common.h>

#include <sof/ipc/schedule.h>
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The global pointer wait_ack_sem is accessed in both ipc_receive_cb() (potentially in interrupt context) and ipc_platform_wait_ack() (thread context) without synchronization. This creates a race condition where the pointer could be set to NULL in ipc_platform_wait_ack() while ipc_receive_cb() is checking or using it. Consider using atomic operations or protecting access with a spinlock.

Copilot uses AI. Check for mistakes.

/**
* @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
Copy link

Copilot AI Nov 3, 2025

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_register_endpoint() is not checked. If endpoint registration fails, subsequent IPC operations will fail silently. Add error handling to check the return value and log an error or return an error code from ipc_ept_init().

Suggested change
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 uses AI. Check for mistakes.
}

static void ipc_complete(void)
{
ipc_service_send(&ipc_ept, NULL, INTEL_ADSP_IPC_SEND_DONE);
Copy link

Copilot AI Nov 3, 2025

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.

Suggested change
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);
}

Copilot uses AI. Check for mistakes.
}

static bool ipc_is_complete(void)
{
return ipc_service_send(&ipc_ept, NULL, INTEL_ADSP_IPC_SEND_IS_COMPLETE) == 0;
Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

}

static int ipc_send_message(uint32_t data, uint32_t ext_data)
{
struct intel_adsp_ipc_msg msg = {.data = data, .ext_data = ext_data};

return ipc_service_send(&ipc_ept, &msg, INTEL_ADSP_IPC_SEND_MSG);
}

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);
Comment on lines +152 to +156
Copy link

Copilot AI Nov 3, 2025

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.
}

#ifdef CONFIG_PM_DEVICE
Expand Down Expand Up @@ -159,8 +227,8 @@ static int ipc_device_resume_handler(const struct device *dev, void *arg)
ipc->task_mask = 0;
ipc->pm_prepare_D3 = false;

/* attach handlers */
intel_adsp_ipc_set_message_handler(INTEL_ADSP_IPC_HOST_DEV, message_handler, ipc);
/* initialize IPC endpoint */
ipc_ept_init(ipc);
Copy link
Collaborator

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

Copy link
Contributor Author

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.


/* schedule task */
#if CONFIG_TWB_IPC_TASK
Expand Down Expand Up @@ -254,7 +322,7 @@ enum task_state ipc_platform_do_cmd(struct ipc *ipc)
void ipc_platform_complete_cmd(struct ipc *ipc)
{
ARG_UNUSED(ipc);
intel_adsp_ipc_complete(INTEL_ADSP_IPC_HOST_DEV);
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

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.

ipc_complete();

#if CONFIG_DEBUG_IPC_COUNTERS
increment_ipc_processed_counter();
Expand All @@ -263,26 +331,26 @@ void ipc_platform_complete_cmd(struct ipc *ipc)

int ipc_platform_send_msg(const struct ipc_msg *msg)
{
if (!intel_adsp_ipc_is_complete(INTEL_ADSP_IPC_HOST_DEV))
if (!ipc_is_complete())
return -EBUSY;

/* prepare the message and copy to mailbox */
struct ipc_cmd_hdr *hdr = ipc_prepare_to_send(msg);

return intel_adsp_ipc_send_message(INTEL_ADSP_IPC_HOST_DEV, hdr->pri, hdr->ext);
return ipc_send_message(hdr->pri, hdr->ext);
}

void ipc_platform_send_msg_direct(const struct ipc_msg *msg)
{
/* prepare the message and copy to mailbox */
struct ipc_cmd_hdr *hdr = ipc_prepare_to_send(msg);

intel_adsp_ipc_send_message_emergency(INTEL_ADSP_IPC_HOST_DEV, hdr->pri, hdr->ext);
ipc_send_message_emergency(hdr->pri, hdr->ext);
}

int ipc_platform_poll_is_host_ready(void)
{
return intel_adsp_ipc_is_complete(INTEL_ADSP_IPC_HOST_DEV);
return ipc_is_complete();
}

int platform_ipc_init(struct ipc *ipc)
Expand All @@ -300,8 +368,9 @@ int platform_ipc_init(struct ipc *ipc)
#endif
/* configure interrupt - work is done internally by Zephyr API */

/* attach handlers */
intel_adsp_ipc_set_message_handler(INTEL_ADSP_IPC_HOST_DEV, message_handler, ipc);
/* initialize IPC endpoint */
ipc_ept_init(ipc);

#ifdef CONFIG_PM
intel_adsp_ipc_set_suspend_handler(INTEL_ADSP_IPC_HOST_DEV,
ipc_device_suspend_handler, ipc);
Expand All @@ -312,22 +381,32 @@ int platform_ipc_init(struct ipc *ipc)
return 0;
}

#ifdef CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE
static bool ipc_wait_complete(const struct device *dev, void *arg)
{
k_sem_give(arg);
return false;
}
#endif

void ipc_platform_wait_ack(struct ipc *ipc)
{
static struct k_sem ipc_wait_sem;

k_sem_init(&ipc_wait_sem, 0, 1);

#ifdef CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE
intel_adsp_ipc_set_done_handler(INTEL_ADSP_IPC_HOST_DEV, ipc_wait_complete, &ipc_wait_sem);
#else
wait_ack_sem = &ipc_wait_sem;
#endif

if (k_sem_take(&ipc_wait_sem, Z_TIMEOUT_MS(10)) == -EAGAIN)
tr_err(&ipc_tr, "Timeout waiting for host ack!");

#ifdef CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE
intel_adsp_ipc_set_done_handler(INTEL_ADSP_IPC_HOST_DEV, NULL, NULL);
#else
wait_ack_sem = NULL;
#endif
}
3 changes: 1 addition & 2 deletions src/platform/intel/ace/lib/watchdog.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ static void watchdog_primary_core_action_on_timeout(void)

/* Send Watchdog Timeout IPC notification */
ipc4_notification_watchdog_init(&notif, 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - we do have low and high priority messages !

Copy link
Collaborator

Choose a reason for hiding this comment

The 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)
Expand Down
Loading