Repository navigation
IPC for upstream - #695
IPC for upstream#695
Conversation
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
@lyakh I am impressed, this looks promising.
I added some comments below, most of them cosmetic or about the patch handling, the only thing I didn't get is the use of both mutex and spin lock, that seems like a belt/braces approach, wondering if this is necessary.
I'll try to break this when I get a moment but I wonder if our QA team could help. @mengdonglin can you ask folks to run their scripts and point to any regression due to this PR? Thanks!
There was a problem hiding this comment.
HIPC -> IPC registers to make this less Intel-centric?
I am also wondering if we could take the scissor and make this the snd_sof_ipc_doorbell structure, others may have different ways of sending stuff across to the firmware
There was a problem hiding this comment.
I've reworded the comment. As for the renaming - sure, we can do that, but maybe after we've fixed all the main upstreaming-blockers?
There was a problem hiding this comment.
Humm, odd. do we need both mutex and spin_lock ?
There was a problem hiding this comment.
In principle it isn't something extraordinary, but in our case after all these changes it becomes apparent, that we can now also drop the spin-lock. I'll add a patch for that.
There was a problem hiding this comment.
In fact, no, I think we do need a spin-lock. But not quite the way it is currently used. I will use it to synchronise IPC IRQ threads and the IPC TX context. I think there is currently a race there. Even with the present code we can have:
[core 0]
sof_ipc_tx_message()
spin_lock_irq(&sdev->ipc_lock);
list_add_tail(&msg->list, &ipc->tx_list);
spin_unlock_irq(&sdev->ipc_lock);
snd_sof_ipc_msgs_tx(sdev);
<run work-queue>
ipc_tx_next_msg()
spin_lock_irq(&sdev->ipc_lock);
snd_sof_dsp_send_msg(sdev, msg);
[core 1]
*** response coming ***
hda_dsp_ipc_irq_thread()
reply = snd_sof_ipc_reply(sdev, msg);
sof_ipc_reply_find_msg()
*** the above doesn't find the message on the list ***
list_move(&msg->list, &ipc->reply_list);
spin_unlock_irq(&sdev->ipc_lock);
So, the receiver will wrongly conclude, that the message isn't a reply. To fix this the sof_ipc_reply_find_msg() must take the same spin-lock as ipc_tx_next_msg(). I'll fix this.
There was a problem hiding this comment.
I must admit though, that this race is rather theoretical. Core 0 in that code fragment is under spin_lock_irq(), i.e. it has preemption and local IRQs disabled. The chance, that the DSP receives the interrupt, sends a response, it is received by the host, an IRQ thread gets scheduled on core 1, and it checks the reply_list within that race window is probably very close to 0.
There was a problem hiding this comment.
would be good to add a comment explaining that the use of a spin lock is intentional.
There was a problem hiding this comment.
we can probably do a better protection here. there is a define with the IPC max size somewhere, and it's way smaller than a page.
There was a problem hiding this comment.
This is a pure "mechanical" protection for memcpy(). The .msg_data is allocated with one page of memory, so, that's the check here. We could use a new macro for the IPC buffer size though. Wouldn't we need that complete page of memory with new large IPC messages?
There was a problem hiding this comment.
@lyakh the IPC is using a window allocated by the firmware, there is nothing that is tied to the Linux pages.
There was a problem hiding this comment.
could this be squashed/handled differently? The change above was added a couple of patches ago and is again modified here.
There was a problem hiding this comment.
I agree, that some patches can and should be squashed together, like 01/11 and 11/11, but in this case I think those are two different changes that just happen to touch the same code.
There was a problem hiding this comment.
if we disable the ipc_queue, then is -EAGAIN a valid return value? retrying isn't going to be more successful.
There was a problem hiding this comment.
@lyakh @plbossart there is no queue anymore. So we can just remove disable_ipc_queue
There was a problem hiding this comment.
@ranj063 @plbossart I agree, that EAGAIN isn't a good choice, I'll replace it with ENODEV. But I actually think that we need that flag. It should be used to wait for a running IPC TX to complete. But for that setting the flag should be guarded by the same mutex. I'll make that change. Thanks for catching!
|
@lyakh @plbossart of all the changes, the removal of the lists is what makes me the most nervous here. But like you said we'd probably have to run some stress tests to see if there is any impact. |
There was a problem hiding this comment.
I think adding one comment here saying how the pages are compressed would be very helpful. I remember that I spent some time to figure out why if i is odd then you need to shift by 4. And now I don't even remember if I figure that out.
There was a problem hiding this comment.
yeah, it's really not clear. It took me a while to figure out that 2.5 bytes for each compressed value means every other value needs to be shifted by 4 bits (half a byte) and ORed, and likewise that the offsets are 2,5,7,10, etc.
Someone wanted to be smart and only managed to confuse everyone else I guess...
There was a problem hiding this comment.
@dbaluta i can empathize with you on that. Everytime I read this bit of code, I need a piece of paper to work out whats going on
There was a problem hiding this comment.
oh, I thought it was a nice little exercise to keep the reader awake, but ok, if you want... ;-)
There was a problem hiding this comment.
Legacy physical page compression used on the HSW/BDW firmware, used to describe deep buffers in a small size. This has been inherited by SOF.
There was a problem hiding this comment.
ipcx & SHIM_BYT_IPCX_DONE is removed, so cmd_done function should be refined because SHIM_BYT_IPCX_DONE is set for is_ipc_ready. And please refine other platforms.
There was a problem hiding this comment.
This is my advice. You could also refer to this branch https://github.com/thesofproject/linux/tree/archive/topic/sof-dev-20180619/sound/soc/sof/intel to get the original code. We added some code to fix IPC bugs, now they are not needed. Please only change the code for host sending msg to FW, not FW to host now.
- if (ipcx & SHIM_BYT_IPCX_DONE &&
- !(imrx & SHIM_IMRX_DONE)) {
- /* Mask Done interrupt before first */
- snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR,
- SHIM_IMRX,
- SHIM_IMRX_DONE,
- SHIM_IMRX_DONE);
- /*
- * handle immediate reply from DSP core. If the msg is
- * found, set done bit in cmd_done which is called at the
- * end of message processing function, else set it here
- * because the done bit can't be set in cmd_done function
- * which is triggered by msg
- */
- if (snd_sof_ipc_reply(sdev, ipcx))
- byt_cmd_done(sdev, SOF_IPC_DSP_REPLY);
+ if (ipcx & SHIM_BYT_IPCX_DONE) {
+
+ snd_sof_ipc_reply(sdev, ipcx);
+
+ /* clear DONE bit - tell DSP we have completed */
+ snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR, SHIM_IPCX,
+ SHIM_BYT_IPCX_DONE, 0);
+
+ /* unmask Done interrupt */
+ snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR, SHIM_IMRX,
+ SHIM_IMRX_DONE, 0)
}
/* new message from DSP */
@@ -427,14 +422,6 @@ static int byt_cmd_done(struct snd_sof_dev *sdev, int dir)
/* unmask busy interrupt */
snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR, SHIM_IMRX,
SHIM_IMRX_BUSY, 0);
- } else {
- /* clear DONE bit - tell DSP we have completed */
- snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR, SHIM_IPCX,
- SHIM_BYT_IPCX_DONE, 0);
-
- /* unmask Done interrupt */
- snd_sof_dsp_update_bits64_unlocked(sdev, BYT_DSP_BAR, SHIM_IMRX,
- SHIM_IMRX_DONE, 0);
}
There was a problem hiding this comment.
Hm, I think there's a bug in my patch set. Looks like there are indeed cases when snd_sof_ipc_reply() has to return a non-zero value, specifically when no message is being transmitted. I'll fix that and will think about your comments. Thanks!
There was a problem hiding this comment.
Concerning your comment. It seems to make sense to me, but I'd postpone it to a separate patch or a patch set. With this patch set I tried to change the actual control flow as little as possible. This would be a significant change. So, unless it is really tightly coupled to this patch set, I'd rather postpone it to a later PR.
|
I have a suggestion: |
There was a problem hiding this comment.
Now snd_sof_ipc_reply alway replies 0. Actually there is another effect for original code returning -EINVAL.
It is for ROM message which is not in our msg list. There is a workaround on up2( I mentioned msg != 0x1004000) , so you don't get any issue, but the workaround is not on other platforms.
There was a problem hiding this comment.
As mentioned above, I think that's wrong. snd_sof_ipc_reply() has to distinguish between replies and new messages from the DSP. I'll restore that function.
There was a problem hiding this comment.
I don't understand this case. If this is for a ROM message, that isn't on the list, so, that isn't a response, that's an asynchronous message from the ROM, right? Then why does the HDA IPC IRQ handler recognises it as a reply: "is this a reply message from the DSP?" I've added a section for this in the google doc, please, explain this either here or there.
There was a problem hiding this comment.
I checked the code and the work flow of security component. Now, when a FW is transferred to ROM at boot up stage or resuming stage, ROM would check the FW with security component, do some job and reply a msg to HOST to notify it is successful.
Actually, you could find "error: rx list empty but received " in demsg at boot up stage or resuming stage. It is this case.
I don't know whether there are other messages would be replied to HOST and ROM also changes.
|
Concerning the CI failure: it looks like a build timeout, we could retrigger it or we can wait until I submit an update. |
5202ac5 to
57f4a2f
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Can we simplify the IPC operations a little more at the generic level and only have a tx_msg() and rx_callback() function pointers in the operations structure. Atm, the ops are rather Intel specific to the doorbell HW, and making then more generic pushes the HW specific code into HW drivers whilst allowing the generic code to better align with SPI, I2C, virtIO APIs for messaging.
There was a problem hiding this comment.
Legacy physical page compression used on the HSW/BDW firmware, used to describe deep buffers in a small size. This has been inherited by SOF.
|
@lyakh btw, please split your PRs per subject. There are several patches in here that can be merged now but are not related to IPC (and will block on the IPC work). |
There was a problem hiding this comment.
@lyakh do we need this function? I think it makes sense to just set ipc_complete and wakeUp here. That way it is easier to tell that it is in the locked context.
There was a problem hiding this comment.
I was on and off about this, in general I'd tend to agree, that it's easier to read with that function inlined, but I decided, that it would add just one more trivial patch to this already rather intense patch series, so, I decided to leave it out for now, we can do it later.
|
@lgirdwood I've split off the page-table patch, thanks. As for simplifying IPC ops - this PR is aimed at addressing upstreaming blockers. I think it's better to keep it that way, resolve all the blockers, get the code into the mainline, and then continue our development. Since this is an internal API, changing it shouldn't be a problem at any point of time. |
The point is that changing it now simplifies the core IPC messaging logic considerably and pushes the HW specific complexity into the HW drivers for abstraction. i.e. making it easier to manage and upstream. |
|
@lyakh @plbossart we've got positive feedback with this PR for at least a couple of lingering stress test issues. Keqiao reported thesofproject/sof#1050 has reduced from 15% to 1% probability of IPC timeouts |
|
@lyakh actually changing the API now would help our chances for upstream, since it'd show a path for others to use their own IPC. Otherwise we are going to answer to technical comments but not to the fundamental open of 'how to I add support for a non-Intel IPC, be it based on RPMsg or not'. |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
minor nit-picks below - mostly I don't remember what the previous comments/replies were...
Looks good, we should probably merge this tomorrow.
There was a problem hiding this comment.
would be good to add a comment explaining that the use of a spin lock is intentional.
There was a problem hiding this comment.
didn't we say BUSY wasn't the right return value? I am confused maybe
There was a problem hiding this comment.
No, I think it was a comment for an EAGAIN error code. This specific hunk isn't adding or changing the return code, it's just replacing a "return" with a "goto" to have a chance to unlock the mutex. A later patch removes this error code completely, once lists of messages are removed too.
|
@plbossart @lgirdwood sure, I'm not opposing that proposed API change "now," I was just trying to make sure, that "now" doesn't mean this PR, but a follow-up one, once this has been merged. |
|
@lyakh Can you look into the conflicts and resubmit? Thanks! |
Invert a condition and use an early return from a function to reduce its nesting depth. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Using the RX work isn't required since also without it the serialisation is guaranteed by proocessing received messages directly in the IRQ thread. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The .wait_txq wait queue head is initialised but never used. Remove it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
IPC TX is currently fully synchronous, the only entry point to send an IPC message is sof_ipc_tx_message() and it unconditionally waits for a reply to the message being sent. Enclosing the complete function into a mutex lock-unlock block ensures, that IPC registers and structs aren't accessed from multiple contexts. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
If snd_sof_ipc_reply() is called from an IPC ISR for a DSP response, i.e. after snd_sof_dsp_send_msg() has sent a message out, but before the message is moved to the .reply_list in ipc_tx_next_msg(), it won't find the message to which the received response is related. This race is almost certainly only theoretical, since ipc_tx_next_msg() is holding a spin-lock and has local interrupts disabled while sending the message out. Still, it has to be closed. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
An IPC TX work was used to serialise IPC message transmission. Now that a mutex is used for this, scheduling isn't needed any more. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
IPC message lists are currently unused. Only one message is ever used. Remove lists and use just one message. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The sof_ipc_tx_message() function is called from multiple locations in SOF to transmit IPC messages. It copies data from the supplied location to an internal fixed-size buffer without verifying the size and prepares a response buffer for data from the DSP, if any, also without checking the size of requested response data. This can lead to buffer overflows. Add a check to avoid it. When allocating those buffers only allocate the maximum IPC message size. Also remove type-casting of those buffers, since they are of the "void *" type. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
.is_ipc_ready() is a platform-provided method, that checks whether an IPC transmission operation is currently in progress. It is used by sof_ipc_tx_message() itself to avoid races. However, now the complete sof_ipc_tx_message() function is protected by a mutex, therefore it cannot be re-entered. This allows us to remove .is_ipc_ready() by implicitly replacing its return code with a "true" value. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
@plbossart sorry, I don't see any conflicts - neither on github nor when trying to apply the patches on top of the current sof-dev, or are you talking about a different branch? |
|
The 2 CI failures are: which I would ignore here, since we don't care about alignment or padding in this case, and using an int where a "bool" should be used seems clumsy to me. And possibly which I'm not sure what to do about and the build seems to complete after that anyway. |
6 participants
This PR aims at resolving ALSA maintainers' comments to the SOF use of IPC. It follows the discussion #580 which then has been continued on google docs. This has survived a simultaneous capture / playback test on UP^2 with nocodec.