Skip to content

ipc: intel_adsp: rework host IPC service backend and add critical send support #100682

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.

Sign up for GitHub

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

Open
tmleman wants to merge 5 commits into
base: main
Choose a base branch
from
+285 −241
Open

ipc: intel_adsp: rework host IPC service backend and add critical send support #100682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15159d9
ipc_service: Add ipc_service_send_critical function
tmleman Dec 4, 2025
173a46d
ipc: intel_adsp: simplify host IPC service backend
tmleman Dec 5, 2025
cb9fec7
intel_adsp: document host IPC backend data model and send hook
tmleman Dec 8, 2025
09b4d00
intel_adsp: move sync IPC wait to common helper
tmleman Dec 8, 2025
3cfbf17
tests: intel_adsp: update smoke IPC tests for new backend
tmleman Dec 12, 2025
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
58 changes: 4 additions & 54 deletions include/zephyr/ipc/backends/intel_adsp_host_ipc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,54 +14,6 @@

#include <zephyr/ipc/ipc_service_backend.h>

/** Enum on IPC send length argument to indicate IPC message type. */
enum intel_adsp_send_len {
/** Normal IPC message. */
INTEL_ADSP_IPC_SEND_MSG,

/** Synchronous IPC message. */
INTEL_ADSP_IPC_SEND_MSG_SYNC,

/** Emergency IPC message. */
INTEL_ADSP_IPC_SEND_MSG_EMERGENCY,

/** Send a DONE message. */
INTEL_ADSP_IPC_SEND_DONE,

/** Query backend to see if IPC is complete. */
INTEL_ADSP_IPC_SEND_IS_COMPLETE,
};

/** Enum on callback return values. */
enum intel_adsp_cb_ret {
/** Callback return to indicate no issue. Must be 0. */
INTEL_ADSP_IPC_CB_RET_OKAY = 0,

/** Callback return to signal needing external completion. */
INTEL_ADSP_IPC_CB_RET_EXT_COMPLETE,
};

/** Enum on callback length argument to indicate which triggers the callback. */
enum intel_adsp_cb_len {
/** Callback length to indicate this is an IPC message. */
INTEL_ADSP_IPC_CB_MSG,

/** Callback length to indicate this is a DONE message. */
INTEL_ADSP_IPC_CB_DONE,
};

/** Struct for IPC message descriptor. */
struct intel_adsp_ipc_msg {
/** Header specific to platform. */
uint32_t data;

/** Extension specific to platform. */
uint32_t ext_data;

/** Timeout for sending synchronuous message. */
k_timeout_t timeout;
};

#ifdef CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE

/**
Expand Down Expand Up @@ -89,6 +41,7 @@ struct intel_adsp_ipc_msg {
*/
typedef bool (*intel_adsp_ipc_handler_t)(const struct device *dev, void *arg, uint32_t data,
uint32_t ext_data);
#endif /* CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE */

/**
* @brief Intel ADSP IPC Message Complete Callback.
Expand All @@ -114,8 +67,6 @@ typedef bool (*intel_adsp_ipc_handler_t)(const struct device *dev, void *arg, ui
*/
typedef bool (*intel_adsp_ipc_done_t)(const struct device *dev, void *arg);

#endif /* CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE */

#ifdef CONFIG_PM_DEVICE
typedef int (*intel_adsp_ipc_resume_handler_t)(const struct device *dev, void *arg);

