ipc: notifications refactor #2454

tlauda · 2020-02-27T11:33:02Z

This set of patches changes the way notifications are allocated and sent. More details in commit descriptions.

dbaluta · 2020-02-27T11:36:10Z

src/drivers/imx/ipc.c

@tlauda do we have a real usecase where checking IPC bits is not enough?

@dbaluta Yes, during specific power flows on cAVS DSP.

lgirdwood

Looks good, but my only thought is that an allocation failure may prevent an IPC message ? Should we pre-allocate the IPC messages (i.e embed into component private data). That way we are always guaranteed to succeed, I would also pre-alloc 8 messages on IPC core too so that we always copy client messages into IPC core messages (meaning we wont have collisions between host/IPC core and clients).

tlauda · 2020-02-27T12:24:58Z

Looks good, but my only thought is that an allocation failure may prevent an IPC message ? Should we pre-allocate the IPC messages (i.e embed into component private data). That way we are always guaranteed to succeed, I would also pre-alloc 8 messages on IPC core too so that we always copy client messages into IPC core messages (meaning we wont have collisions between host/IPC core and clients).

IPCs are allocated along with notification owner. There are no other allocations during the owner's lifetime. I think I don't get the second part. What is your definition of host/IPC core/client?

mmaka1 · 2020-02-27T12:28:19Z

@lgirdwood Pre-allocation on the IPC core would consume a lot of memory when max ipc size is aligned with the mailbox size since the core would need to alloc max for each one in the pool. While an originator can do precise allocation.

mmaka1 · 2020-02-27T12:36:51Z

src/ipc/handler.c

I am not sure why there is a dedicated function needed for each specific message. Looks like some generic ipc_prepare(...) called by a client could do the work?

mmaka1 · 2020-02-27T12:37:03Z

src/ipc/handler.c

This should not be part of ipc code I believe. How about:

position_update(...) { mailbox_stream_write(...) ipc_send(...) }

lgirdwood · 2020-02-27T12:51:48Z

@tlauda the thinking here with hostIPC core vs client is client changing any data during a copy to SRAM window (e.g. during client IRQ code).
@mmaka1 Sorry, to be clearer I was thinking more about the struct msg pre-allocation, rather than the whole mailbox.

tlauda · 2020-02-28T07:12:19Z

@lgirdwood With these set of patches we're totally changing the approach to owning and sending notifications. The idea was proposed by @mmaka1 as a potential solution for optimizing memory usage. Just to sum up:

There is no longer any common pool for IPC notifications. Every initiator allocates one notification message with exact size it requires.
Notifications can be send with high priority meaning we will try to send it right away and if the upstream channel is busy, we will add it to the beginning of message queue.
If it happens the notification from this particular originator is already in the queue, we just update the data and don't insert it again.
Everything is protected using lock.

paulstelian97 · 2020-02-28T08:27:46Z

@lgirdwood With these set of patches we're totally changing the approach to owning and sending notifications. The idea was proposed by @mmaka1 as a potential solution for optimizing memory usage. Just to sum up:

There is no longer any common pool for IPC notifications. Every initiator allocates one notification message with exact size it requires.

Notifications can be send with high priority meaning we will try to send it right away and if the upstream channel is busy, we will add it to the beginning of message queue.

If it happens the notification from this particular originator is already in the queue, we just update the data and don't insert it again.

Everything is protected using lock.

I have one issue: should there be support for canceling a notification that is in the queue whenever the data structures for a component are freed? We 100% don't want to send invalid notifications via a use-after-free do we?

tlauda · 2020-02-28T09:03:29Z

@lgirdwood With these set of patches we're totally changing the approach to owning and sending notifications. The idea was proposed by @mmaka1 as a potential solution for optimizing memory usage. Just to sum up:

There is no longer any common pool for IPC notifications. Every initiator allocates one notification message with exact size it requires.

Notifications can be send with high priority meaning we will try to send it right away and if the upstream channel is busy, we will add it to the beginning of message queue.

If it happens the notification from this particular originator is already in the queue, we just update the data and don't insert it again.

Everything is protected using lock.

