Upstream: address IPC tx/rx work necessity #580

Activity

1. ranj063

   changed the title [-]Upstream: address IPC tx/rx work necessarity[/-] [+]Upstream: address IPC tx/rx work necessity[/+] on Jan 23, 2019
2. wenqingfu

   assigned

   lyakh

   on Feb 1, 2019
3. lyakh commented on Mar 1, 2019

   

   lyakh

   on Mar 1, 2019

   Collaborator

   More actions

   @plbossart Let's use this issue, since removing scheduled work is probably the most intrusive change of all proposed. Here's my analysis and a proposed transition:

   1. synchronous TX. Since every IPC transmission is anyway waiting for a reply,
      no need for work offloading. Instead sleep on a mutex, send, wait for reply.
      This will also make handling of simultaneous racing users explicit: they will
      be serialised by the mutex.

   2. synchronous RX. The FW always waits for the host to receive a message before
      sending the next one. DSP->Host messages can be of 3 types:

      1. replies. There must be exactly one reply to each Host->DSP message.
      2. panic messages.
      3. new message.
         (1) and (2) are processed immediately in the IRQ thread context, (3) is
         postponed to the scheduler work queue. Moving (3) to the IRQ thread would
         simplify the state machine, reduce latency, close a race window.

   1+2.
   That means, there is no need for .tx_list and .reply_list message queues,
   they can be replaced with just one pointer for the current message, waiting
   for a DSP response. We also don't need a list of empty messages, since we
   will only be processing one at a time. If a timeout is desired for sending, a
   timeout version can be created too.

   3. what happens if the Host and the DSP decide to send a message simultaneously?
      They both will check a "busy" flag and, if free, they will proceed with
      copying the data to the buffer and triggering an interrupt on the receiver
      side. This creates a race window. Mailbox windows are configured in FW in
      platform.c, Host and DSP windows are separate non-overlapping (verified only
      for cAVS) memory regions. Does simultaneously writing to initiator registers
      from the DSP and the Host sides work?

   4. snd_sof_dsp_is_ipc_ready() can be implemented using
      wait_event(_interruptible)(_timeout)(). Presumably if any of the HW busy
      flags is set, some processing - either sending or receiving - will be
      handling them, after which the event can be triggered. Since TX will be
      serialised, only an incoming RX message from the DSP can render the mailbox
      not ready. After RX processing completion the event would be toggled.
      However, Pierre explains, that not-ready is only produced by a TX in
      progress. If that's the case, snd_sof_dsp_is_ipc_ready() can simply be
      removed.

   5. for RPMsg compatibility, a callback-based sending can be added too. That
      version could use queued work, but maybe a dedicated work queue should be
      created. However, this doesn't seem like a major rework, therefore it can be
      left out for now.

   6. no need to search for a matching reply, no message ID is needed.

   7. no retry is needed with synchronous operation.

   8. writing cmd to shim: BDW and HSW don't do that. Again, it would be easier to
      limit upstreaming to HDA to begin with.

   9. page table fix:

   		u8 *pg_table;
   
   		if (i & 1)
   			put_unaligned_le32(pg_table[0] & 0xf | pfn << 4,
   					   pg_table);
   		else
   			put_unaligned_le32(pfn, pg_table);
   

   10. why are PM functions protected by a mutex? E.g. in sof_compressed_open(),
       sof_compressed_free(), sof_set_restore_stream().

   11. struct snd_sof_dev::ipc_lock has to be clarified. The comment "lock for IPC
       users" isn't very helpful. Maybe it can be completely replaced by a mutex.

   Proposed transition path:

   1. Currently the RX path has no locking. Explicit serialisation is also not
      required since it will be executed completely in IRQ thread context.
      Therefore it is safe to first eliminate the RX work.

   2. Add a mutex to sending, encompassing TX and waiting for a reply.

   3. Remove TX work

   4. Remove message lists
4. plbossart commented on Mar 1, 2019

   

   plbossart

   on Mar 1, 2019

   Member

   More actions

   @lyakh: there's a ton of good info in there, but I am at a loss as to how to reply. GitHub is not a wiki or editable at will, it's just not productive to use this tool for an interactive design discussion. We might as well use Google Docs or something meant for collaborative editing.
5. lyakh commented on Mar 18, 2019

   

   lyakh

   on Mar 18, 2019

   Collaborator

   More actions

   resolved by #695
6. lyakh

   closed this as completedon Mar 18, 2019
7. ujfalusi

   added a commit that references this issue on Jan 11, 2023

   devlink: hold region lock when flushing snapshots

   ...

   b4cafb3