Re: [Sound-open-firmware] [PATCH RFC] ASoC: SOF: IPC: remove direct mailbox reading from SOF core

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@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 */