Skip to content

ipc4: Simplify ipc response sending memory allocations #9777

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

Merged
kv2019i merged 2 commits into from
Jan 17, 2025
Merged

ipc4: Simplify ipc response sending memory allocations #9777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2987e09
ipc4: handler: Remove unnecessary response buffer allocation
softwarecki Jan 14, 2025
33e5dc0
ipc: Remove unnecessary cache manipulation
softwarecki Jan 14, 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
3 changes: 1 addition & 2 deletions src/include/sof/ipc/msg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ struct ipc_msg {
uint32_t header; /* specific to platform */
uint32_t extension; /* extension specific to platform */
uint32_t tx_size; /* payload size in bytes */
void *tx_data; /* pointer to payload data */
bool is_shared; /* the message is shared cross-core */
void *tx_data; /* pointer to payload data, must be in a non-cached memory */
struct list_item list;
};

Expand Down
9 changes: 0 additions & 9 deletions src/ipc/ipc-common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,15 +216,6 @@ void ipc_msg_send(struct ipc_msg *msg, void *data, bool high_priority)
msg->tx_data != data) {
ret = memcpy_s(msg->tx_data, msg->tx_size, data, msg->tx_size);
assert(!ret);
if (!cpu_is_primary(cpu_get_id())) {
/* Write back data to memory to maintain coherence between cores.
* The response was prepared on a secondary core but will be sent
* to the host from the primary core.
*/
dcache_writeback_region((__sparse_force void __sparse_cache *)msg->tx_data,
msg->tx_size);
msg->is_shared = true;
}
}

/*
Expand Down
58 changes: 8 additions & 50 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ struct ipc4_msg_data {
static struct ipc4_msg_data msg_data;

/* fw sends a fw ipc message to send the status of the last host ipc message */
static struct ipc_msg msg_reply = {0, 0, 0, 0, false, LIST_INIT(msg_reply.list)};
static struct ipc_msg msg_reply = {0, 0, 0, 0, LIST_INIT(msg_reply.list)};

static struct ipc_msg msg_notify = {0, 0, 0, 0, false, LIST_INIT(msg_notify.list)};
static struct ipc_msg msg_notify = {0, 0, 0, 0, LIST_INIT(msg_notify.list)};

#if CONFIG_LIBRARY
static inline struct ipc4_message_request *ipc4_get_message_request(void)
Expand Down Expand Up @@ -778,29 +778,14 @@ static int ipc4_process_ipcgtw_cmd(struct ipc4_message_request *ipc4)
uint32_t reply_size = 0;
int err;

/* NOTE: reply implementation is messy! First, reply payload is copied
* to ipc->comp_data buffer. Then, new buffer is allocated and assigned
* to msg_reply.tx_data. ipc_msg_send() copies payload from ipc->comp_data
* to msg_reply.tx_data. Then, ipc_prepare_to_send() copies payload from
* msg_reply.tx_data to memory window and frees msg_reply.tx_data. That is
* quite weird: seems one extra copying can be eliminated.
*/

err = copier_ipcgtw_process((const struct ipc4_ipcgtw_cmd *)ipc4, ipc->comp_data,
&reply_size);
/* reply size is returned in header extension dword */
msg_reply.extension = reply_size;

if (reply_size > 0) {
msg_reply.tx_data = rballoc(0, SOF_MEM_CAPS_RAM, reply_size);
if (msg_reply.tx_data) {
msg_reply.tx_size = reply_size;
} else {
ipc_cmd_err(&ipc_tr, "failed to allocate %u bytes for msg_reply.tx_data",
reply_size);
msg_reply.extension = 0;
return IPC4_OUT_OF_MEMORY;
}
msg_reply.tx_data = ipc->comp_data;
msg_reply.tx_size = reply_size;
}

