Repository navigation
ASoC: SOF: intel: don't read CTX_SAVE reply - #651
Conversation
There was a problem hiding this comment.
I thought we discussed that the msg->header field would no longer be used, see e.g. #586
May be that's a good counter-example that it should? Or can you use the body of the msg to find thhe command information
There was a problem hiding this comment.
@plbossart Yes, I can use the body to find the command information as struct sof_ipc_cmd_hdr is always the first element of msg_data. However, I think check msg->header doesn't against #586. We don't write msg->header to SHIM register in #586, but it doesn't touch msg->header. In other word, we can still use msg->header after #586 is merged.
There was a problem hiding this comment.
no the idea was that the msg->header would be removed if it serves no purpose.
There was a problem hiding this comment.
@plbossart updated. use the body of the msg to check if it is SOF_IPC_PM_CTX_SAVE
|
@bardliao should we generalize this to maybe support other IPC messages that dont require reading the mailbox? |
@ranj063 Yes, it's a good idea. Probably to add a flag in |
|
On 2/14/19 8:33 PM, Bard Liao wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In sound/soc/sof/intel/hda-ipc.c
<#651 (comment)>:
> @@ -89,8 +89,19 @@ int hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev,
int ret = 0;
u32 size;
- /* get IPC reply from DSP in the mailbox */
- sof_mailbox_read(sdev, sdev->host_box.offset, &reply, sizeof(reply));
+ if (msg->header == (SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CTX_SAVE)) {
@plbossart <https://github.com/plbossart> Yes, I can use the body to
find the command information as |struct sof_ipc_cmd_hdr| is always the
first element of |msg_data|. However, I think check |msg->header|
doesn't against #586
<#586>. We don't write
|msg->header| to SHIM register in #586
<#586>, but it doesn't
touch |msg->header|. In other word, we can still use |msg->header|
after #586 <#586> is merged.
it's really the other way around. If we no longer use the msg->header to
pass any information to the firmware, then the plan was to remove this
field since the information it contained was duplicated from the body of
the message going over a mailbox.
That said, there may be a need for sideband signals for power management
that don't go over the mailbox. I'd like the thread of the IPC design to
make progress before we commit to using this field or remove it.
|
|
Playback and suspend works fine in loop with applied this patch. There is no error message with it. kernel: cros-sof-v4.19 / 8492020d5967 |
There was a problem hiding this comment.
Isn't the header in a memory window? How does this work if the memory is no longer powered?
It feels like we actually do need to preserve the use of the msg/msg_ext bits precisely for this case.
There was a problem hiding this comment.
@plbossart msg->msg_data is dynamic allocated msg->msg_data = devm_kzalloc(sdev->dev, PAGE_SIZE, GFP_KERNEL);. I think it is not in a memory window.
|
I test the newest patch on WHL RVP, here is the dmesg, no CTX_SAVE error message. Test Env: dmesg: |
|
+ hdr = (struct sof_ipc_cmd_hdr *)msg->msg_data;
+ if (hdr->cmd == (SOF_IPC_GLB_PM_MSG | SOF_IPC_PM_CTX_SAVE)) {
+ /*
+ * memory windows are powered off before sending IPC reply,
+ * so we can't read the mailbox for CTX_SAVE reply.
+ */
@plbossart <https://github.com/plbossart> msg->msg_data is dynamic
allocated msg->msg_data = devm_kzalloc(sdev->dev, PAGE_SIZE,
GFP_KERNEL);
<https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/ipc.c#L816>.
I think it is not in a memory window.
That doesn't seem right. where is the data actually coming from?
Everything we do is based on memory-windows so there's got to be a copy
somewhere.
… |
The data is from memcpy(msg->msg_data, msg_data, msg_bytes); |
The memory windows are powered off before sending IPC reply back to the host, so the host should not try to read the mailbox for CTX_SAVE reply. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
@plbossart this is not coming from the memory window. The msg_data is pre-allocated kernel memory which is used for copying to the mailbox during ipc tx. The mailbox reply reading step actually happens after this check. So this check looks legitimate. But my concern is about the placement of this check. Should we possibly move this check to tx_wait_done() prior to Line 221 in ee19d24 While I'm at it, I want to raise my original point again. I think this should be more generic by maybe adding a field/flag to the ipc msg structure which suggests that the reply should not be read for this msg. |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
ok, I get the flow now and this makes sense.
Can you rebase and resubmit to try and solve CI issues?
5fa0c61 to
d2456f6
Compare
|
@plbossart CI issues are solved |
5 participants
The memory windows are powered off before sending IPC reply back to the
host, so the host should not try to read the mailbox for CTX_SAVE reply.
Signed-off-by: Bard liao yung-chuan.liao@linux.intel.com
Fix thesofproject/sof#984