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

Takashi Iwai
2018-12-12T08:17:55Z

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