[Sound-open-firmware] [PATCH RFC] ASoC: SOF: IPC: remove direct mailbox reading from SOF core
Ranjani Sridharan
ranjani.sridharan at linux.intel.com
Sat Mar 30 20:02:59 CET 2019
On Fri, 2019-03-29 at 17:56 +0100, Guennadi Liakhovetski wrote:
> Obtaining data from the DSP by reading the mailbox is Intel-specific,
> move them from the SOF core to Intel-specific drivers.
>
> Signed-off-by: Guennadi Liakhovetski <
> guennadi.liakhovetski at linux.intel.com>
> ---
>
> This is just an RFC, it only supports APL at the moment, but has been
> tested
> on it. Adding all other platforms should be simple. Please, suggest
> better names.
>
> sound/soc/sof/intel/apl.c | 4 ++++
> sound/soc/sof/intel/hda-pcm.c | 40
> +++++++++++++++++++++++++++++++
> sound/soc/sof/intel/hda-stream.c | 18 +++++++++-----
> sound/soc/sof/intel/hda.h | 16 ++++++++++++-
> sound/soc/sof/intel/shim.h | 6 +++++
> sound/soc/sof/ipc.c | 51 +++++++++++++++++-------------
> ----------
> sound/soc/sof/ops.h | 28 ++++++++++++++++++++++
> sound/soc/sof/pcm.c | 30 +++++++++++++----------
> sound/soc/sof/sof-priv.h | 13 ++++++++++
> 9 files changed, 156 insertions(+), 50 deletions(-)
>
> diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
> index 654d741..d7fa884 100644
> --- a/sound/soc/sof/intel/apl.c
> +++ b/sound/soc/sof/intel/apl.c
> @@ -64,6 +64,10 @@
> .pcm_trigger = hda_dsp_pcm_trigger,
> .pcm_pointer = hda_dsp_pcm_pointer,
>
> + .pcm_read_dev = hda_pcm_read_dev,
> + .pcm_read_stream = hda_pcm_read_stream,
> + .pcm_params = hda_pcm_params,
> +
> /* firmware loading */
> .load_firmware = snd_sof_load_firmware_raw,
>
> diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-
> pcm.c
> index bbcef74..fcbf64d 100644
> --- a/sound/soc/sof/intel/hda-pcm.c
> +++ b/sound/soc/sof/intel/hda-pcm.c
> @@ -240,3 +240,43 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev,
> substream->runtime->private_data = NULL;
> return 0;
> }
> +
> +void hda_pcm_read_dev(struct snd_sof_dev *sdev, void *p, size_t sz)
> +{
> + snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
> +}
> +
> +void hda_pcm_read_stream(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + void *p, size_t sz)
> +{
> + struct hdac_stream *hstream = substream->runtime->private_data;
> + struct sof_intel_hda_stream *hda_stream = container_of(hstream,
> + struct sof_intel_hda_stream,
> hda_stream.hstream);
> +
> + snd_sof_dsp_mailbox_read(sdev, hda_stream->stream.posn_offset,
> + p, sz);
> +
> + dev_dbg(sdev->dev, "posn mailbox: posn offset is 0x%x",
> + hda_stream->stream.posn_offset);
> +}
> +
> +int hda_pcm_params(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + const struct sof_ipc_pcm_params_reply *reply)
> +{
> + struct hdac_stream *hstream = substream->runtime->private_data;
> + struct sof_intel_hda_stream *hda_stream = container_of(hstream,
> + struct sof_intel_hda_stream,
> hda_stream.hstream);
> + /* validate offset */
> + size_t posn_offset = reply->posn_offset;
> +
> + /* check if offset is overflow or it is not aligned */
> + if (posn_offset > sdev->stream_box.size ||
> + posn_offset % sizeof(struct sof_ipc_stream_posn) != 0)
> + return -EINVAL;
> +
> + hda_stream->stream.posn_offset = sdev->stream_box.offset +
> posn_offset;
> +
> + return 0;
> +}
> diff --git a/sound/soc/sof/intel/hda-stream.c
> b/sound/soc/sof/intel/hda-stream.c
> index 5b8e589..6f1530d 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -552,11 +552,14 @@ int hda_dsp_stream_init(struct snd_sof_dev
> *sdev)
>
> /* create capture streams */
> for (i = 0; i < num_capture; i++) {
> -
> - stream = devm_kzalloc(sdev->dev, sizeof(*stream),
> GFP_KERNEL);
> - if (!stream)
> + struct sof_intel_hda_stream *hda_stream =
> devm_kzalloc(sdev->dev,
> + sizeof(*hda_stream),
> GFP_KERNEL);
> + if (!hda_stream)
> return -ENOMEM;
>
> + stream = &hda_stream->hda_stream;
> + hda_stream->stream.hw_stream_priv = stream;
> +
> stream->pphc_addr = sdev->bar[HDA_DSP_PP_BAR] +
> SOF_HDA_PPHC_BASE + SOF_HDA_PPHC_INTERVAL * i;
>
> @@ -601,11 +604,14 @@ int hda_dsp_stream_init(struct snd_sof_dev
> *sdev)
>
> /* create playback streams */
> for (i = num_capture; i < num_total; i++) {
> -
> - stream = devm_kzalloc(sdev->dev, sizeof(*stream),
> GFP_KERNEL);
> - if (!stream)
> + struct sof_intel_hda_stream *hda_stream =
> devm_kzalloc(sdev->dev,
> + sizeof(*hda_stream),
> GFP_KERNEL);
> + if (!hda_stream)
> return -ENOMEM;
>
> + stream = &hda_stream->hda_stream;
> + hda_stream->stream.hw_stream_priv = stream;
> +
> /* we always have DSP support */
> stream->pphc_addr = sdev->bar[HDA_DSP_PP_BAR] +
> SOF_HDA_PPHC_BASE + SOF_HDA_PPHC_INTERVAL * i;
> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> index f9c055a..fc14c01 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -12,6 +12,7 @@
> #define __SOF_INTEL_HDA_H
>
> #include <sound/hda_codec.h>
> +#include <sound/hdaudio_ext.h>
> #include "shim.h"
>
> /* PCI registers */
> @@ -360,7 +361,7 @@ struct sof_intel_hda_dev {
> /* hw config */
> const struct sof_intel_dsp_desc *desc;
>
> - /*trace */
> + /* trace */
> struct hdac_ext_stream *dtrace_stream;
>
> /* if position update IPC needed */
> @@ -386,6 +387,11 @@ static inline struct hda_bus *sof_to_hbus(struct
> snd_sof_dev *s)
> return &hda->hbus;
> }
>
> +struct sof_intel_hda_stream {
> + struct hdac_ext_stream hda_stream;
> + struct sof_intel_stream stream;
> +};
> +
> #define bus_to_sof_hda(bus) \
> container_of(bus, struct sof_intel_hda_dev, hbus.core)
>
> @@ -462,6 +468,14 @@ int hda_dsp_stream_spib_config(struct
> snd_sof_dev *sdev,
> struct hdac_ext_stream *stream,
> int enable, u32 size);
>
> +void hda_pcm_read_dev(struct snd_sof_dev *sdev, void *p, size_t sz);
> +void hda_pcm_read_stream(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + void *p, size_t sz);
> +int hda_pcm_params(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + const struct sof_ipc_pcm_params_reply *reply);
> +
> /*
> * DSP IPC Operations.
> */
> diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h
> index 617da2d..16e4a34 100644
> --- a/sound/soc/sof/intel/shim.h
> +++ b/sound/soc/sof/intel/shim.h
> @@ -176,4 +176,10 @@ struct sof_intel_dsp_desc {
> extern const struct sof_intel_dsp_desc hsw_chip_info;
> extern const struct sof_intel_dsp_desc tng_chip_info;
>
> +struct sof_intel_stream {
> + int posn_offset;
> +
> + void *hw_stream_priv;
> +};
> +
> #endif
> diff --git a/sound/soc/sof/ipc.c b/sound/soc/sof/ipc.c
> index 63f9e96..01e9576 100644
> --- a/sound/soc/sof/ipc.c
> +++ b/sound/soc/sof/ipc.c
> @@ -343,7 +343,7 @@ void snd_sof_ipc_msgs_rx(struct snd_sof_dev
> *sdev)
> int err = 0;
>
> /* read back header */
> - snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset, &hdr,
> sizeof(hdr));
> + snd_sof_pcm_read_dev(sdev, &hdr, sizeof(hdr));
> ipc_log_header(sdev->dev, "ipc rx", hdr.cmd);
>
> cmd = hdr.cmd & SOF_GLB_TYPE_MASK;
> @@ -406,8 +406,7 @@ static void ipc_trace_message(struct snd_sof_dev
> *sdev, u32 msg_id)
> switch (msg_id) {
> case SOF_IPC_TRACE_DMA_POSITION:
> /* read back full message */
> - snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset,
> &posn,
> - sizeof(posn));
> + snd_sof_pcm_read_dev(sdev, &posn, sizeof(posn));
> snd_sof_trace_update_pos(sdev, &posn);
> break;
> default:
> @@ -424,15 +423,14 @@ static void ipc_trace_message(struct
> snd_sof_dev *sdev, u32 msg_id)
> static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id)
> {
> struct sof_ipc_stream_posn posn;
> + struct snd_sof_pcm_stream *stream;
> struct snd_sof_pcm *spcm;
> - u32 posn_offset;
> int direction;
>
> /* check if we have stream box */
> if (sdev->stream_box.size == 0) {
> /* read back full message */
> - snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset,
> &posn,
> - sizeof(posn));
> + snd_sof_pcm_read_dev(sdev, &posn, sizeof(posn));
>
> spcm = snd_sof_find_spcm_comp(sdev, posn.comp_id,
> &direction);
> } else {
> @@ -446,39 +444,35 @@ static void ipc_period_elapsed(struct
> snd_sof_dev *sdev, u32 msg_id)
> return;
> }
>
> - /* have stream box read from stream box */
> - if (sdev->stream_box.size != 0) {
> - posn_offset = spcm->posn_offset[direction];
> - snd_sof_dsp_mailbox_read(sdev, posn_offset, &posn,
> - sizeof(posn));
> + stream = &spcm->stream[direction];
>
> - dev_dbg(sdev->dev, "posn mailbox: posn offset is 0x%x",
> - posn_offset);
> - }
> + /* have stream box read from stream box */
> + if (sdev->stream_box.size != 0)
> + snd_sof_pcm_read_stream(sdev, stream->substream,
> + &posn, sizeof(posn));
>
> dev_dbg(sdev->dev, "posn : 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));
> + memcpy(&stream->posn, &posn, sizeof(posn));
>
> /* only inform ALSA for period_wakeup mode */
> - if (!spcm->stream[direction].substream->runtime-
> >no_period_wakeup)
> - snd_pcm_period_elapsed(spcm-
> >stream[direction].substream);
> + if (!stream->substream->runtime->no_period_wakeup)
> + snd_pcm_period_elapsed(stream->substream);
> }
>
> /* DSP notifies host of an XRUN within FW */
> static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id)
> {
> struct sof_ipc_stream_posn posn;
> + struct snd_sof_pcm_stream *stream;
> struct snd_sof_pcm *spcm;
> - u32 posn_offset;
> int direction;
>
> /* check if we have stream MMIO on this platform */
> if (sdev->stream_box.size == 0) {
> /* read back full message */
> - snd_sof_dsp_mailbox_read(sdev, sdev->dsp_box.offset,
> &posn,
> - sizeof(posn));
> + snd_sof_pcm_read_dev(sdev, &posn, sizeof(posn));
>
> spcm = snd_sof_find_spcm_comp(sdev, posn.comp_id,
> &direction);
> } else {
> @@ -491,23 +485,20 @@ static void ipc_xrun(struct snd_sof_dev *sdev,
> u32 msg_id)
> return;
> }
>
> - /* have stream box read from stream box */
> - if (sdev->stream_box.size != 0) {
> - posn_offset = spcm->posn_offset[direction];
> - snd_sof_dsp_mailbox_read(sdev, posn_offset, &posn,
> - sizeof(posn));
> + stream = &spcm->stream[direction];
>
> - dev_dbg(sdev->dev, "posn mailbox: posn offset is 0x%x",
> - posn_offset);
> - }
> + /* have stream box read from stream box */
> + if (sdev->stream_box.size != 0)
> + snd_sof_pcm_read_stream(sdev, stream->substream,
> + &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);
>
> #if defined(CONFIG_SND_SOC_SOF_DEBUG_XRUN_STOP)
> /* stop PCM on XRUN - used for pipeline debug */
> - memcpy(&spcm->stream[direction].posn, &posn, sizeof(posn));
> - snd_pcm_stop_xrun(spcm->stream[direction].substream);
> + memcpy(&stream->posn, &posn, sizeof(posn));
> + snd_pcm_stop_xrun(stream->substream);
> #endif
> }
>
> diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
> index cba2d16..54a6c6e 100644
> --- a/sound/soc/sof/ops.h
> +++ b/sound/soc/sof/ops.h
> @@ -330,6 +330,34 @@ static inline int
> snd_sof_dma_trace_trigger(struct snd_sof_dev *sdev, int cmd)
> return 0;
> }
>
> +/* host read DSP device data */
> +static inline void snd_sof_pcm_read_dev(struct snd_sof_dev *sdev,
> + void *p, size_t sz)
Hi Guennadi,
I think we should drop the pcm prefix from the op name. We read the
mailbox data for all IPC's and not just the ones related to the pcm
stream. So, maybe we can call it snd_sof_ipc_read()?
Thanks,
Ranjani
> +{
> + if (sof_ops(sdev) && sof_ops(sdev)->pcm_read_dev)
> + sof_ops(sdev)->pcm_read_dev(sdev, p, sz);
> +}
> +
> +/* host read DSP stream data */
> +static inline void snd_sof_pcm_read_stream(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream
> *substream,
> + void *p, size_t sz)
> +{
> + if (sof_ops(sdev) && sof_ops(sdev)->pcm_read_stream)
> + sof_ops(sdev)->pcm_read_stream(sdev, substream, p, sz);
> +}
> +
> +/* host configure DSP HW parameters */
> +static inline int snd_sof_pcm_params(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + const struct sof_ipc_pcm_params_reply
> *reply)
> +{
> + if (sof_ops(sdev) && sof_ops(sdev)->pcm_params)
> + return sof_ops(sdev)->pcm_params(sdev, substream,
> reply);
> +
> + return 0;
> +}
> +
> /* host stream pointer */
> static inline snd_pcm_uframes_t
> snd_sof_pcm_platform_pointer(struct snd_sof_dev *sdev,
> diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
> index 545eed4..a7c6b63 100644
> --- a/sound/soc/sof/pcm.c
> +++ b/sound/soc/sof/pcm.c
> @@ -38,6 +38,20 @@ static int create_page_table(struct
> snd_pcm_substream *substream,
> spcm->stream[stream].page_table.area, size);
> }
>
> +static int sof_pcm_dsp_params(struct snd_sof_pcm *spcm, struct
> snd_pcm_substream *substream,
> + const struct sof_ipc_pcm_params_reply
> *reply)
> +{
> + struct snd_sof_dev *sdev = spcm->sdev;
> + /* validate offset */
> + int ret = snd_sof_pcm_params(sdev, substream, reply);
> +
> + if (ret < 0)
> + dev_err(sdev->dev, "error: got wrong reply for PCM
> %d\n",
> + spcm->pcm.pcm_id);
> +
> + return ret;
> +}
> +
> /* this may get called several times by oss emulation */
> static int sof_pcm_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params)
> @@ -50,7 +64,6 @@ static int sof_pcm_hw_params(struct
> snd_pcm_substream *substream,
> struct snd_sof_pcm *spcm;
> struct sof_ipc_pcm_params pcm;
> struct sof_ipc_pcm_params_reply ipc_params_reply;
> - int posn_offset;
> int ret;
>
> /* nothing todo for BE */
> @@ -158,18 +171,9 @@ static int sof_pcm_hw_params(struct
> snd_pcm_substream *substream,
> return ret;
> }
>
> - /* validate offset */
> - posn_offset = ipc_params_reply.posn_offset;
> -
> - /* check if offset is overflow or it is not aligned */
> - if (posn_offset > sdev->stream_box.size ||
> - posn_offset % sizeof(struct sof_ipc_stream_posn) != 0) {
> - dev_err(sdev->dev, "error: got wrong posn offset 0x%x
> for PCM %d\n",
> - posn_offset, spcm->pcm.pcm_id);
> - return -EINVAL;
> - }
> - spcm->posn_offset[substream->stream] =
> - sdev->stream_box.offset + posn_offset;
> + ret = sof_pcm_dsp_params(spcm, substream, &ipc_params_reply);
> + if (ret < 0)
> + return ret;
>
> /* save pcm hw_params */
> memcpy(&spcm->params[substream->stream], params,
> sizeof(*params));
> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index c4465b2d..1ab4d5b 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -148,6 +148,19 @@ struct snd_sof_dsp_ops {
> snd_pcm_uframes_t (*pcm_pointer)(struct snd_sof_dev *sdev,
> struct snd_pcm_substream
> *substream); /* optional */
>
> + /* host read DSP device data */
> + void (*pcm_read_dev)(struct snd_sof_dev *sdev, void *p, size_t
> sz);
> +
> + /* host read DSP stream data */
> + void (*pcm_read_stream)(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + void *p, size_t sz);
> +
> + /* host configure DSP HW parameters */
> + int (*pcm_params)(struct snd_sof_dev *sdev,
> + struct snd_pcm_substream *substream,
> + const struct sof_ipc_pcm_params_reply
> *reply);
> +
> /* pre/post firmware run */
> int (*pre_fw_run)(struct snd_sof_dev *sof_dev); /* optional */
> int (*post_fw_run)(struct snd_sof_dev *sof_dev); /* optional */
More information about the Sound-open-firmware
mailing list