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.
* 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.
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);
- 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);@@ -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?
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?
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 */