ipc: increase msg max size #2216

abonislawski · 2019-12-17T09:14:49Z

It will increase max size for ipc msg, needed for #2212

Signed-off-by: Adrian Bonislawski adrian.bonislawski@linux.intel.com

lbetlej · 2019-12-17T12:52:47Z

@abonislawski I'd suggest to make more future proof solution by increasing max IPC size to 2 KB for example (depending on amount of space left unoccupied in a memory window in case of Intel HW), this way number of IPC operation for sending large payloads to DSP would be significantly reduced. A typical example is transport of a large parameter set for keyphrase detection algorithm.

lgirdwood · 2019-12-18T14:10:42Z

@dbaluta any objections on your end to 2kB ?

dbaluta · 2019-12-18T14:14:15Z

@lgirdwood fine for us. We actually don't have any hard limitation for messages passed through dsp box / host box as in our side we use a shared DRAM area which can host up to 8MB so far. We trimmed the memory windows to match the ones on Intel.

src/platform/cannonlake/include/platform/lib/memory.h

jajanusz · 2020-01-02T13:46:38Z

SOFCI TEST

abonislawski · 2020-01-02T17:31:01Z

@abonislawski I'd suggest to make more future proof solution by increasing max IPC size to 2 KB for example (depending on amount of space left unoccupied in a memory window in case of Intel HW), this way number of IPC operation for sending large payloads to DSP would be significantly reduced. A typical example is transport of a large parameter set for keyphrase detection algorithm.

@lbetlej I tried 2000 and 2048 but both results in problem with FW load, not sure why
It is used only for static allocations so it will waste a lot of memory if set way more than needed

lgirdwood · 2020-01-06T14:00:36Z

@abonislawski one thing to watch out for when changing section sizes is that we are also growing/shrinking other sections correctly (and updating their metadata for linker and allocator etc).
rimage and GNU ld have some built in checks to make sure no sections overlap, but it could be our linker sections are not aligned with some section macros ?

Increased ipc msg max size will allow to fit a bigger ipc, e.g. needed for mux channel map. Because of that it is also needed to increase HEAP_SYSTEM_M_SIZE to fit in memory bigger ipc msg size. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@linux.intel.com>

abonislawski · 2020-01-08T13:37:43Z

@abonislawski one thing to watch out for when changing section sizes is that we are also growing/shrinking other sections correctly (and updating their metadata for linker and allocator etc).
rimage and GNU ld have some built in checks to make sure no sections overlap, but it could be our linker sections are not aligned with some section macros ?

@lgirdwood I think there should be no problem with sections, after recent changes for most of them we only provide size of sections and linker will put them where he thinks.
As discussed with @tlauda no need for more changes to allow 2kB size, but please comment @tlauda

lgirdwood · 2020-01-08T14:31:44Z

Ok, looks good to me, is there a corresponding kernel PR ?

jajanusz · 2020-01-08T14:51:16Z

Please figure out if you want 768b or 2kb before merging this

jajanusz · 2020-01-16T12:18:12Z

@abonislawski Please let us know what's your plan for this PR, you can mark it as WIP if you are working on some dependency

abonislawski · 2020-01-17T11:57:07Z

@jajanusz it would be the best to merge this and #2212 because Emil is waiting so long with mux tests.
And later I will try to make this ipc allocations less static to allow full msg size, ok?

lgirdwood · 2020-01-17T14:02:16Z

@jajanusz @abonislawski are we just waiting for a corresponding kernel PR now ?

jajanusz · 2020-01-17T14:18:16Z

@abonislawski please fix CI issues and take care of kernel patch, then we can merge this

abonislawski · 2020-01-20T08:59:07Z

@lgirdwood @jajanusz kernel PR:
thesofproject/linux#1738

plbossart · 2020-01-22T13:33:06Z

@plbossart
So, if your question is do I have to add more ifs like those after merging this PR?, then answer is: NO.
Can we merge it then?

Well, 1 correction - these IPCs are not used AFAIK in driver, they exist in FW.
So if you use mux then, IPC for new FW will be bigger (beyond old limit), but if you don't use mux in driver yet (as we suspect) then it doesn't matter.

but we don't have a mux-specific IPC, do we? It should be the component ID and then the component-specific data. The IPC is only the transport - I hope.

abonislawski · 2020-01-22T13:34:01Z

Well, 1 correction - these IPCs are not used AFAIK in driver, they exist in FW.
So if you use mux then, IPC for new FW will be bigger (beyond old limit), but if you don't use mux in driver yet (as we suspect) then it doesn't matter.