Expand Down Expand Up @@ -152,13 +103,13 @@ struct intel_adsp_ipc_data {

/** Argument for message handler callback. */
void *handler_arg;
#endif /* CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE */

/** Callback for done notification. */
intel_adsp_ipc_done_t done_notify;

/** Argument for done notification callback. */
void *done_arg;
#endif /* CONFIG_INTEL_ADSP_IPC_OLD_INTERFACE */

#ifdef CONFIG_PM_DEVICE
/** Pointer to resume handler. */
Expand All @@ -179,9 +130,8 @@ struct intel_adsp_ipc_data {
* Endpoint private data struct.
*/
struct intel_adsp_ipc_ept_priv_data {
/** Callback return value (enum intel_adsp_cb_ret). */
int cb_ret;

/* Message done flag. */
bool msg_done;
/** Pointer to additional private data. */
void *priv;
};
Expand Down
30 changes: 30 additions & 0 deletions include/zephyr/ipc/ipc_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,36 @@ int ipc_service_deregister_endpoint(struct ipc_ept *ept);
*/
int ipc_service_send(struct ipc_ept *ept, const void *data, size_t len);

/** @brief Send a high-priority message bypassing normal state checks.
*
* This function sends a high-priority message using the backend in a
* special fast path that skips normal state and busy checking. It is
* intended for critical system notifications such as crash reports,
* fatal errors, or emergency shutdown signals that must be delivered
* even when the IPC channel is otherwise considered busy or blocked.
*
* WARNING: This function should only be used for critical system notifications.
* Misuse can lead to data corruption or system instability. The backend must
* support this operation.
*
* @param[in] ept Registered endpoint by @ref ipc_service_register_endpoint.
* @param[in] data Pointer to the critical message buffer to send.
* @param[in] len Number of bytes to send.
*
* @retval -EIO when no backend is registered or send_critical hook is missing
* from backend.
* @retval -EINVAL when instance or endpoint is invalid.
* @retval -ENOENT when the endpoint is not registered with the instance.
* @retval -EBADMSG when the data is invalid (i.e. invalid data format,
* invalid length exceeds backend limits, ...).
* @retval -ENOTSUP when the operation is not supported by backend.
* @retval -ENOMEM when even critical path buffers are exhausted.
*
* @retval bytes number of bytes sent.
* @retval other errno codes depending on the implementation of the backend.
*/
int ipc_service_send_critical(struct ipc_ept *ept, const void *data, size_t len);

/** @brief Get the TX buffer size
*
* Get the maximal size of a buffer which can be obtained by @ref
Expand Down
23 changes: 23 additions & 0 deletions include/zephyr/ipc/ipc_service_backend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,29 @@ struct ipc_service_backend {
int (*send)(const struct device *instance, void *token,
const void *data, size_t len);

/** @brief Pointer to the function that sends critical messages bypassing flow control.
*
* This function sends high-priority messages directly, bypassing busy checks
* and normal queuing mechanisms. It should be used only for critical system
* notifications like crash reports or fatal errors.
*
* @param[in] instance Instance pointer.
* @param[in] token Backend-specific token.
* @param[in] data Pointer to the buffer to send.
* @param[in] len Number of bytes to send.
*
* @retval -EINVAL when instance is invalid.
* @retval -ENOENT when the endpoint is not registered with the instance.
* @retval -EBADMSG when the message is invalid.
* @retval -ENOTSUP when critical send is not supported.
* @retval -ENOMEM when even critical buffers are unavailable.
*
* @retval bytes number of bytes sent.
* @retval other errno codes depending on the implementation of the backend.
*/
int (*send_critical)(const struct device *instance, void *token,
const void *data, size_t len);

/** @brief Pointer to the function that will be used to register endpoints.
*
* @param[in] instance Instance to register the endpoint onto.
Expand Down
91 changes: 36 additions & 55 deletions soc/intel/intel_adsp/common/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,94 +43,75 @@ static void intel_adsp_ipc_receive_cb(const void *data, size_t len, void *priv)
struct intel_adsp_ipc_data *devdata = dev->data;
struct intel_adsp_ipc_ept_priv_data *priv_data =
(struct intel_adsp_ipc_ept_priv_data *)priv;
bool done = true;

if (len == INTEL_ADSP_IPC_CB_MSG) {
const struct intel_adsp_ipc_msg *msg = (const struct intel_adsp_ipc_msg *)data;

if (devdata->handle_message != NULL) {
done = devdata->handle_message(dev, devdata->handler_arg, msg->data,
msg->ext_data);
}

if (done) {
priv_data->cb_ret = INTEL_ADSP_IPC_CB_RET_OKAY;
} else {
priv_data->cb_ret = -EBADMSG;
}
} else if (len == INTEL_ADSP_IPC_CB_DONE) {
bool external_completion = false;

if (devdata->done_notify != NULL) {
external_completion = devdata->done_notify(dev, devdata->done_arg);
}

if (external_completion) {
priv_data->cb_ret = INTEL_ADSP_IPC_CB_RET_EXT_COMPLETE;
} else {
priv_data->cb_ret = INTEL_ADSP_IPC_CB_RET_OKAY;
}

__ASSERT(len == sizeof(uint32_t) * 2, "Unexpected IPC payload length: %zu", len);
__ASSERT(data != NULL, "IPC payload pointer is NULL");

const uint32_t *msg = (const uint32_t *)data;
Copy link
Contributor

@lyakh lyakh Dec 18, 2025

Choose a reason for hiding this comment

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

looks like the type-cast shouldn't be needed


if (devdata->handle_message != NULL) {
priv_data->msg_done = devdata->handle_message(dev, devdata->handler_arg, msg[0],
msg[1]);
}
}

void intel_adsp_ipc_complete(const struct device *dev)
{
int ret;

ret = ipc_service_send(&intel_adsp_ipc_ept, NULL, INTEL_ADSP_IPC_SEND_DONE);
ARG_UNUSED(dev);
int ret = ipc_service_release_rx_buffer(&intel_adsp_ipc_ept, NULL);

ARG_UNUSED(ret);
__ASSERT(ret == 0, "ipc_service_release_rx_buffer() failed: %d", ret);
}

bool intel_adsp_ipc_is_complete(const struct device *dev)
{
int ret;

ret = ipc_service_send(&intel_adsp_ipc_ept, NULL, INTEL_ADSP_IPC_SEND_IS_COMPLETE);

return ret == 0;
ARG_UNUSED(dev);
return ipc_service_get_tx_buffer_size(&intel_adsp_ipc_ept) > 0;
}

int intel_adsp_ipc_send_message(const struct device *dev, uint32_t data, uint32_t ext_data)
{
struct intel_adsp_ipc_msg msg = {.data = data, .ext_data = ext_data};
int ret;

ret = ipc_service_send(&intel_adsp_ipc_ept, &msg, INTEL_ADSP_IPC_SEND_MSG);
ARG_UNUSED(dev);
uint32_t msg[2] = {data, ext_data};
Copy link
Contributor

@kv2019i kv2019i Dec 9, 2025

Choose a reason for hiding this comment

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

I've been looking at how to bolt my #98862 on top of this, but seems this is doable. So if the full payload is sent using this interface, the msg would be 4x32bit, with inband header and an optional larger payload.

I do wonder if the "struct intel_adsp_ipc_msg" should be kept as the payload is somewhat special already with the 2x32bit inband message.

Copy link
Contributor Author

@tmleman tmleman Dec 10, 2025

Choose a reason for hiding this comment

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

I haven't seen the changes you're proposing before. I was considering a similar solution, but instead of a structure with a pointer, I saw it more as a buffer that has space for two dwords of message + payload. When SOF sends a message, the IPC backend would know how many bytes to copy to the mailbox based on the passed size.

Copy link
Contributor

@kv2019i kv2019i Dec 11, 2025

Choose a reason for hiding this comment

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

@tmleman Given SOF client code is using a scatter gather approach (the IPC payload is a tuple of upto two register words and a variable size payload), I think we need some structure for the payload. We will have variance betweeb different HW implementations (some transfer data inband in the doorbell registers, some don't), and between IPC versions (IPC3 has no concept of "ext_data").

I was thinking a four item tuple "first_word, second_word, payload_ptr, payload_len" would be enough to cover all variation we know about. Given we are mixing data and pointers, I think it would be better to use a struct to describe the object that is passed to IPC service send routine. I think this makes the code more readable both in SOF side, as well as the various Zephyr backends.

Copy link
Contributor

@kv2019i kv2019i Dec 17, 2025

Choose a reason for hiding this comment

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

@tmleman Any takes on this? Do you prefer to merge this first, and then follow-up with the payload extension? I'm ok with this as well, but would be good to have a rough consensus of how this can be extended. If this will be forever limited to "two word" payloads, not sure it's worth the effort tot move this to IPC service as we already know now this will only be useful for one type of hardware. And if that's the case, SOF code could just directly use the driver interface.

Copy link
Contributor Author

@tmleman tmleman Dec 17, 2025

Choose a reason for hiding this comment

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

@kv2019i Right now I don’t have a concrete design for the payload descriptor that would be passed to the backend via the IPC service.

The steps I had in mind are:

  • After these changes are merged, update the Zephyr version in SOF and run regression tests.
  • Introduce IPC service usage in SOF and replace the direct use of the Intel‑specific IPC driver.
  • Clean up the Intel ADSP IPC path by removing the old IPC driver so only the backend remains.
  • Once we are in that state, we can extend the backend to handle richer IPC payloads (data in mailboxes/shared memory).

Today, the data we pass through IPC service is intentionally backend‑specific, and each vendor can define its own layout. I’d like to hear from other vendors what they would need here. If we see enough commonality for SOF IPC, we can then agree on a shared struct or helper type on top of that.

Copy link
Contributor Author

@tmleman tmleman Dec 17, 2025

Choose a reason for hiding this comment

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

@kv2019i here are two PR in SOF to test those changes:

Copy link
Contributor

@kv2019i kv2019i Dec 17, 2025

Choose a reason for hiding this comment

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

@tmleman Ack thanks, I think we are aligned then. I'm good to take this in steps. We can use the size parameter to incrementally add features (if message length is more than 2 words, then client is using a newer interface).

W.r.t. this:

Today, the data we pass through IPC service is intentionally backend‑specific, and each vendor can define its own
layout. I’d like to hear from other vendors what they would need here.

I think can aim higher already now. If you look at IPC3 drivers in https://github.com/thesofproject/sof/blob/main/src/drivers/ , we have 4 four vendors using exactly the same layout. So I think it's really already aligned in SOF -- we have two layouts currently, IPC3 and IPC4 and the two are never compiled in same binary, so in practise there's no variation and the layout doesn't vary from hardware to another. This has the great benefit that we don't need add another abstract on client (SOF) side, but can directly use Zephyr IPC interface.

I mean if any folks from other vendors object, I'm happy to change my mind, but given this already worked once, I'd rather go towards this goal and keep code in SOF clean and take full benefit of the hardware abstraction Zephyr provides.


if (ret < 0) {
return ret;
}

return 0;
return ipc_service_send(&intel_adsp_ipc_ept, &msg, sizeof(msg));
}

/*
* This helper sends an IPC message and then waits synchronously for completion using the backend
* semaphore. It is currently only used by tests, SOF firmware does not rely on it.
*
* The long‑term plan is to either:
* - remove this helper entirely,
* - move the synchronous wait logic to the application layer (SOF), or
* - extend the generic IPC service API with an explicit synchronous send primitive.
*
* Until that decision is made, the function is kept here only to support existing test code and
* should not be used by new callers.
*/
int intel_adsp_ipc_send_message_sync(const struct device *dev, uint32_t data, uint32_t ext_data,
k_timeout_t timeout)
{
struct intel_adsp_ipc_msg msg = {
.data = data,
.ext_data = ext_data,
.timeout = timeout,
};
int ret;
uint32_t msg[2] = {data, ext_data};
struct intel_adsp_ipc_data *devdata = dev->data;

ret = ipc_service_send(&intel_adsp_ipc_ept, &msg, INTEL_ADSP_IPC_SEND_MSG_SYNC);
int ret = ipc_service_send(&intel_adsp_ipc_ept, &msg, sizeof(msg));

if (ret < 0) {
return ret;
k_sem_take(&devdata->sem, timeout);
Copy link
Contributor

@lyakh lyakh Dec 18, 2025

Choose a reason for hiding this comment

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

oh... so the only reason it can fail is because it's delayed? I see a few more ways how ipc_service_send() can fail. At least we should check a specific error code, since there's no other way to indicate asynchronous execution. Or maybe we can call the backend .send() function directly or maybe a new internal function like

int intel_adsp_ipc_send_message_sync(...)
{
    return intel_adsp_ipc_send_message_internal(..., timeout);
}
int intel_adsp_ipc_send_message(...)
{
    return intel_adsp_ipc_send_message_internal(..., 0);
}

also previously the check was ret == 0 which wasn't perfect but made a bit more sense: not an error, not a positive byte count that has already been sent, but 0, meaning asynchronous sending in progress.

}

return 0;
return ret;
}

void intel_adsp_ipc_send_message_emergency(const struct device *dev, uint32_t data,
uint32_t ext_data)
{
struct intel_adsp_ipc_msg msg = {.data = data, .ext_data = ext_data};
int ret;
uint32_t msg[2] = {data, ext_data};

ret = ipc_service_send(&intel_adsp_ipc_ept, &msg, INTEL_ADSP_IPC_SEND_MSG_EMERGENCY);
ret = ipc_service_send_critical(&intel_adsp_ipc_ept, &msg, sizeof(msg));

ARG_UNUSED(ret);
}
Expand Down
Loading
Loading