I have one issue: should there be support for canceling a notification that is in the queue whenever the data structures for a component are freed? We 100% don't want to send invalid notifications via a use-after-free do we?

There is such support in ipc_msg_free().

lgirdwood · 2020-02-28T16:30:04Z

Jenkins CI is known issue, but we have UT failure in the internal CI.

Adds additional parameter to control whether upstream IPC channel is currently free or not. It's helpful in case for some reason checking IPC bits is not enough. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

Returns status -EBUSY, when IPC upstream channel is currently busy and notification cannot be sent. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

Adds improved implementation for sending IPC notifications to host. New function takes ipc_msg directly and adds it to message queue if message is not already there. Also critical notifications will be sent right away or added to the beginning of the queue if sending fails. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

Adds helper methods for initial building of IPC notifications. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

Adds helper methods for IPC notifications allocation and free. This way we can allocate only the size we need without preallocating the maximum size. Free operation automatically handles synchronization. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

Changes implementation of notifications allocation. Instead of using common empty list of messages let's switch to allocating message per source of notification. This way we won't have problem with memory and lack of empty messages and also we will be able to optimize memory. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

lgirdwood · 2020-03-02T10:29:27Z

SOFCI TEST

lgirdwood · 2020-03-02T10:29:53Z

Rerunning CI now that WHL DUT has been rebooted.

tlauda requested review from RanderWang, akloniex, bardliao, dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and xiulipan as code owners February 27, 2020 11:33

dbaluta reviewed Feb 27, 2020

View reviewed changes

tlauda force-pushed the topic/notifications-refactor branch from aa70f91 to 3192c4d Compare February 27, 2020 11:48

tlauda mentioned this pull request Feb 27, 2020

detect_test: add notification to the list if channel busy #2440

Closed

lgirdwood reviewed Feb 27, 2020

View reviewed changes

tlauda force-pushed the topic/notifications-refactor branch from 3192c4d to e0e16e1 Compare February 27, 2020 12:30

mmaka1 reviewed Feb 27, 2020

View reviewed changes

paulstelian97 approved these changes Feb 28, 2020

View reviewed changes

tlauda force-pushed the topic/notifications-refactor branch 2 times, most recently from 2f9ca73 to caae55b Compare February 28, 2020 13:44

jajanusz approved these changes Feb 28, 2020

View reviewed changes

lgirdwood approved these changes Feb 28, 2020

View reviewed changes

tlauda added 2 commits March 2, 2020 09:59

ipc: add is_notification_pending parameter

eb4b4c1

Adds additional parameter to control whether upstream IPC channel is currently free or not. It's helpful in case for some reason checking IPC bits is not enough. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

ipc: return status on notification sending

48c93c3

Returns status -EBUSY, when IPC upstream channel is currently busy and notification cannot be sent. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

tlauda added 4 commits March 2, 2020 09:59

ipc: add helper methods for building notifications

12c07c1

Adds helper methods for initial building of IPC notifications. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

tlauda force-pushed the topic/notifications-refactor branch from caae55b to ecdc7e6 Compare March 2, 2020 09:22

bkokoszx mentioned this pull request Mar 2, 2020

dai: overwrite hardware frame_fmt parameter with private frame_fmt #2421

Merged

tlauda merged commit 3609fa6 into thesofproject:master Mar 2, 2020

ipc: notifications refactor #2454

ipc: notifications refactor #2454

Uh oh!

Conversation

tlauda commented Feb 27, 2020

Uh oh!

dbaluta Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

tlauda Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

tlauda commented Feb 27, 2020

Uh oh!

mmaka1 commented Feb 27, 2020

Uh oh!

mmaka1 Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

mmaka1 Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

tlauda commented Feb 28, 2020

Uh oh!

paulstelian97 commented Feb 28, 2020

Uh oh!

tlauda commented Feb 28, 2020

Uh oh!

lgirdwood commented Feb 28, 2020

Uh oh!

lgirdwood commented Mar 2, 2020

Uh oh!

lgirdwood commented Mar 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lgirdwood commented Feb 27, 2020 •

edited

Loading