[Sound-open-firmware] [PATCH V3 2/2] ASoc: SOF: Replace IPC position update with memory window
Pan, Xiuli
xiuli.pan at linux.intel.com
Mon Mar 5 07:16:03 CET 2018
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 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.
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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list