ASoC: SOF: IPC4 message abstraction and API change by ujfalusi · Pull Request #3334 · thesofproject/linux

ujfalusi · 2021-12-14T12:03:03Z

Hi,

Create a container struct to encompass the IPC4 message (the header and the optional data part) so that we can simplify the API and make it possible later on to have generic ops among different IPC versions.

Note: I'm not sure about the new msg_bytes/reply_bytes definition for IPC4, I felt that passing 0 as msg_bytes when there is a message (there must be always) but we don't have the optional data, strange, but it can be justified, if it is preferred.

ujfalusi · 2021-12-14T12:29:37Z

+	 * The msg_total_size should be passed as msg_bytes for function expecting
+	 * a message size and underlaying code should take this into account.
+	 */
+	sdev->ipc->max_payload_size += sizeof(struct sof_ipc4_msg);


I have missed the setup of the reply placeholder, this should be added after line 227:

ipc4_msg = msg->reply_data; ipc4_msg->data_ptr = msg->reply_data + sizeof(struct sof_ipc4_msg);

So that the data_ptr points to the memory right after the header.

Or I could have two separate allocations for the message struct then the data section?

RanderWang

LGTM

ranj063 · 2021-12-14T18:39:55Z

+	 * The msg_total_size should be passed as msg_bytes for function expecting
+	 * a message size and underlaying code should take this into account.
+	 */
+	sdev->ipc->max_payload_size += sizeof(struct sof_ipc4_msg);


I dont understand this @ujfalusi . The max payload for me refers to the MAX size of the mailbox. Why would we include the header and extension in it?

This was exactly my concern.
I guess I can modify things that the msg_bytes is the optional data size which goes via the mailbox, but then the msg_bytes is kind of half backed as we still have the u64 header, which is a message by itself in some cases.

ujfalusi · 2021-12-15T12:18:11Z

Changes since v1:

the size of struct sof_ipc4_msg is not counted towards the msg/reply_bytes
Remove the header and extension members from struct snd_sof_ipc_msg
Use the new ipc4 encapsulation for message reception

ujfalusi · 2021-12-17T21:26:47Z

Changes since v2:

Remove the header parameter from sof_ipc_tx_message() and derivatives
Remove the header and extension members from struct snd_sof_ipc_msg

ranj063

@ujfalusi should these patches be fixups? the ipc4 stuff is not upstream yet. Maybe thats why I am not able to appreciate why we're doing this

