[Sound-open-firmware] [PATCH V3 2/2] ASoc: SOF: Replace IPC position update with memory window
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Sat Mar 3 02:55:07 CET 2018
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 at 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 at 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);
> 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?
> 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 */
More information about the Sound-open-firmware
mailing list