Yeap, exactly and anyway in both cases this is not about this PR

abonislawski · 2020-01-22T13:36:09Z

but we don't have a mux-specific IPC, do we? It should be the component ID and then the component-specific data. The IPC is only the transport - I hope.

I think "component-specific-data" is what we are thinking about. Only this will change

plbossart · 2020-01-22T13:39:28Z

just for fun I looked up all the usages of this value in the kernel:

include/sound/sof/header.h:#define SOF_IPC_MSG_MAX_SIZE                 384
sound/soc/sof/ipc.c:    if (msg_bytes > SOF_IPC_MSG_MAX_SIZE ||
sound/soc/sof/ipc.c:        reply_bytes > SOF_IPC_MSG_MAX_SIZE)
sound/soc/sof/ipc.c:    sparams->pl_size = SOF_IPC_MSG_MAX_SIZE - sparams->hdr_bytes;
sound/soc/sof/ipc.c:    partdata = kzalloc(SOF_IPC_MSG_MAX_SIZE, GFP_KERNEL);
sound/soc/sof/ipc.c:    if (cdata->rhdr.hdr.size <= SOF_IPC_MSG_MAX_SIZE) {
sound/soc/sof/ipc.c:    msg->msg_data = devm_kzalloc(sdev->dev, SOF_IPC_MSG_MAX_SIZE,
sound/soc/sof/ipc.c:    msg->reply_data = devm_kzalloc(sdev->dev, SOF_IPC_MSG_MAX_SIZE,
sound/soc/sof/topology.c:       if (ipc_size > SOF_IPC_MSG_MAX_SIZE) {

It seems we are going to need to make this a variable that depends on the ABI level?

jajanusz · 2020-01-22T13:47:34Z

just for fun I looked up all the usages of this value in the kernel:

include/sound/sof/header.h:#define SOF_IPC_MSG_MAX_SIZE                 384
sound/soc/sof/ipc.c:    if (msg_bytes > SOF_IPC_MSG_MAX_SIZE ||
sound/soc/sof/ipc.c:        reply_bytes > SOF_IPC_MSG_MAX_SIZE)
sound/soc/sof/ipc.c:    sparams->pl_size = SOF_IPC_MSG_MAX_SIZE - sparams->hdr_bytes;
sound/soc/sof/ipc.c:    partdata = kzalloc(SOF_IPC_MSG_MAX_SIZE, GFP_KERNEL);
sound/soc/sof/ipc.c:    if (cdata->rhdr.hdr.size <= SOF_IPC_MSG_MAX_SIZE) {
sound/soc/sof/ipc.c:    msg->msg_data = devm_kzalloc(sdev->dev, SOF_IPC_MSG_MAX_SIZE,
sound/soc/sof/ipc.c:    msg->reply_data = devm_kzalloc(sdev->dev, SOF_IPC_MSG_MAX_SIZE,
sound/soc/sof/topology.c:       if (ipc_size > SOF_IPC_MSG_MAX_SIZE) {

It seems we are going to need to make this a variable that depends on the ABI level?

Do you want to add this conditional macro first? You can assume that it has higher size since MINOR 13

plbossart · 2020-01-22T14:07:46Z

just for fun I looked up all the usages of this value in the kernel:

include/sound/sof/header.h:#define SOF_IPC_MSG_MAX_SIZE                 384
sound/soc/sof/ipc.c:    if (msg_bytes > SOF_IPC_MSG_MAX_SIZE ||
sound/soc/sof/ipc.c:        reply_bytes > SOF_IPC_MSG_MAX_SIZE)
sound/soc/sof/ipc.c:    sparams->pl_size = SOF_IPC_MSG_MAX_SIZE - sparams->hdr_bytes;
sound/soc/sof/ipc.c:    partdata = kzalloc(SOF_IPC_MSG_MAX_SIZE, GFP_KERNEL);
sound/soc/sof/ipc.c:    if (cdata->rhdr.hdr.size <= SOF_IPC_MSG_MAX_SIZE) {
sound/soc/sof/ipc.c:    msg->msg_data = devm_kzalloc(sdev->dev, SOF_IPC_MSG_MAX_SIZE,
sound/soc/sof/ipc.c:    msg->reply_data = devm_kzalloc(sdev->dev, SOF_IPC_MSG_MAX_SIZE,
sound/soc/sof/topology.c:       if (ipc_size > SOF_IPC_MSG_MAX_SIZE) {

It seems we are going to need to make this a variable that depends on the ABI level?

Do you want to add this conditional macro first? You can assume that it has higher size since MINOR 13

I was thinking of a variable field in a structure, not sure if we need a complicated macro.

But one question: we've used 'large IPCs' before, where information is sent for 'process' in multiple smaller IPCs. @juimonen worked on this. Doesn't this apply to the MUX case?

Put differently, are we really going to need to update the IPC size everytime we've got a structure that doesn't fit in the current size? or should we break this in pieces and transport it in multiple passes?

abonislawski · 2020-01-22T14:28:07Z

Put differently, are we really going to need to update the IPC size everytime we've got a structure that doesn't fit in the current size? or should we break this in pieces and transport it in multiple passes?

No, as discussed in the first comments: in the near future 'ipc msg size' will be changed to maximum available along with ipc msg allocation changes in fw.

But one question: we've used 'large IPCs' before, where information is sent for 'process' in multiple smaller IPCs. @juimonen worked on this. Doesn't this apply to the MUX case?

I dont remember details but we decided to stay with the current approach.
@akloniex do you remember the reasons?

But anyway, ipc msg size is going to be increased to maximum available size to not waste resources and splitting mux configuration will not be needed

juimonen · 2020-01-22T15:33:59Z

If I recall correctly, mux config was then fitting the 1 payload. The config was then in topology as byte control data and If I recall correctly we had some issues of setting the default data. As I see we are only missing the firmware side support in demux for large data. You can see how it is done in the eq's. @singalsu can also comment as he was just hacking around the eq's.

I you want to have large data blob or just large number of parameters as part of basic widget (not its related control), I think we don't support that atm.

singalsu · 2020-01-22T15:39:00Z

@singalsu can also comment as he was just hacking around the eq's.

Yep, it works perfectly. Please see eq_fir.c code.

juimonen · 2020-01-22T17:03:40Z

and just my 2 cents to this:

If we are still using the binary blob for mux, we should honestly get rid of it and specify real visible values in topology/kernel/firmware for it. The binary blob was done as we we're in horribly hurry and understood that specifying it then would take ages. Anyway we should do it and only use binary blobs where they are proper, like eq transfer function for example (or if something needs to be hidden for some reason).
I don't think increasing the msg size really makes a big difference in ipc traffic, we are talking about 1-10-100 messages (with widget parameter setting) and our ipc should handle that without issues? Otherwise it is very crappy ipc...

jajanusz · 2020-01-23T10:35:00Z

We should keep logic as simple as possible. Multi-part IPCs require additional logic in components and should be used when there is no alternative. Increasing IPC msg size limit is good.

abonislawski · 2020-01-23T10:41:16Z

@juimonen @singalsu thanks for comments but regardless of mux we want to increase the ipc msg limit to maximum, 384 is a limit without any reasons.

Multi part IPC is a way to go if we will reach real hard limits

lgirdwood · 2020-01-23T12:00:36Z

src/include/ipc/header.h


 /** Maximum message size for mailbox Tx/Rx */
-#define SOF_IPC_MSG_MAX_SIZE			384
+#define SOF_IPC_MSG_MAX_SIZE			768


@abonislawski my understanding after recent discussions is that we are moving to 2kB.

@lgirdwood To move to 2kB we have to rework how msgs are allocated. @abonislawski will work on that, but for now we would like to use as much as we can without these changes.

@lgirdwood just like @jajanusz said, please merge

@lgirdwood To move to 2kB we have to rework how msgs are allocated. @abonislawski will work on that, but for now we would like to use as much as we can without these changes.

can we quantify the work needed to support 2KB IPC messages? If we can have ONE ABI change it's less work for me, and less confusion for everyone.

@plbossart hardly to say but atm Im busy with other tasks

@plbossart It's not 5-minutes-task. Msg allocation and handling code would need to be reworked and we have more urgent tasks now. We want to increase it to 768 atm, cos without it we stall another feature. I know it's inconvenient for you, but we can't help much with that now. We would greatly appreciate your help with approving this one.

Humm, what happens if I don't? The mux would be stalled, but what is the impact? Put differently, who needs the mux support from the kernel level in the coming weeks?

juimonen · 2020-01-23T13:45:00Z

@jajanusz @abonislawski yes thanks from my side no objection to increase the max size.

lbetlej

@abonislawski we have a combined @mmaka1 @plbossart @lbetlej proposal to make change that is scalable for the future:

Set IPC max size in FW to 4KB (cavs hw memory window size)
Report IPC max max size via FW ready to a driver
Driver uses value reported by FW

abonislawski · 2020-02-27T11:28:52Z

@plbossart @lbetlej @mmaka1 I will add reporting capability of IPC msg max size to extended manifest when #2427 merged

lgirdwood · 2020-02-27T12:03:54Z

@abonislawski I assume we will be changing to 4kB ?

abonislawski · 2020-02-27T20:02:39Z

@lgirdwood not now but it will not require another kernel change

lgirdwood

Can we resubmit with the larger size and let CI do it's thing.

kv2019i · 2020-09-04T07:31:59Z

@abonislawski ping? what's the status of this?

abonislawski · 2020-09-08T12:07:25Z

@kv2019i ipc size added to ext_manifest and merged in FW but still not used in kernel

lgirdwood · 2020-09-17T13:39:45Z

@abonislawski ping. Will move to v1.7

mwasko · 2021-01-25T16:00:16Z

Closing this PR as the related 2212 (closed - cancelled)

abonislawski requested review from a team, lbetlej, lgirdwood, mmaka1 and plbossart as code owners December 17, 2019 09:14

dbaluta reviewed Dec 18, 2019

View reviewed changes

src/platform/cannonlake/include/platform/lib/memory.h Outdated Show resolved Hide resolved

jajanusz approved these changes Jan 2, 2020

View reviewed changes

abonislawski force-pushed the ipc_msg_size branch from 7064979 to 904fd7b Compare January 2, 2020 16:21

abonislawski force-pushed the ipc_msg_size branch from 904fd7b to 65285e2 Compare January 8, 2020 12:48

abonislawski requested a review from tlauda as a code owner January 8, 2020 12:48

dbaluta approved these changes Jan 8, 2020

View reviewed changes

lgirdwood added the ABI ABI change involved label Jan 8, 2020

lgirdwood added this to the v1.5 milestone Jan 8, 2020

abonislawski mentioned this pull request Jan 9, 2020

mux: using channel map as mux configuration #2212

Closed

abonislawski mentioned this pull request Jan 21, 2020

ASoC: SOF: increase ipc msg max size thesofproject/linux#1738

Closed

lgirdwood requested changes Jan 23, 2020

View reviewed changes

lbetlej requested changes Jan 31, 2020

View reviewed changes

lgirdwood modified the milestones: v1.5, v1.6 Feb 20, 2020

lgirdwood reviewed Feb 27, 2020

View reviewed changes

lgirdwood modified the milestones: v1.6, v1.7 Sep 17, 2020

lgirdwood modified the milestones: v1.7, v1.8 Jan 25, 2021

mwasko closed this Jan 25, 2021

ipc: increase msg max size #2216

ipc: increase msg max size #2216

Uh oh!

Conversation

abonislawski commented Dec 17, 2019

Uh oh!

lbetlej commented Dec 17, 2019

Uh oh!

lgirdwood commented Dec 18, 2019

Uh oh!

dbaluta commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jajanusz commented Jan 2, 2020

Uh oh!

abonislawski commented Jan 2, 2020

Uh oh!

lgirdwood commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abonislawski commented Jan 8, 2020

Uh oh!

lgirdwood commented Jan 8, 2020

Uh oh!

jajanusz commented Jan 8, 2020

Uh oh!

jajanusz commented Jan 16, 2020

Uh oh!

abonislawski commented Jan 17, 2020

Uh oh!

lgirdwood commented Jan 17, 2020

Uh oh!

jajanusz commented Jan 17, 2020

Uh oh!

abonislawski commented Jan 20, 2020

Uh oh!

plbossart commented Jan 22, 2020

Uh oh!

abonislawski commented Jan 22, 2020

Uh oh!

abonislawski commented Jan 22, 2020

Uh oh!

plbossart commented Jan 22, 2020

Uh oh!

jajanusz commented Jan 22, 2020

Uh oh!

plbossart commented Jan 22, 2020

Uh oh!

abonislawski commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juimonen commented Jan 22, 2020

Uh oh!

singalsu commented Jan 22, 2020

Uh oh!

juimonen commented Jan 22, 2020

Uh oh!

jajanusz commented Jan 23, 2020

Uh oh!

abonislawski commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

jajanusz Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

abonislawski Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

abonislawski Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

jajanusz Jan 30, 2020

Choose a reason for hiding this comment

dbaluta commented Dec 18, 2019 •

edited

Loading

lgirdwood commented Jan 6, 2020 •

edited

Loading

abonislawski commented Jan 22, 2020 •

edited

Loading

abonislawski commented Jan 23, 2020 •

edited

Loading