Skip to content

[RFC][WIP] large message ipc #609

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

Closed
juimonen wants to merge 2 commits into from
Closed

[RFC][WIP] large message ipc #609

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
653fe90
ASoC: SOF: ipc: enable large ipc messages
Jan 30, 2019
31abaa6
ASoC: SOF: ipc: test big data with controls
Jan 30, 2019
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
10 changes: 9 additions & 1 deletion include/sound/sof/header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
#define SOF_IPC_FW_READY SOF_GLB_TYPE(0x7U)
#define SOF_IPC_GLB_DAI_MSG SOF_GLB_TYPE(0x8U)
#define SOF_IPC_GLB_TRACE_MSG SOF_GLB_TYPE(0x9U)

#define SOF_IPC_GLB_LARGE SOF_GLB_TYPE(0xBU)
/*
Copy link
Collaborator

@dbaluta dbaluta Jan 31, 2019

Choose a reason for hiding this comment

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

Why not 0XAU?

Copy link
Author

@juimonen juimonen Jan 31, 2019

Choose a reason for hiding this comment

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

0XAU was already defined in sof fw side... maybe the kernel side header duplicate is not up to date

Copy link
Collaborator

@dbaluta dbaluta Jan 31, 2019

Choose a reason for hiding this comment

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

sof fw side... maybe the kernel side header duplicate is not up to date

Yes, I noticed that now. We should update the Linux side.

* DSP Command Message Types
*/
Expand Down Expand Up @@ -153,6 +153,14 @@ struct sof_ipc_compound_hdr {
uint32_t count; /**< count of 0 means end of compound sequence */
} __packed;

struct sof_ipc_large_hdr {
struct sof_ipc_cmd_hdr hdr;
uint32_t cmd;
uint32_t count;
uint32_t id;
unsigned char data[];
} __packed;

Copy link
Member

@plbossart plbossart Jan 31, 2019

Choose a reason for hiding this comment

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

could we instead use the concept of compound_ipc, but make sure that there is a notion of start/stop and continuation between commands.

Copy link
Author

@juimonen juimonen Jan 31, 2019

Choose a reason for hiding this comment

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

I don't understand what is the point of having large message implementation if they fit our ipc frame? For e.g. fir filter coeffs are byte blobs that can be really long. If they are going to be transferred to dsp they have to be chopped and framed somehow. So they are not compound messages but 1 message that doesn't fit the transfer size.

I'm not totally sure how the kernel scheduling goes here: we are sending max size chunks in our "normal" ipc way, so I guess the kernel has time to schedule something else, but I guess other IPC at the same time to dsp might be difficult. We could disable large messages if there's stream playing?

Copy link
Collaborator

@dbaluta dbaluta Jan 31, 2019

Choose a reason for hiding this comment

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

@plbossart @juimonen why can't we use a larger shared memory area? e.g reserve a chunk of system RAM for communication between DSP and CPU? Thus we can allocate few MB to allow large transfer with a single transfer.

Basically, get rid of the 4K mailbox.

/** @}*/

#endif
2 changes: 1 addition & 1 deletion include/uapi/sound/sof/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

/* SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 0
#define SOF_ABI_MINOR 1
#define SOF_ABI_PATCH 0

/* SOF ABI version number. Format within 32bit word is MMmmmppp */
Expand Down
60 changes: 56 additions & 4 deletions sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ static void ipc_log_header(struct device *dev, u8 *text, u32 cmd)
break;
case SOF_IPC_GLB_TRACE_MSG:
str = "GLB_TRACE_MSG"; break;
case SOF_IPC_GLB_LARGE:
str = "GLB_LARGE"; break;
default:
str = "unknown GLB command"; break;
}
Expand Down Expand Up @@ -241,10 +243,9 @@ static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,
return ret;
}

/* send IPC message from host to DSP */
int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
void *msg_data, size_t msg_bytes, void *reply_data,
size_t reply_bytes)
static int sof_ipc_tx_msg(struct snd_sof_ipc *ipc, u32 header,
void *msg_data, size_t msg_bytes, void *reply_data,
size_t reply_bytes)
{
struct snd_sof_dev *sdev = ipc->sdev;
struct snd_sof_ipc_msg *msg;
Expand Down Expand Up @@ -279,6 +280,57 @@ int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
/* now wait for completion */
return tx_wait_done(ipc, msg, reply_data);
}

/* send IPC message from host to DSP */
int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
void *msg_data, size_t msg_bytes, void *reply_data,
size_t reply_bytes)
{
struct sof_ipc_large_hdr *hdr;
uint32_t num_msg = 0;
uint32_t offset = 0;
uint32_t send_bytes = 0;
int ret = -1;
int size;
int i;

if (msg_bytes <= SOF_IPC_MSG_MAX_SIZE)
return sof_ipc_tx_msg(ipc, header, msg_data, msg_bytes,
reply_data, reply_bytes);

/* if bigger than max size, chop to smaller pieces */
dev_dbg(ipc->sdev->dev, "sending large ipc size %zu\n", msg_bytes);
size = SOF_IPC_MSG_MAX_SIZE - sizeof(struct sof_ipc_large_hdr);
num_msg = (msg_bytes + size - 1) / size;

hdr = (struct sof_ipc_large_hdr *)
kzalloc(SOF_IPC_MSG_MAX_SIZE, GFP_KERNEL);
if (!hdr)
return -ENOMEM;

hdr->hdr.cmd = SOF_IPC_GLB_LARGE;
hdr->cmd = header;
hdr->count = num_msg;

for (i = 0; i < num_msg; i++) {
hdr->id = i;
send_bytes = msg_bytes > size ? size : msg_bytes;
memcpy(hdr->data, msg_data + offset, send_bytes);
hdr->hdr.size = send_bytes;
ret = sof_ipc_tx_msg(ipc, SOF_IPC_GLB_LARGE, hdr, send_bytes +
sizeof(struct sof_ipc_large_hdr),
reply_data, reply_bytes);
if (ret < 0)
Copy link
Member

@plbossart plbossart Jan 31, 2019

Choose a reason for hiding this comment

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

why not use multiple regular IPC transfers, I don't get the need for a new type of transfers.

Copy link
Author

@juimonen juimonen Jan 31, 2019

Choose a reason for hiding this comment

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

so actually I renamed the old tx_message to tx_msg, so this is sending multiple "old style" max size ipc transfers (they are just framed inside new struct to be able to gather up on dsp side). Smaller than max transfer size messages are going just as before. This way I didn't have to touch topology.c.

break;

offset += size;
msg_bytes -= send_bytes;
}

kfree(hdr);

return ret;
}
EXPORT_SYMBOL(sof_ipc_tx_message);

/* send next IPC message in list */
Expand Down
6 changes: 5 additions & 1 deletion sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,11 +300,15 @@ static int sof_control_load_bytes(struct snd_soc_component *scomp,
struct sof_ipc_ctrl_data *cdata;
struct snd_soc_tplg_bytes_control *control =
(struct snd_soc_tplg_bytes_control *)hdr;
/*
const int max_size = SOF_IPC_MSG_MAX_SIZE -
sizeof(const struct sof_ipc_ctrl_data);
*/

const int max_size = SOF_IPC_MSG_MAX_SIZE * 10;

/* init the get/put bytes data */
scontrol->size = SOF_IPC_MSG_MAX_SIZE;
scontrol->size = max_size;
scontrol->control_data = kzalloc(scontrol->size, GFP_KERNEL);
cdata = scontrol->control_data;
if (!scontrol->control_data)
Expand Down