Re: [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

[Takashi Iwai <tiwai@suse.de>]

On Tue, 11 Dec 2018 22:23:08 +0100,
Pierre-Louis Bossart wrote:
> 
> From: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> 
> Define an IPC ABI for all host <--> DSP communication. This ABI should
> be transport agnostic. i.e. it should work on MMIO and SPI/I2C style
> interfaces.
> 
> Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/sound/sof/control.h   | 125 ++++++
>  include/sound/sof/dai-intel.h | 175 ++++++++
>  include/sound/sof/dai.h       |  75 ++++
>  include/sound/sof/header.h    | 158 +++++++
>  include/sound/sof/info.h      | 118 +++++
>  include/sound/sof/pm.h        |  48 ++
>  include/sound/sof/stream.h    | 149 +++++++
>  include/sound/sof/trace.h     |  66 +++
>  sound/soc/sof/ipc.c           | 813 ++++++++++++++++++++++++++++++++++
>  9 files changed, 1727 insertions(+)
>  create mode 100644 include/sound/sof/control.h
>  create mode 100644 include/sound/sof/dai-intel.h
>  create mode 100644 include/sound/sof/dai.h
>  create mode 100644 include/sound/sof/header.h
>  create mode 100644 include/sound/sof/info.h
>  create mode 100644 include/sound/sof/pm.h
>  create mode 100644 include/sound/sof/stream.h
>  create mode 100644 include/sound/sof/trace.h
>  create mode 100644 sound/soc/sof/ipc.c
> 
> diff --git a/include/sound/sof/control.h b/include/sound/sof/control.h
> new file mode 100644
> index 000000000000..7d7d2eba7d76
> --- /dev/null
> +++ b/include/sound/sof/control.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
> +/*
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + * redistributing this file, you may do so under either license.
> + *
> + * Copyright(c) 2018 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INCLUDE_SOUND_SOF_CONTROL_H__
> +#define __INCLUDE_SOUND_SOF_CONTROL_H__
> +
> +#include <uapi/sound/sof/header.h>
> +#include <sound/sof/header.h>
> +
> +/*
> + * Component Mixers and Controls
> + */
> +
> +/* channel positions - uses same values as ALSA */
> +enum sof_ipc_chmap {
> +	SOF_CHMAP_UNKNOWN = 0,
> +	SOF_CHMAP_NA,		/**< N/A, silent */
> +	SOF_CHMAP_MONO,		/**< mono stream */
> +	SOF_CHMAP_FL,		/**< front left */
> +	SOF_CHMAP_FR,		/**< front right */
> +	SOF_CHMAP_RL,		/**< rear left */
> +	SOF_CHMAP_RR,		/**< rear right */
> +	SOF_CHMAP_FC,		/**< front centre */
> +	SOF_CHMAP_LFE,		/**< LFE */
> +	SOF_CHMAP_SL,		/**< side left */
> +	SOF_CHMAP_SR,		/**< side right */
> +	SOF_CHMAP_RC,		/**< rear centre */
> +	SOF_CHMAP_FLC,		/**< front left centre */
> +	SOF_CHMAP_FRC,		/**< front right centre */
> +	SOF_CHMAP_RLC,		/**< rear left centre */
> +	SOF_CHMAP_RRC,		/**< rear right centre */
> +	SOF_CHMAP_FLW,		/**< front left wide */
> +	SOF_CHMAP_FRW,		/**< front right wide */
> +	SOF_CHMAP_FLH,		/**< front left high */
> +	SOF_CHMAP_FCH,		/**< front centre high */
> +	SOF_CHMAP_FRH,		/**< front right high */
> +	SOF_CHMAP_TC,		/**< top centre */
> +	SOF_CHMAP_TFL,		/**< top front left */
> +	SOF_CHMAP_TFR,		/**< top front right */
> +	SOF_CHMAP_TFC,		/**< top front centre */
> +	SOF_CHMAP_TRL,		/**< top rear left */
> +	SOF_CHMAP_TRR,		/**< top rear right */
> +	SOF_CHMAP_TRC,		/**< top rear centre */
> +	SOF_CHMAP_TFLC,		/**< top front left centre */
> +	SOF_CHMAP_TFRC,		/**< top front right centre */
> +	SOF_CHMAP_TSL,		/**< top side left */
> +	SOF_CHMAP_TSR,		/**< top side right */
> +	SOF_CHMAP_LLFE,		/**< left LFE */
> +	SOF_CHMAP_RLFE,		/**< right LFE */
> +	SOF_CHMAP_BC,		/**< bottom centre */
> +	SOF_CHMAP_BLC,		/**< bottom left centre */
> +	SOF_CHMAP_BRC,		/**< bottom right centre */
> +	SOF_CHMAP_LAST = SOF_CHMAP_BRC,
> +};
> +
> +/* control data type and direction */
> +enum sof_ipc_ctrl_type {
> +	/*  per channel data - uses struct sof_ipc_ctrl_value_chan */
> +	SOF_CTRL_TYPE_VALUE_CHAN_GET = 0,
> +	SOF_CTRL_TYPE_VALUE_CHAN_SET,
> +	/* component data - uses struct sof_ipc_ctrl_value_comp */
> +	SOF_CTRL_TYPE_VALUE_COMP_GET,
> +	SOF_CTRL_TYPE_VALUE_COMP_SET,
> +	/* bespoke data - struct struct sof_abi_hdr */
> +	SOF_CTRL_TYPE_DATA_GET,
> +	SOF_CTRL_TYPE_DATA_SET,
> +};
> +
> +/* control command type */
> +enum sof_ipc_ctrl_cmd {
> +	SOF_CTRL_CMD_VOLUME = 0, /**< maps to ALSA volume style controls */
> +	SOF_CTRL_CMD_ENUM,	/**< maps to ALSA enum style controls */
> +	SOF_CTRL_CMD_SWITCH,	/**< maps to ALSA switch style controls */
> +	SOF_CTRL_CMD_BINARY,	/**< maps to ALSA binary style controls */
> +};
> +
> +/* generic channel mapped value data */
> +struct sof_ipc_ctrl_value_chan {
> +	uint32_t channel;	/**< channel map - enum sof_ipc_chmap */
> +	uint32_t value;

Any reason to avoid s32 and u32?
If this is supposed to be shared with user-space (or user-space may
use this as a reference of data struct), we should consider placing in
uapi directory, too.


> +/* wait for IPC message reply */
> +static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,
> +			void *reply_data)
> +{
> +	struct snd_sof_dev *sdev = ipc->sdev;
> +	struct sof_ipc_cmd_hdr *hdr = (struct sof_ipc_cmd_hdr *)msg->msg_data;
> +	unsigned long flags;
> +	int ret;
> +
> +	/* wait for DSP IPC completion */
> +	ret = wait_event_timeout(msg->waitq, msg->ipc_complete,
> +				 msecs_to_jiffies(IPC_TIMEOUT_MSECS));
> +
> +	spin_lock_irqsave(&sdev->ipc_lock, flags);

Since this must be a sleepable context, you can safely use
spin_lock_irq() here.

> +/* send IPC message from host to DSP */
> +int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
> +		       void *msg_data, size_t msg_bytes, void *reply_data,
> +		       size_t reply_bytes)
> +{
> +	struct snd_sof_dev *sdev = ipc->sdev;
> +	struct snd_sof_ipc_msg *msg;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sdev->ipc_lock, flags);

Ditto.  This one calls tx_wait_done() later.

It's better to define more strictly which one can be called from the
spinlocked context and which not.


> +void snd_sof_ipc_free(struct snd_sof_dev *sdev)
> +{
> +	cancel_work_sync(&sdev->ipc->tx_kwork);
> +	cancel_work_sync(&sdev->ipc->rx_kwork);
> +}
> +EXPORT_SYMBOL(snd_sof_ipc_free);

Not specific to this function but a general question:
why not EXPORT_SYMBOL_GPL() in general in the whole SOF codes?


thanks,

Takashi