On 3/3/2018 09:55, Pierre-Louis Bossart wrote:
If you ran checkpatch --codespell --strict, you would see this:
WARNING: 'postion' may be misspelled - perhaps 'position'? #6: Use memory window to store postion data, use old IPC as a notify.
CHECK: if this code is redundant consider removing it #109: FILE: sound/soc/sof/ipc.c:260: +#if 0
CHECK: if this code is redundant consider removing it #150: FILE: sound/soc/sof/ipc.c:300: +#if 0
please fix and resubmit.
see also comments below.
On 03/02/2018 03:21 AM, Xiuli Pan wrote:
From: Pan Xiuli xiuli.pan@linux.intel.com
Use memory window to store postion data, use old IPC as a notify.
Add memory window handler for BYT, HSW, BDW and APL.
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com
Test with: Mininow max rt5651 GP-MRB nocodec SOF master: 1693b66bb1d804ded975767cc1e5911e6ff9c93c SOF-Tool master: a02abb799405d0e4ad0d6bb46eacf6fbe958c06e https://github.com/plbossart/sound/tree/topic/sof-v4.14: 9513a73b981bc1917705671ec54402a7e21672eb
sound/soc/sof/hw-apl.c | 4 ++++ sound/soc/sof/hw-bdw.c | 4 ++++ sound/soc/sof/hw-byt.c | 4 ++++ sound/soc/sof/hw-hsw.c | 4 ++++ sound/soc/sof/ipc.c | 40 +++++++++++++++++++++++++++++++++++----- sound/soc/sof/pcm.c | 15 ++++++++++++--- sound/soc/sof/sof-priv.h | 2 ++ 7 files changed, 65 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sof/hw-apl.c b/sound/soc/sof/hw-apl.c index de0efe1..5a65e72 100644 --- a/sound/soc/sof/hw-apl.c +++ b/sound/soc/sof/hw-apl.c @@ -1164,11 +1164,15 @@ static void apl_get_windows(struct snd_sof_dev *sdev) snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, outbox_offset, outbox_size); + sdev->stream_box.offset = stream_offset; + sdev->stream_box.size = stream_size; dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", inbox_offset, inbox_size); dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", outbox_offset, outbox_size); + dev_dbg(sdev->dev, " stream region 0x%x - size 0x%x\n", + stream_offset, stream_size); } static int apl_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) diff --git a/sound/soc/sof/hw-bdw.c b/sound/soc/sof/hw-bdw.c index 5ec0c5b..9bcb0de 100644 --- a/sound/soc/sof/hw-bdw.c +++ b/sound/soc/sof/hw-bdw.c @@ -440,11 +440,15 @@ static void bdw_get_windows(struct snd_sof_dev *sdev) snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, outbox_offset, outbox_size); + sdev->stream_box.offset = stream_offset; + sdev->stream_box.size = stream_size; dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", inbox_offset, inbox_size); dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", outbox_offset, outbox_size); + dev_dbg(sdev->dev, " stream region 0x%x - size 0x%x\n", + stream_offset, stream_size); } static int bdw_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) diff --git a/sound/soc/sof/hw-byt.c b/sound/soc/sof/hw-byt.c index 031cec6..43a4028 100644 --- a/sound/soc/sof/hw-byt.c +++ b/sound/soc/sof/hw-byt.c @@ -257,11 +257,15 @@ static void byt_get_windows(struct snd_sof_dev *sdev) snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, outbox_offset, outbox_size); + sdev->stream_box.offset = stream_offset; + sdev->stream_box.size = stream_size; dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", inbox_offset, inbox_size); dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", outbox_offset, outbox_size); + dev_dbg(sdev->dev, " stream region 0x%x - size 0x%x\n", + stream_offset, stream_size); } static int byt_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) diff --git a/sound/soc/sof/hw-hsw.c b/sound/soc/sof/hw-hsw.c index 2f36216..b8523b4 100644 --- a/sound/soc/sof/hw-hsw.c +++ b/sound/soc/sof/hw-hsw.c @@ -441,11 +441,15 @@ static void hsw_get_windows(struct snd_sof_dev *sdev) snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size, outbox_offset, outbox_size); + sdev->stream_box.offset = stream_offset; + sdev->stream_box.size = stream_size; dev_dbg(sdev->dev, " mailbox upstream 0x%x - size 0x%x\n", inbox_offset, inbox_size); dev_dbg(sdev->dev, " mailbox downstream 0x%x - size 0x%x\n", outbox_offset, outbox_size); + dev_dbg(sdev->dev, " stream region 0x%x - size 0x%x\n", + stream_offset, stream_size); } static int hsw_fw_ready(struct snd_sof_dev *sdev, u32 msg_id) diff --git a/sound/soc/sof/ipc.c b/sound/soc/sof/ipc.c index da427d1..41068a3 100644 --- a/sound/soc/sof/ipc.c +++ b/sound/soc/sof/ipc.c @@ -249,10 +249,15 @@ EXPORT_SYMBOL(snd_sof_dsp_mailbox_init); static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id) { - struct snd_sof_pcm *spcm; struct sof_ipc_stream_posn posn; + struct snd_sof_pcm *spcm; + u32 posn_offset; int direction; + /* TODO: virtual BE/FE will still use IPC message. Should still use this
virtual has multiple meanings. This could be a stream with no PCM or a stream interfacing with virtio.
This is for the virtio part. I will refine the comment.
+ * read back function. + */ +#if 0 /* read back full message */ snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset, &posn, sizeof(posn)); @@ -260,33 +265,56 @@ static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id) posn.host_posn, posn.dai_posn, posn.wallclock); spcm = snd_sof_find_spcm_comp(sdev, posn.comp_id, &direction); +#endif + spcm = snd_sof_find_spcm_comp(sdev, SOF_IPC_MESSAGE_ID(msg_id), + &direction);
why can't this be dynamic, if mbox is supported use it else send an IPC to get the info.
That sounds a good idea. Let's discuss with Liam about this new design. + Liam Could we just try to check different memory window to get position info.
if (!spcm) { dev_err(sdev->dev, "error: period elapsed for unknown component %d\n", posn.comp_id); return; } + posn_offset = spcm->posn_offset[direction]; + snd_sof_dsp_mailbox_read(sdev, posn_offset, &posn, sizeof(posn));
+ dev_dbg(sdev->dev, "posn mailbox: posn offset is 0x%x", posn_offset); + dev_dbg(sdev->dev, "posn mailbox: host 0x%llx dai 0x%llx wall 0x%llx\n", + posn.host_posn, posn.dai_posn, posn.wallclock);
memcpy(&spcm->stream[direction].posn, &posn, sizeof(posn)); snd_pcm_period_elapsed(spcm->stream[direction].substream); } static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id) { - struct snd_sof_pcm *spcm; struct sof_ipc_stream_posn posn; + struct snd_sof_pcm *spcm; + u32 posn_offset; int direction; + /* TODO: virtual BE/FE will still use IPC message. Should still use this + * read back function. + */
+#if 0 /* read back full message */ snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset, &posn, sizeof(posn)); spcm = snd_sof_find_spcm_comp(sdev, posn.comp_id, &direction); +#endif + spcm = snd_sof_find_spcm_comp(sdev, SOF_IPC_MESSAGE_ID(msg_id), + &direction);
same here, why can't it be capability-based?
if (!spcm) { dev_err(sdev->dev, "error: XRUN for unknown component %d\n", posn.comp_id); return; } + posn_offset = spcm->posn_offset[direction]; + snd_sof_dsp_mailbox_read(sdev, posn_offset, &posn, sizeof(posn));
dev_dbg(sdev->dev, "posn XRUN: host %llx comp %d size %d\n", posn.host_posn, posn.xrun_comp_id, posn.xrun_size); @@ -298,12 +326,14 @@ static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id) static void ipc_stream_message(struct snd_sof_dev *sdev, u32 msg_id) { - switch (msg_id) { + u32 msg_cmd = msg_id & SOF_CMD_TYPE_MASK;
What is the intent of this filter? It's super misleading since the header for ipc_run is : static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id)
and used as:
spcm = snd_sof_find_spcm_comp(sdev, SOF_IPC_MESSAGE_ID(msg_id), &direction);
Here we just got know which PCM is updated from the IPC message ID. We used the low 16 bit data for the PCM ID and update that PCM's position. Just got some redesign about the IPC header. Should add some comment about this
+ switch (msg_cmd) { case SOF_IPC_STREAM_POSITION: ipc_period_elapsed(sdev, msg_id); break; case SOF_IPC_STREAM_TRIG_XRUN: - ipc_xrun(sdev, msg_id); + ipc_xrun(sdev, msg_cmd); break; default: dev_err(sdev->dev, "error: unhandled stream message %x\n", @@ -372,7 +402,7 @@ static void ipc_msgs_rx(struct work_struct *work) case SOF_IPC_GLB_COMP_MSG: break; case SOF_IPC_GLB_STREAM_MSG: - ipc_stream_message(sdev, type); + ipc_stream_message(sdev, hdr.cmd);
type = hdr.cmd & SOF_CMD_TYPE_MASK;
static void ipc_stream_message(struct snd_sof_dev *sdev, u32 msg_id) { u32 msg_cmd = msg_id & SOF_CMD_TYPE_MASK;
switch (msg_cmd) { case SOF_IPC_STREAM_POSITION: ipc_period_elapsed(sdev, msg_id); break; case SOF_IPC_STREAM_TRIG_XRUN: ipc_xrun(sdev, msg_cmd); break;
WTH?
Some redesign about IPC message header. Did not know how to handle these IPC interface, did you have any idea about these? We need get the ID of the updated PCM to get position info updated.
break; case SOF_IPC_GLB_TRACE_MSG: ipc_trace_message(sdev, type); diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c index 771625f..129f018 100644 --- a/sound/soc/sof/pcm.c +++ b/sound/soc/sof/pcm.c @@ -53,6 +53,7 @@ static int sof_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_sof_pcm *spcm = rtd->sof; struct sof_ipc_pcm_params pcm; struct sof_ipc_pcm_params_reply ipc_params_reply; + int posn_offset; int ret; /* nothing todo for BE */ @@ -132,9 +133,6 @@ static int sof_pcm_hw_params(struct snd_pcm_substream *substream, return -EINVAL; } - /* copy offset */ - //spcm->posn_offset[substream->stream] = ipc_params_reply.posn_offset;
/* firmware already configured host stream */ if (ops && ops->host_stream_prepare) { pcm.params.stream_tag = @@ -147,6 +145,17 @@ static int sof_pcm_hw_params(struct snd_pcm_substream *substream, pcm.hdr.cmd, &pcm, sizeof(pcm), &ipc_params_reply, sizeof(ipc_params_reply)); + /* caclcute offset */
calculate
+ posn_offset = ipc_params_reply.posn_offset; + if (posn_offset > sdev->stream_box.size && + posn_offset % sizeof(struct sof_ipc_stream_posn) == 0) {
I don't get your range checking, it looks puzzling. A combination of logical AND and modulo for a position?
Liam asked to have a validation about the offset. This check the offset size is overflow or if the offset is not aligned with the structure size. I will add a comment about this check.
+ dev_err(sdev->dev, "error: got wrong posn offset 0x%x for PCM %d\n", + posn_offset, ret); + return ret; + } + spcm->posn_offset[substream->stream] = + sdev->stream_box.offset + posn_offset;
return ret; } diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index a096595..ecf0012 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -173,6 +173,7 @@ struct snd_sof_pcm { struct snd_sof_dev *sdev; struct snd_soc_tplg_pcm pcm; struct snd_sof_pcm_stream stream[2]; + u32 posn_offset[2]; struct mutex mutex; /* TODO: not used ? remove ? */ struct list_head list; /* list in sdev pcm list */ }; @@ -321,6 +322,7 @@ struct snd_sof_dev { struct snd_sof_ipc *ipc; struct snd_sof_mailbox dsp_box; /* DSP initiated IPC */ struct snd_sof_mailbox host_box; /* Host initiated IPC */ + struct snd_sof_mailbox stream_box; /* Stream position update */ u64 irq_status; int ipc_irq; u32 next_comp_id; /* monotonic - reset during S3 */
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware