Repository navigation
Conversation
We should find message from reply_list with the whole message header(32 bits), not only the lowest 16 bits message id, otherwise, the searching will always return either with the 1st message in the list(as all our messages now are with 0x0 lowest 16 bits), or with no matched and show error logs always to be "error: rx list empty but received 0x0". Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
As some of the highest IPC register bits are occupied for busy/done interrupts, we can't use them for IPC message payload. This is only an simple fix for this ipc message matching check but not perfect, we may need to switch to use host_box to get the whole message for checking in future, which need changes together in FW side. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
I really don't like the methodology here. We keep doing those incremental IPC fixes and it's still the weakest link in the chain. Seriously, all the problems i've encountered since I came back at the end of October are due to IPC issues.
We really need to uplevel things and have a design document (text only) that describes how the IPC is supposed to work, and second look at the implementation.
I will not merge any IPC-related fixes until we have such a document and a shared understanding of the end-goal.
agreed, as I mentioned in the 2nd commit, I don't like this imperfect idea, I prefer we do this fix after SHIM register message cleanup first, and then we may only need the 1st commit here. I submitted it with [RFC] to draw intention to the issue existed here, maybe I should change title to [DO NOT MERGE] now. |
keyonjie
changed the title
|
@plbossart @keyonjie agree. A design document would be excellent to understand how things works. As far as I understand:
I think we can now fix 1) by merging the two PR referenced there. |
| header = SOF_IPC_MESSAGE_ID(header); | ||
|
|
||
| list_for_each_entry(msg, &ipc->reply_list, list) { | ||
| if (SOF_IPC_MESSAGE_ID(msg->header) == header) |
There was a problem hiding this comment.
@keyonjie could you explain where those low 16 bits of IPC commands, masked by SOF_IPC_MESSAGE_ID(), are actually set? I've searched through both the driver and the firmware IPC code and see a few locations, where those bits are read and checked, but I don't see them anywhere being set?
|
I still don't get what the issue is:
What are we trying to accomplish here? |
Agree, if we can make sure dealing with replied messages sequentially, then we don't need the list. Today's problem is that we can't make sure about that as we are using a scheduled work to reading/handling the replied messages, so, if #581 is merged, the next step we can remove this list, that's the reason I prefix this PR "DO NOT MERGE" -- we don't necessarily need searching/matching replied message.
This is a choice, if we decide to employ(the reality is that we haven't done that in both FW and driver) this ID(lowest 16 bits, 0~65535), we need implement it in both driver(increase ID for each Tx) and FW(make sure the new added ID content won't break any existed message comparing/checking logic) side simultaneously.
|
|
closing since @lyakh is working on a better IPC |
5 participants
This is aimed to fix #516, it is only RFC and I am not quite satisfied with the solution yet.
The issue is that sof_ipc_reply_find_msg() may find(logically only) wrong message from reply list in today's implementation.
As described in the 2nd commit, this is only an simple fix for this ipc message matching check but not
perfect, we may need to switch to use host_box to get the whole message for checking in future, which need changes together in FW side.
Any comments or follow up will be appreciated.