return err < 0 ? IPC4_FAILURE : IPC4_SUCCESS;
Expand Down Expand Up @@ -1054,7 +1039,6 @@ static int ipc4_get_large_config_module_instance(struct ipc4_message_request *ip
const struct comp_driver *drv;
struct comp_dev *dev = NULL;
uint32_t data_offset;
void *response_buffer;
int ret = memcpy_s(&config, sizeof(config), ipc4, sizeof(*ipc4));

if (ret < 0)
Expand Down Expand Up @@ -1134,15 +1118,8 @@ static int ipc4_get_large_config_module_instance(struct ipc4_message_request *ip
return ret;

msg_reply.extension = reply.extension.dat;
response_buffer = rballoc(0, SOF_MEM_CAPS_RAM, data_offset);
if (response_buffer) {
msg_reply.tx_size = data_offset;
msg_reply.tx_data = response_buffer;
} else {
ipc_cmd_err(&ipc_tr, "error: failed to allocate tx_data");
ret = IPC4_OUT_OF_MEMORY;
}

msg_reply.tx_size = data_offset;
msg_reply.tx_data = data;
return ret;
}

Expand Down Expand Up @@ -1494,26 +1471,8 @@ struct ipc_cmd_hdr *ipc_prepare_to_send(const struct ipc_msg *msg)
msg_data.msg_out.pri = msg->header;
msg_data.msg_out.ext = msg->extension;

if (msg->tx_size) {
/* Invalidate cache to ensure we read the latest data from memory.
* The response was prepared on a secondary core but will be sent
* to the host from the primary core.
*/
if (msg->is_shared) {
dcache_invalidate_region((__sparse_force void __sparse_cache *)msg->tx_data,
msg->tx_size);
}

if (msg->tx_size)
mailbox_dspbox_write(0, (uint32_t *)msg->tx_data, msg->tx_size);
}

/* free memory for get config function */
if (msg == &msg_reply && msg_reply.tx_size > 0) {
rfree(msg_reply.tx_data);
msg_reply.tx_data = NULL;
msg_reply.tx_size = 0;
msg_reply.is_shared = false;
}

return &msg_data.msg_out;
}
Expand Down Expand Up @@ -1546,7 +1505,6 @@ void ipc_send_panic_notification(void)
{
msg_notify.header = SOF_IPC4_NOTIF_HEADER(SOF_IPC4_EXCEPTION_CAUGHT);
msg_notify.extension = cpu_get_id();
msg_notify.is_shared = !cpu_is_primary(cpu_get_id());
msg_notify.tx_size = 0;
msg_notify.tx_data = NULL;
list_init(&msg_notify.list);
Expand Down Expand Up @@ -1579,7 +1537,6 @@ void ipc_send_buffer_status_notify(void)
msg_notify.header = SOF_IPC4_NOTIF_HEADER(SOF_IPC4_NOTIFY_LOG_BUFFER_STATUS);
msg_notify.extension = 0;
msg_notify.tx_size = 0;
msg_notify.is_shared = false;

tr_dbg(&ipc_tr, "tx-notify\t: %#x|%#x", msg_notify.header, msg_notify.extension);

Expand Down Expand Up @@ -1607,6 +1564,7 @@ void ipc_cmd(struct ipc_cmd_hdr *_hdr)
/* no process on scheduled thread */
atomic_set(&msg_data.delayed_reply, 0);
msg_data.delayed_error = 0;
msg_reply.tx_data = NULL;
Copy link
Collaborator

@lyakh lyakh Jan 16, 2025

Choose a reason for hiding this comment

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

not particularly against this change, but just for clarity - this is just "cleaning up," right? Without it it would work just as well?

Copy link
Collaborator Author

@softwarecki softwarecki Jan 16, 2025

Choose a reason for hiding this comment

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

Yes, this is just cleaning, without this change everything should work. The code only checks the tx_size field.

msg_reply.tx_size = 0;
msg_reply.header = in->primary.dat;
msg_reply.extension = in->extension.dat;
Expand Down
1 change: 0 additions & 1 deletion src/library_manager/lib_notification.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ struct ipc_msg *lib_notif_msg_init(uint32_t header, uint32_t size)
/* Update header and size, since message handle can be reused */
msg->header = header;
msg->tx_size = size;
msg->is_shared = !cpu_is_primary(cpu_get_id());
return msg;
}

Expand Down
Loading