ranj063 · 2021-12-18T00:25:41Z

 static int ipc4_tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,
 			     void *reply_data)
 {
+	struct sof_ipc4_msg *ipc4_msg = msg->reply_data;


@ujfalusi I'm not able to appreciate why we need to do this. Maybe I'm missing the point. There's struct sof_ipc_msg that we use to pass the IPC message to be sent. What is the purpose of the sof_ipc4_msg? we dont have a sof_ipc3_msg similar to sof_ipc4_msg isnt it? So why is ipc4 special?

@ranj063, all ipc3 messages start with struct sof_ipc_cmd_hdr followed by the payload, if any. When we receive the reply or notification then the header is behind of the msg->reply_data.
In IPC3 we pass the whole message, including the header via mailbox.

IPC4 on the other hand is a 'split' protocol in this sense. The header is not passed via the mailbox, it is written to registers and the mailbox is only used in case there is additional data for the message.

We could do an asymmetric handling of IPC4, but I don't think it helps the reader by using the struct sof_ipc4_msg for TX and spreading out the reply, notification into struct sof_ipc_msg

Users of the IPC interface should not be using the struct sof_ipc_msg as it is internal to the IPC 'subsystem' of SOF, we should have a simple interface of "send this message and give me the reply here" and on the notification side it should be that, "here, we have received this message".
Ideally only the low level and high level should know how to deal with the pointer, in the middle layer we just pass things.
I consider the ipc4_tx_wait_done() here a low(ish) level functionality as it does error checking, but it should be possible to abstract it away to only wait w/o knowing anything about the content.

Oh, I see now.
The RX for notification is handled completely differently than the reply in IPC3. The reply is copied to reply_data while the notification is not, the IPC3 RX function itself reads the message from the mailbox (first the header only, then the whole message when it knows the size).

Hrm, It might be better to rename the reply_data and reply_size to rx_data and rx_size and use it for both reply and notifications/events.
We are serializing things, so while a reply is handled it is guarantied that a notification will not change the rx_data.

That would make it a bit more well defined and we would save on kmalloc() for notifications as a bonus?

ujfalusi · 2022-01-03T10:22:51Z

Changes since v3:

Move the struct sof_ipc4_msg definition to global header (include/sound/sof/ipc4/header.h) so it can be used outside of the core (not via sof-priv.h)

ujfalusi · 2022-01-05T12:48:41Z

Changes since v4:

Fixed the tx error handling to print the tx message's primary and extension.

ujfalusi · 2022-01-07T15:35:33Z

Changes since v5:

Fixed the typos in commit messages pointed out by @ranj063

bardliao

LGTM

The message and reply size can be checked earlier to fail early in case the requested size can not be sent due to it's length. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The sof_ipc4_tx_message (and _unlocked) does not have support for async operations. There is no need to allocate a buffer and copy each message to it to be sent to the DSP, we can use the passed message data pointer directly. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The struct sof_ipc4_msg encapsulates an IPC4 message for easier handling. In IPC4 the header (u64) is treated differently compared to IPC3. IPC4 gives the option that the header can bypass the mailbox memory where only the optional data part of the message is sent. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

IPC4 related code must use the struct sof_ipc4_msg to pass the message to be sent to the firmware. Similarly on message reception, the platform code must make sure that the ipc4 message container is updated before calling the handler. For message sending the msg_data is mandatory and must point to the ipc4 message container while the msg_bytes is the size of the optional data part of the message (data_bytes == ipc4_msg->data_size). On message reception, platform code is responsible to update the ipc4 struct's header (primary and extension) before calling the handler. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The header parameter is not used anymore and now it can be dropped from the parameter list of tx_message(). Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Neither the header or the extension is used by the underlying code, they can be dropped from the snd_sof_ipc_msg struct. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

RanderWang · 2022-01-17T13:58:20Z

 		/* check for FW boot completion */
 		if (sdev->fw_state == SOF_FW_BOOT_IN_PROGRESS) {
-			err = sof_ops(sdev)->fw_ready(sdev, msg);
+			err = sof_ops(sdev)->fw_ready(sdev, ipc4_msg->primary);


Can you help to add debug log for default case to notify unsupported reply ? thanks!

Right, should I add a new patch on top or a new PR? We can only print the primary and extension for now, but we should have similar logging as with IPC3.

@RanderWang, something like this?
ujfalusi@41ebbc9

you can create a new PR if you like

RanderWang

LGTM

ujfalusi requested review from RanderWang, bardliao, kv2019i, lgirdwood, plbossart and ranj063 December 14, 2021 12:03

ujfalusi commented Dec 14, 2021

View reviewed changes

RanderWang reviewed Dec 14, 2021

View reviewed changes

ranj063 reviewed Dec 14, 2021

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/ipc4_msg_01 branch from dc6464d to 251f692 Compare December 15, 2021 12:15

ujfalusi force-pushed the peter/sof/pr/ipc4_msg_01 branch from 251f692 to f03d30a Compare December 17, 2021 21:24

ranj063 reviewed Dec 18, 2021

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/ipc4_msg_01 branch from f03d30a to ddf78b3 Compare January 3, 2022 10:21

ujfalusi force-pushed the peter/sof/pr/ipc4_msg_01 branch from ddf78b3 to 81316e8 Compare January 5, 2022 12:48

ranj063 reviewed Jan 5, 2022

View reviewed changes

Comment thread sound/soc/sof/ipc4.c Resolved

Comment thread sound/soc/sof/debug.c Resolved

ujfalusi force-pushed the peter/sof/pr/ipc4_msg_01 branch from 81316e8 to 206f032 Compare January 7, 2022 15:35

ranj063 previously approved these changes Jan 9, 2022

View reviewed changes

bardliao previously approved these changes Jan 10, 2022

View reviewed changes

ujfalusi marked this pull request as ready for review January 10, 2022 07:31

ujfalusi requested review from ajitkupandey, dbaluta, juimonen, keyonjie, kuanhsuncheng and libinyang as code owners January 10, 2022 07:31

ujfalusi requested review from lyakh and yaochunhung as code owners January 10, 2022 07:31

RanderWang previously approved these changes Jan 10, 2022

View reviewed changes

kuanhsuncheng previously approved these changes Jan 10, 2022

View reviewed changes

ujfalusi changed the title ~~[RFC] IPC4 message abstraction and API change~~ ASoC: SOF: IPC4 message abstraction and API change Jan 14, 2022

ujfalusi added 6 commits January 14, 2022 09:37

ASoC: SOF: ipc4: Check sizes early in sof_ipc4_tx_message_unlocked

e6ebda8

The message and reply size can be checked earlier to fail early in case the requested size can not be sent due to it's length. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: Drop 'header' parameter from tx_message() API

17ebb66

The header parameter is not used anymore and now it can be dropped from the parameter list of tx_message(). Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: Remove header and extension from struct snd_sof_ipc_msg

7326973

Neither the header or the extension is used by the underlying code, they can be dropped from the snd_sof_ipc_msg struct. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi dismissed stale reviews from kuanhsuncheng, RanderWang, bardliao, and ranj063 via 7326973 January 14, 2022 07:39

ujfalusi force-pushed the peter/sof/pr/ipc4_msg_01 branch from 206f032 to 7326973 Compare January 14, 2022 07:39

RanderWang reviewed Jan 17, 2022

View reviewed changes

RanderWang approved these changes Jan 17, 2022

View reviewed changes

plbossart approved these changes Jan 17, 2022

View reviewed changes

ranj063 approved these changes Jan 18, 2022

View reviewed changes

plbossart merged commit 4f79168 into thesofproject:topic/sof-dev Jan 20, 2022

marc-hb mentioned this pull request Feb 9, 2022

[BUG] logger "ABI" dictionary check ignores configuration and other changes thesofproject/sof#3890

Closed

ujfalusi deleted the peter/sof/pr/ipc4_msg_01 branch November 18, 2022 07:30

Conversation

ujfalusi commented Dec 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Dec 15, 2021

Uh oh!

ujfalusi commented Dec 17, 2021

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ujfalusi commented Jan 3, 2022

Uh oh!

ujfalusi commented Jan 5, 2022

Uh oh!

Uh oh!

Uh oh!

ujfalusi commented Jan 7, 2022

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants