[RFC 00/13] ASoC: Intel: avs: Topology and path management

[RFC 00/13] ASoC: Intel: avs: Topology and path management

[Cezary Rojewski]

A continuation of avs-driver initial series [1]. This chapter covers
path management and topology parsing part which was ealier path of the
main series. The two patches that represented these two subjects in the
initial series, have been split into many to allow for easier review and
discussion.

AVS topology is split into two major parts: dictionaries - found within
ASoC topology manifest - and path templates - found within DAPM widget
private data. Dictionaries job is to reduce the total amount of memory
occupied by topology elements. Rather than having every pipeline and
module carry its own information, each refers to specific entry in
specific dictionary by provided (from topology file) indexes. In
consequence, most struct avs_tplg_xxx are made out of pointers.

A 'path' represents a DSP side of audio stream in runtime - is a logical
container for pipelines which are themselves composed of modules -
processing units. Due to high range of possible audio format
combinations, there can be more variants of given path (and thus, its
pipelines and modules) than total number of pipelines and module
instances which firmware supports concurrently, all the instance IDs are
allocated dynamically with help of IDA interface. 'Path templates' come
from topology file and describe a pattern which is later used to
actually create runtime 'path'.

[1]: https://lore.kernel.org/alsa-devel/20220207122108.3780926-1-cezary.rojewski@intel.com/T/#t


Cezary Rojewski (13):
  ASoC: Intel: avs: Declare vendor tokens
  ASoC: Intel: avs: Add topology parsing infrastructure
  ASoC: Intel: avs: Parse module-extension tuples
  ASoC: Intel: avs: Parse pplcfg and binding tuples
  ASoC: Intel: avs: Parse pipeline and module tuples
  ASoC: Intel: avs: Parse path and path templates tuples
  ASoC: Intel: avs: Add topology loading operations
  ASoC: Intel: avs: Declare path and its components
  ASoC: Intel: avs: Path creation and freeing
  ASoC: Intel: avs: Path state management
  ASoC: Intel: avs: Arm paths after creating them
  ASoC: Intel: avs: Prepare modules before bindings them
  ASoC: Intel: avs: Configure modules according to their type

 include/uapi/sound/intel/avs/tokens.h |  126 ++
 sound/soc/intel/Kconfig               |    2 +
 sound/soc/intel/avs/Makefile          |    3 +-
 sound/soc/intel/avs/avs.h             |   23 +
 sound/soc/intel/avs/path.c            | 1008 ++++++++++++++++
 sound/soc/intel/avs/path.h            |   72 ++
 sound/soc/intel/avs/topology.c        | 1600 +++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h        |  195 +++
 8 files changed, 3028 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/sound/intel/avs/tokens.h
 create mode 100644 sound/soc/intel/avs/path.c
 create mode 100644 sound/soc/intel/avs/path.h
 create mode 100644 sound/soc/intel/avs/topology.c
 create mode 100644 sound/soc/intel/avs/topology.h

-- 
2.25.1

[RFC 01/13] ASoC: Intel: avs: Declare vendor tokens

[Cezary Rojewski]

Expose all vendor tokens that help shape AVS topology. Parsing helpers
introduced in follow up patches make use of these to know which block
they are currently dealing with and to verify their correctness.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/uapi/sound/intel/avs/tokens.h | 126 ++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 include/uapi/sound/intel/avs/tokens.h

diff --git a/include/uapi/sound/intel/avs/tokens.h b/include/uapi/sound/intel/avs/tokens.h
new file mode 100644
index 000000000000..754f02b2f444
--- /dev/null
+++ b/include/uapi/sound/intel/avs/tokens.h
@@ -0,0 +1,126 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright(c) 2021 Intel Corporation. All rights reserved.
+ *
+ * Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+ *          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+ */
+
+#ifndef __UAPI_SOUND_INTEL_AVS_TOKENS_H
+#define __UAPI_SOUND_INTEL_AVS_TOKENS_H
+
+enum avs_tplg_token {
+	/* struct avs_tplg */
+	AVS_TKN_MANIFEST_NAME_STRING			= 1,
+	AVS_TKN_MANIFEST_VERSION_U32			= 2,
+	AVS_TKN_MANIFEST_NUM_LIBRARIES_U32		= 3,
+	AVS_TKN_MANIFEST_NUM_AFMTS_U32			= 4,
+	AVS_TKN_MANIFEST_NUM_MODCFGS_BASE_U32		= 5,
+	AVS_TKN_MANIFEST_NUM_MODCFGS_EXT_U32		= 6,
+	AVS_TKN_MANIFEST_NUM_PPLCFGS_U32		= 7,
+	AVS_TKN_MANIFEST_NUM_BINDINGS_U32		= 8,
+
+	/* struct avs_tplg_library */
+	AVS_TKN_LIBRARY_ID_U32				= 101,
+	AVS_TKN_LIBRARY_NAME_STRING			= 102,
+
+	/* struct avs_audio_format */
+	AVS_TKN_AFMT_ID_U32				= 201,
+	AVS_TKN_AFMT_SAMPLE_RATE_U32			= 202,
+	AVS_TKN_AFMT_BIT_DEPTH_U32			= 203,
+	AVS_TKN_AFMT_CHANNEL_MAP_U32			= 204,
+	AVS_TKN_AFMT_CHANNEL_CFG_U32			= 205,
+	AVS_TKN_AFMT_INTERLEAVING_U32			= 206,
+	AVS_TKN_AFMT_NUM_CHANNELS_U32			= 207,
+	AVS_TKN_AFMT_VALID_BIT_DEPTH_U32		= 208,
+	AVS_TKN_AFMT_SAMPLE_TYPE_U32			= 209,
+
+	/* struct avs_tplg_modcfg_base */
+	AVS_TKN_MODCFG_BASE_ID_U32			= 301,
+	AVS_TKN_MODCFG_BASE_CPC_U32			= 302,
+	AVS_TKN_MODCFG_BASE_IBS_U32			= 303,
+	AVS_TKN_MODCFG_BASE_OBS_U32			= 304,
+	AVS_TKN_MODCFG_BASE_PAGES_U32			= 305,
+
+	/* struct avs_tplg_modcfg_ext */
+	AVS_TKN_MODCFG_EXT_ID_U32			= 401,
+	AVS_TKN_MODCFG_EXT_TYPE_UUID			= 402,
+	AVS_TKN_MODCFG_CPR_OUT_AFMT_ID_U32		= 403,
+	AVS_TKN_MODCFG_CPR_FEATURE_MASK_U32		= 404,
+	AVS_TKN_MODCFG_CPR_DMA_TYPE_U32			= 405,
+	AVS_TKN_MODCFG_CPR_DMABUFF_SIZE_U32		= 406,
+	AVS_TKN_MODCFG_CPR_VINDEX_U8			= 407,
+	AVS_TKN_MODCFG_CPR_BLOB_FMT_ID_U32		= 408,
+	AVS_TKN_MODCFG_MICSEL_OUT_AFMT_ID_U32		= 409,
+	AVS_TKN_MODCFG_INTELWOV_CPC_LP_MODE_U32		= 410,
+	AVS_TKN_MODCFG_SRC_OUT_FREQ_U32			= 411,
+	AVS_TKN_MODCFG_MUX_REF_AFMT_ID_U32		= 412,
+	AVS_TKN_MODCFG_MUX_OUT_AFMT_ID_U32		= 413,
+	AVS_TKN_MODCFG_AEC_REF_AFMT_ID_U32		= 414,
+	AVS_TKN_MODCFG_AEC_OUT_AFMT_ID_U32		= 415,
+	AVS_TKN_MODCFG_AEC_CPC_LP_MODE_U32		= 416,
+	AVS_TKN_MODCFG_ASRC_OUT_FREQ_U32		= 417,
+	AVS_TKN_MODCFG_ASRC_MODE_U8			= 418,
+	AVS_TKN_MODCFG_ASRC_DISABLE_JITTER_U8		= 419,
+	AVS_TKN_MODCFG_UPDOWN_MIX_OUT_CHAN_CFG_U32	= 420,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_SELECT_U32	= 421,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_0_S32		= 422,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_1_S32		= 423,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_2_S32		= 424,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_3_S32		= 425,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_4_S32		= 426,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_5_S32		= 427,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_6_S32		= 428,
+	AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_7_S32		= 429,
+	AVS_TKN_MODCFG_UPDOWN_MIX_CHAN_MAP_U32		= 430,
+	AVS_TKN_MODCFG_EXT_NUM_INPUT_PINS_U16		= 431,
+	AVS_TKN_MODCFG_EXT_NUM_OUTPUT_PINS_U16		= 432,
+
+	/* struct avs_tplg_pplcfg */
+	AVS_TKN_PPLCFG_ID_U32				= 1401,
+	AVS_TKN_PPLCFG_REQ_SIZE_U16			= 1402,
+	AVS_TKN_PPLCFG_PRIORITY_U8			= 1403,
+	AVS_TKN_PPLCFG_LOW_POWER_BOOL			= 1404,
+	AVS_TKN_PPLCFG_ATTRIBUTES_U16			= 1405,
+	AVS_TKN_PPLCFG_TRIGGER_U32			= 1406,
+
+	/* struct avs_tplg_binding */
+	AVS_TKN_BINDING_ID_U32				= 1501,
+	AVS_TKN_BINDING_TARGET_TPLG_NAME_STRING		= 1502,
+	AVS_TKN_BINDING_TARGET_PATH_TMPL_ID_U32		= 1503,
+	AVS_TKN_BINDING_TARGET_PPL_ID_U32		= 1504,
+	AVS_TKN_BINDING_TARGET_MOD_ID_U32		= 1505,
+	AVS_TKN_BINDING_TARGET_MOD_PIN_U8		= 1506,
+	AVS_TKN_BINDING_MOD_ID_U32			= 1507,
+	AVS_TKN_BINDING_MOD_PIN_U8			= 1508,
+	AVS_TKN_BINDING_IS_SINK_U8			= 1509,
+
+	/* struct avs_tplg_pipeline */
+	AVS_TKN_PPL_ID_U32				= 1601,
+	AVS_TKN_PPL_PPLCFG_ID_U32			= 1602,
+	AVS_TKN_PPL_NUM_BINDING_IDS_U32			= 1603,
+	AVS_TKN_PPL_BINDING_ID_U32			= 1604,
+
+	/* struct avs_tplg_module */
+	AVS_TKN_MOD_ID_U32				= 1701,
+	AVS_TKN_MOD_MODCFG_BASE_ID_U32			= 1702,
+	AVS_TKN_MOD_IN_AFMT_ID_U32			= 1703,
+	AVS_TKN_MOD_CORE_ID_U8				= 1704,
+	AVS_TKN_MOD_PROC_DOMAIN_U8			= 1705,
+	AVS_TKN_MOD_MODCFG_EXT_ID_U32			= 1706,
+
+	/* struct avs_tplg_path_template */
+	AVS_TKN_PATH_TMPL_ID_U32			= 1801,
+
+	/* struct avs_tplg_path */
+	AVS_TKN_PATH_ID_U32				= 1901,
+	AVS_TKN_PATH_FE_FMT_ID_U32			= 1902,
+	AVS_TKN_PATH_BE_FMT_ID_U32			= 1903,
+
+	/* struct avs_tplg_pin_format */
+	AVS_TKN_PIN_FMT_INDEX_U32			= 2201,
+	AVS_TKN_PIN_FMT_IOBS_U32			= 2202,
+	AVS_TKN_PIN_FMT_AFMT_ID_U32			= 2203,
+};
+
+#endif
-- 
2.25.1

[RFC 02/13] ASoC: Intel: avs: Add topology parsing infrastructure

[Cezary Rojewski]

AVS topology is split into two major parts: dictionaries - found within
ASoC topology manifest - and path templates - found within DAPM widget
private data. Dictionaries job is to reduce the total amount of memory
occupied by topology elements. Rather than having every pipeline and
module carry its own information, each refers to specific entry in
specific dictionary by provided (from topology file) indexes. In
consequence, most struct avs_tplg_xxx are made out of pointers.

To support the above, range of parsing helpers for all value-types known
to ALSA: uuid, bool, byte, short, word and string are added. Additional
handlers help translate pointer-types and more complex objects such as
audio formats and module base configs.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/avs.h      |  14 +
 sound/soc/intel/avs/topology.c | 595 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |  44 +++
 3 files changed, 653 insertions(+)
 create mode 100644 sound/soc/intel/avs/topology.c
 create mode 100644 sound/soc/intel/avs/topology.h

diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index 20987c7744a3..61842720c894 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -13,10 +13,12 @@
 #include <linux/firmware.h>
 #include <sound/hda_codec.h>
 #include <sound/hda_register.h>
+#include <sound/soc-component.h>
 #include "messages.h"
 #include "registers.h"
 
 struct avs_dev;
+struct avs_tplg;
 
 struct avs_dsp_ops {
 	int (* const power)(struct avs_dev *, u32, bool);
@@ -223,4 +225,16 @@ int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib, u32 id);
 int avs_hda_transfer_modules(struct avs_dev *adev, bool load,
 			     struct avs_module_entry *mods, u32 num_mods);
 
+/* Soc component members */
+
+struct avs_soc_component {
+	struct snd_soc_component base;
+	struct avs_tplg *tplg;
+
+	struct list_head node;
+};
+
+#define to_avs_soc_component(comp) \
+	container_of(comp, struct avs_soc_component, base)
+
 #endif /* __SOUND_SOC_INTEL_AVS_H */
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
new file mode 100644
index 000000000000..4b8b415ca006
--- /dev/null
+++ b/sound/soc/intel/avs/topology.c
@@ -0,0 +1,595 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2021 Intel Corporation. All rights reserved.
+//
+// Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+//          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+//
+
+#include <linux/uuid.h>
+#include <sound/soc.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc-topology.h>
+#include <uapi/sound/intel/avs/tokens.h>
+#include "avs.h"
+#include "topology.h"
+
+/* Get pointer to vendor array at the specified offset. */
+#define avs_tplg_vendor_array_at(array, offset) \
+	((struct snd_soc_tplg_vendor_array *)((u8 *)array + offset))
+
+/* Get pointer to vendor array that is next in line. */
+#define avs_tplg_vendor_array_next(array) \
+	(avs_tplg_vendor_array_at(array, le32_to_cpu((array)->size)))
+
+/*
+ * Scan provided block of tuples for the specified token. If found,
+ * @offset is updated with position at which first matching token is
+ * located.
+ *
+ * Returns 0 on success, -ENOENT if not found and error code otherwise.
+ */
+static int
+avs_tplg_vendor_array_lookup(struct snd_soc_tplg_vendor_array *tuples,
+			     u32 block_size, u32 token, u32 *offset)
+{
+	u32 pos = 0;
+
+	while (block_size > 0) {
+		struct snd_soc_tplg_vendor_value_elem *tuple;
+		u32 tuples_size = le32_to_cpu(tuples->size);
+
+		if (tuples_size > block_size)
+			return -EINVAL;
+
+		tuple = tuples->value;
+		if (le32_to_cpu(tuple->token) == token) {
+			*offset = pos;
+			return 0;
+		}
+
+		block_size -= tuples_size;
+		pos += tuples_size;
+		tuples = avs_tplg_vendor_array_next(tuples);
+	}
+
+	return -ENOENT;
+}
+
+/*
+ * See avs_tplg_vendor_array_lookup() for description.
+ *
+ * Behaves exactly like its precursor but starts from the next vendor
+ * array in line. Useful when searching for the finish line of an
+ * arbitrary entry in a list of entries where each is composed of
+ * several vendor tuples and a specific token marks the beginning of
+ * a new entry block.
+ */
+static int
+avs_tplg_vendor_array_lookup_next(struct snd_soc_tplg_vendor_array *tuples,
+				  u32 block_size, u32 token, u32 *offset)
+{
+	u32 tuples_size = le32_to_cpu(tuples->size);
+	int ret;
+
+	if (tuples_size > block_size)
+		return -EINVAL;
+
+	tuples = avs_tplg_vendor_array_next(tuples);
+	block_size -= tuples_size;
+
+	ret = avs_tplg_vendor_array_lookup(tuples, block_size, token, offset);
+	if (!ret)
+		*offset += tuples_size;
+	return ret;
+}
+
+/*
+ * Scan provided block of tuples for the specified token which marks
+ * the boarder of an entry block. Behavior is similar to
+ * avs_tplg_vendor_array_lookup() except 0 is also returned if no
+ * matching token has been found. In such case, returned @size is
+ * assigned to @block_size as the entire block belongs to the current
+ * entry.
+ *
+ * Returns 0 on success, error code otherwise.
+ */
+static int
+avs_tplg_vendor_entry_size(struct snd_soc_tplg_vendor_array *tuples,
+			   u32 block_size, u32 entry_id_token, u32 *size)
+{
+	int ret;
+
+	ret = avs_tplg_vendor_array_lookup_next(tuples, block_size, entry_id_token, size);
+	if (ret == -ENOENT) {
+		*size = block_size;
+		ret = 0;
+	}
+
+	return ret;
+}
+
+/*
+ * Vendor tuple parsing descriptor.
+ *
+ * @token: vendor specific token that identifies tuple
+ * @type: tuple type, one of SND_SOC_TPLG_TUPLE_TYPE_XXX
+ * @offset: offset of a struct's field to initialize
+ * @parse: parsing function, extracts and assigns value to object's field
+ */
+struct avs_tplg_token_parser {
+	enum avs_tplg_token token;
+	u32 type;
+	u32 offset;
+	int (*parse)(struct snd_soc_component *comp, void *elem, void *object, u32 offset);
+};
+
+static int
+avs_parse_uuid_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
+	guid_t *val = (guid_t *)((u8 *)object + offset);
+
+	guid_copy((guid_t *)val, (const guid_t *)&tuple->value);
+
+	return 0;
+}
+
+static int
+avs_parse_bool_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
+	bool *val = (bool *)((u8 *)object + offset);
+
+	*val = le32_to_cpu(tuple->value);
+
+	return 0;
+}
+
+static int
+avs_parse_byte_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
+	u8 *val = ((u8 *)object + offset);
+
+	*val = le32_to_cpu(tuple->value);
+
+	return 0;
+}
+
+static int
+avs_parse_short_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
+	u16 *val = (u16 *)((u8 *)object + offset);
+
+	*val = le32_to_cpu(tuple->value);
+
+	return 0;
+}
+
+static int
+avs_parse_word_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
+	u32 *val = (u32 *)((u8 *)object + offset);
+
+	*val = le32_to_cpu(tuple->value);
+
+	return 0;
+}
+
+static int
+avs_parse_string_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_string_elem *tuple = elem;
+	char *val = (char *)((u8 *)object + offset);
+
+	snprintf(val, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s", tuple->string);
+
+	return 0;
+}
+
+static int avs_parse_uuid_tokens(struct snd_soc_component *comp, void *object,
+				 const struct avs_tplg_token_parser *parsers, int count,
+				 struct snd_soc_tplg_vendor_array *tuples)
+{
+	struct snd_soc_tplg_vendor_uuid_elem *tuple;
+	int ret, i, j;
+
+	/* Parse element by element. */
+	for (i = 0; i < le32_to_cpu(tuples->num_elems); i++) {
+		tuple = &tuples->uuid[i];
+
+		for (j = 0; j < count; j++) {
+			/* Ignore non-UUID tokens. */
+			if (parsers[j].type != SND_SOC_TPLG_TUPLE_TYPE_UUID ||
+			    parsers[j].token != le32_to_cpu(tuple->token))
+				continue;
+
+			ret = parsers[j].parse(comp, tuple, object, parsers[j].offset);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int avs_parse_string_tokens(struct snd_soc_component *comp, void *object,
+				   const struct avs_tplg_token_parser *parsers, int count,
+				   struct snd_soc_tplg_vendor_array *tuples)
+{
+	struct snd_soc_tplg_vendor_string_elem *tuple;
+	int ret, i, j;
+
+	/* Parse element by element. */
+	for (i = 0; i < le32_to_cpu(tuples->num_elems); i++) {
+		tuple = &tuples->string[i];
+
+		for (j = 0; j < count; j++) {
+			/* Ignore non-string tokens. */
+			if (parsers[j].type != SND_SOC_TPLG_TUPLE_TYPE_STRING ||
+			    parsers[j].token != le32_to_cpu(tuple->token))
+				continue;
+
+			ret = parsers[j].parse(comp, tuple, object, parsers[j].offset);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int avs_parse_word_tokens(struct snd_soc_component *comp, void *object,
+				 const struct avs_tplg_token_parser *parsers, int count,
+				 struct snd_soc_tplg_vendor_array *tuples)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple;
+	int ret, i, j;
+
+	/* Parse element by element. */
+	for (i = 0; i < le32_to_cpu(tuples->num_elems); i++) {
+		tuple = &tuples->value[i];
+
+		for (j = 0; j < count; j++) {
+			/* Ignore non-integer tokens. */
+			if (!(parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_WORD ||
+			      parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_SHORT ||
+			      parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_BYTE ||
+			      parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_BOOL))
+				continue;
+
+			if (parsers[j].token != le32_to_cpu(tuple->token))
+				continue;
+
+			ret = parsers[j].parse(comp, tuple, object, parsers[j].offset);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int avs_parse_tokens(struct snd_soc_component *comp, void *object,
+			    const struct avs_tplg_token_parser *parsers, size_t count,
+			    struct snd_soc_tplg_vendor_array *tuples, int priv_size)
+{
+	int array_size, ret;
+
+	while (priv_size > 0) {
+		array_size = le32_to_cpu(tuples->size);
+
+		if (array_size <= 0) {
+			dev_err(comp->dev, "invalid array size 0x%x\n", array_size);
+			return -EINVAL;
+		}
+
+		/* Make sure there is enough data before parsing. */
+		priv_size -= array_size;
+		if (priv_size < 0) {
+			dev_err(comp->dev, "invalid array size 0x%x\n", array_size);
+			return -EINVAL;
+		}
+
+		switch (le32_to_cpu(tuples->type)) {
+		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
+			ret = avs_parse_uuid_tokens(comp, object, parsers, count, tuples);
+			break;
+		case SND_SOC_TPLG_TUPLE_TYPE_STRING:
+			ret = avs_parse_string_tokens(comp, object, parsers, count, tuples);
+			break;
+		case SND_SOC_TPLG_TUPLE_TYPE_BOOL:
+		case SND_SOC_TPLG_TUPLE_TYPE_BYTE:
+		case SND_SOC_TPLG_TUPLE_TYPE_SHORT:
+		case SND_SOC_TPLG_TUPLE_TYPE_WORD:
+			ret = avs_parse_word_tokens(comp, object, parsers, count, tuples);
+			break;
+		default:
+			dev_err(comp->dev, "unknown token type %d\n", tuples->type);
+			ret = -EINVAL;
+		}
+
+		if (ret) {
+			dev_err(comp->dev, "parsing %ld tokens of %d type failed: %d\n",
+				count, tuples->type, ret);
+			return ret;
+		}
+
+		tuples = avs_tplg_vendor_array_next(tuples);
+	}
+
+	return 0;
+}
+
+#define AVS_DEFINE_PTR_PARSER(name, type, member) \
+static int \
+avs_parse_##name##_ptr(struct snd_soc_component *comp, void *elem, void *object, u32 offset) \
+{ \
+	struct snd_soc_tplg_vendor_value_elem *tuple = elem;		\
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);	\
+	type **val = (type **)(object + offset);			\
+	u32 idx;							\
+									\
+	idx = le32_to_cpu(tuple->value);				\
+	if (idx >= acomp->tplg->num_##member)				\
+		return -EINVAL;						\
+									\
+	*val = &acomp->tplg->member[idx];				\
+									\
+	return 0;							\
+}
+
+AVS_DEFINE_PTR_PARSER(audio_format, struct avs_audio_format, fmts);
+AVS_DEFINE_PTR_PARSER(modcfg_base, struct avs_tplg_modcfg_base, modcfgs_base);
+
+static int
+parse_audio_format_bitfield(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_value_elem *velem = elem;
+	struct avs_audio_format *audio_format = object;
+
+	switch (offset) {
+	case AVS_TKN_AFMT_NUM_CHANNELS_U32:
+		audio_format->num_channels = le32_to_cpu(velem->value);
+		break;
+	case AVS_TKN_AFMT_VALID_BIT_DEPTH_U32:
+		audio_format->valid_bit_depth = le32_to_cpu(velem->value);
+		break;
+	case AVS_TKN_AFMT_SAMPLE_TYPE_U32:
+		audio_format->sample_type = le32_to_cpu(velem->value);
+		break;
+	}
+
+	return 0;
+}
+
+static int parse_link_formatted_string(struct snd_soc_component *comp, void *elem,
+				       void *object, u32 offset)
+{
+	struct snd_soc_tplg_vendor_string_elem *tuple = elem;
+	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
+	char *val = (char *)((u8 *)object + offset);
+
+	/*
+	 * Dynamic naming - string formats, e.g.: ssp%d - supported only for
+	 * topologies describing single device e.g.: an I2S codec on SSP0.
+	 */
+	if (hweight_long(mach->link_mask) != 1)
+		return avs_parse_string_token(comp, elem, object, offset);
+
+	snprintf(val, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, tuple->string,
+		 __ffs(mach->link_mask));
+
+	return 0;
+}
+
+static int
+parse_dictionary_header(struct snd_soc_component *comp,
+			struct snd_soc_tplg_vendor_array *tuples,
+			void **dict, u32 *num_entries, size_t entry_size,
+			u32 num_entries_token)
+{
+	struct snd_soc_tplg_vendor_value_elem *tuple;
+
+	/* Dictionary header consists of single tuple - entry count. */
+	tuple = tuples->value;
+	if (le32_to_cpu(tuple->token) != num_entries_token) {
+		dev_err(comp->dev, "invalid dictionary header, expected: %d\n",
+			num_entries_token);
+		return -EINVAL;
+	}
+
+	*num_entries = le32_to_cpu(tuple->value);
+	*dict = devm_kcalloc(comp->card->dev, *num_entries, entry_size, GFP_KERNEL);
+	if (!*dict)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int
+parse_dictionary_entries(struct snd_soc_component *comp,
+			 struct snd_soc_tplg_vendor_array *tuples, u32 block_size,
+			 void *dict, u32 num_entries, size_t entry_size,
+			 u32 entry_id_token,
+			 const struct avs_tplg_token_parser *parsers, size_t num_parsers)
+{
+	void *pos = dict;
+	int i;
+
+	for (i = 0; i < num_entries; i++) {
+		u32 esize;
+		int ret;
+
+		ret = avs_tplg_vendor_entry_size(tuples, block_size,
+						 entry_id_token, &esize);
+		if (ret)
+			return ret;
+
+		ret = avs_parse_tokens(comp, pos, parsers, num_parsers, tuples, esize);
+		if (ret < 0) {
+			dev_err(comp->dev, "parse entry: %d of type: %d failed: %d\n",
+				i, entry_id_token, ret);
+			return ret;
+		}
+
+		pos += entry_size;
+		block_size -= esize;
+		tuples = avs_tplg_vendor_array_at(tuples, esize);
+	}
+
+	return 0;
+}
+
+static int parse_dictionary(struct snd_soc_component *comp,
+			    struct snd_soc_tplg_vendor_array *tuples, u32 block_size,
+			    void **dict, u32 *num_entries, size_t entry_size,
+			    u32 num_entries_token, u32 entry_id_token,
+			    const struct avs_tplg_token_parser *parsers, size_t num_parsers)
+{
+	int ret;
+
+	ret = parse_dictionary_header(comp, tuples, dict, num_entries,
+				      entry_size, num_entries_token);
+	if (ret)
+		return ret;
+
+	block_size -= le32_to_cpu(tuples->size);
+	/* With header parsed, move on to parsing entries. */
+	tuples = avs_tplg_vendor_array_next(tuples);
+
+	return parse_dictionary_entries(comp, tuples, block_size, *dict,
+					*num_entries, entry_size,
+					entry_id_token, parsers, num_parsers);
+}
+
+static const struct avs_tplg_token_parser library_parsers[] = {
+	{
+		.token = AVS_TKN_LIBRARY_NAME_STRING,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_STRING,
+		.offset = offsetof(struct avs_tplg_library, name),
+		.parse = avs_parse_string_token,
+	},
+};
+
+static int avs_tplg_parse_libraries(struct snd_soc_component *comp,
+				    struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
+{
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg = acomp->tplg;
+
+	return parse_dictionary(comp, tuples, block_size, (void **)&tplg->libs,
+				&tplg->num_libs, sizeof(*tplg->libs),
+				AVS_TKN_MANIFEST_NUM_LIBRARIES_U32,
+				AVS_TKN_LIBRARY_ID_U32,
+				library_parsers, ARRAY_SIZE(library_parsers));
+}
+
+static const struct avs_tplg_token_parser audio_format_parsers[] = {
+	{
+		.token = AVS_TKN_AFMT_SAMPLE_RATE_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_audio_format, sampling_freq),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_AFMT_BIT_DEPTH_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_audio_format, bit_depth),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_AFMT_CHANNEL_MAP_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_audio_format, channel_map),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_AFMT_CHANNEL_CFG_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_audio_format, channel_config),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_AFMT_INTERLEAVING_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_audio_format, interleaving),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_AFMT_NUM_CHANNELS_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = AVS_TKN_AFMT_NUM_CHANNELS_U32,
+		.parse = parse_audio_format_bitfield,
+	},
+	{
+		.token = AVS_TKN_AFMT_VALID_BIT_DEPTH_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = AVS_TKN_AFMT_VALID_BIT_DEPTH_U32,
+		.parse = parse_audio_format_bitfield,
+	},
+	{
+		.token = AVS_TKN_AFMT_SAMPLE_TYPE_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = AVS_TKN_AFMT_SAMPLE_TYPE_U32,
+		.parse = parse_audio_format_bitfield,
+	},
+};
+
+static int avs_tplg_parse_audio_formats(struct snd_soc_component *comp,
+					struct snd_soc_tplg_vendor_array *tuples,
+					u32 block_size)
+{
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg = acomp->tplg;
+
+	return parse_dictionary(comp, tuples, block_size, (void **)&tplg->fmts,
+				&tplg->num_fmts, sizeof(*tplg->fmts),
+				AVS_TKN_MANIFEST_NUM_AFMTS_U32,
+				AVS_TKN_AFMT_ID_U32,
+				audio_format_parsers, ARRAY_SIZE(audio_format_parsers));
+}
+
+static const struct avs_tplg_token_parser modcfg_base_parsers[] = {
+	{
+		.token = AVS_TKN_MODCFG_BASE_CPC_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_base, cpc),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_BASE_IBS_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_base, ibs),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_BASE_OBS_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_base, obs),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_BASE_PAGES_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_base, is_pages),
+		.parse = avs_parse_word_token,
+	},
+};
+
+static int avs_tplg_parse_modcfgs_base(struct snd_soc_component *comp,
+				       struct snd_soc_tplg_vendor_array *tuples,
+				       u32 block_size)
+{
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg = acomp->tplg;
+
+	return parse_dictionary(comp, tuples, block_size, (void **)&tplg->modcfgs_base,
+				&tplg->num_modcfgs_base, sizeof(*tplg->modcfgs_base),
+				AVS_TKN_MANIFEST_NUM_MODCFGS_BASE_U32,
+				AVS_TKN_MODCFG_BASE_ID_U32,
+				modcfg_base_parsers, ARRAY_SIZE(modcfg_base_parsers));
+}
diff --git a/sound/soc/intel/avs/topology.h b/sound/soc/intel/avs/topology.h
new file mode 100644
index 000000000000..a3ab5d15c9ee
--- /dev/null
+++ b/sound/soc/intel/avs/topology.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright(c) 2021 Intel Corporation. All rights reserved.
+ *
+ * Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+ *          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+ */
+
+#ifndef __SOUND_SOC_INTEL_AVS_TPLG_H
+#define __SOUND_SOC_INTEL_AVS_TPLG_H
+
+#include <linux/list.h>
+#include "messages.h"
+
+#define INVALID_OBJECT_ID	UINT_MAX
+
+struct snd_soc_component;
+
+struct avs_tplg {
+	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+	u32 version;
+	struct snd_soc_component *comp;
+
+	struct avs_tplg_library *libs;
+	u32 num_libs;
+	struct avs_audio_format *fmts;
+	u32 num_fmts;
+	struct avs_tplg_modcfg_base *modcfgs_base;
+	u32 num_modcfgs_base;
+};
+
+struct avs_tplg_library {
+	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+};
+
+/* Matches header of struct avs_mod_cfg_base. */
+struct avs_tplg_modcfg_base {
+	u32 cpc;
+	u32 ibs;
+	u32 obs;
+	u32 is_pages;
+};
+
+#endif
-- 
2.25.1

Re: [RFC 02/13] ASoC: Intel: avs: Add topology parsing infrastructure

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> AVS topology is split into two major parts: dictionaries - found within
> ASoC topology manifest - and path templates - found within DAPM widget

what is a "path template"? this is the third time I review your patches
and I have yet to find a description of all this.

If you introduce a new concept you really need to explain what problem
you are trying to solve, why it's important and what other alternatives
could be considered. Consider adding a Documentation file to explain
what you are trying to accomplish.

Jumping to optimizations of memory footprint through dictionaries is too
early.

> private data. Dictionaries job is to reduce the total amount of memory
> occupied by topology elements. Rather than having every pipeline and
> module carry its own information, each refers to specific entry in
> specific dictionary by provided (from topology file) indexes. In
> consequence, most struct avs_tplg_xxx are made out of pointers.
> To support the above, range of parsing helpers for all value-types known
> to ALSA: uuid, bool, byte, short, word and string are added. Additional
> handlers help translate pointer-types and more complex objects such as
> audio formats and module base configs.
> 
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/intel/avs/avs.h      |  14 +
>  sound/soc/intel/avs/topology.c | 595 +++++++++++++++++++++++++++++++++
>  sound/soc/intel/avs/topology.h |  44 +++
>  3 files changed, 653 insertions(+)
>  create mode 100644 sound/soc/intel/avs/topology.c
>  create mode 100644 sound/soc/intel/avs/topology.h
> 
> diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
> index 20987c7744a3..61842720c894 100644
> --- a/sound/soc/intel/avs/avs.h
> +++ b/sound/soc/intel/avs/avs.h
> @@ -13,10 +13,12 @@
>  #include <linux/firmware.h>
>  #include <sound/hda_codec.h>
>  #include <sound/hda_register.h>
> +#include <sound/soc-component.h>
>  #include "messages.h"
>  #include "registers.h"
>  
>  struct avs_dev;
> +struct avs_tplg;
>  
>  struct avs_dsp_ops {
>  	int (* const power)(struct avs_dev *, u32, bool);
> @@ -223,4 +225,16 @@ int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib, u32 id);
>  int avs_hda_transfer_modules(struct avs_dev *adev, bool load,
>  			     struct avs_module_entry *mods, u32 num_mods);
>  
> +/* Soc component members */
> +
> +struct avs_soc_component {
> +	struct snd_soc_component base;
> +	struct avs_tplg *tplg;
> +
> +	struct list_head node;
> +};
> +
> +#define to_avs_soc_component(comp) \
> +	container_of(comp, struct avs_soc_component, base)
> +
>  #endif /* __SOUND_SOC_INTEL_AVS_H */
> diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
> new file mode 100644
> index 000000000000..4b8b415ca006
> --- /dev/null
> +++ b/sound/soc/intel/avs/topology.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright(c) 2021 Intel Corporation. All rights reserved.
> +//
> +// Authors: Cezary Rojewski <cezary.rojewski@intel.com>
> +//          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
> +//
> +
> +#include <linux/uuid.h>
> +#include <sound/soc.h>
> +#include <sound/soc-acpi.h>
> +#include <sound/soc-topology.h>
> +#include <uapi/sound/intel/avs/tokens.h>
> +#include "avs.h"
> +#include "topology.h"
> +
> +/* Get pointer to vendor array at the specified offset. */
> +#define avs_tplg_vendor_array_at(array, offset) \
> +	((struct snd_soc_tplg_vendor_array *)((u8 *)array + offset))
> +
> +/* Get pointer to vendor array that is next in line. */
> +#define avs_tplg_vendor_array_next(array) \
> +	(avs_tplg_vendor_array_at(array, le32_to_cpu((array)->size)))
> +
> +/*
> + * Scan provided block of tuples for the specified token. If found,
> + * @offset is updated with position at which first matching token is
> + * located.
> + *
> + * Returns 0 on success, -ENOENT if not found and error code otherwise.
> + */
> +static int
> +avs_tplg_vendor_array_lookup(struct snd_soc_tplg_vendor_array *tuples,
> +			     u32 block_size, u32 token, u32 *offset)
> +{
> +	u32 pos = 0;
> +
> +	while (block_size > 0) {
> +		struct snd_soc_tplg_vendor_value_elem *tuple;
> +		u32 tuples_size = le32_to_cpu(tuples->size);
> +
> +		if (tuples_size > block_size)
> +			return -EINVAL;
> +
> +		tuple = tuples->value;
> +		if (le32_to_cpu(tuple->token) == token) {
> +			*offset = pos;
> +			return 0;
> +		}
> +
> +		block_size -= tuples_size;
> +		pos += tuples_size;
> +		tuples = avs_tplg_vendor_array_next(tuples);
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +/*
> + * See avs_tplg_vendor_array_lookup() for description.
> + *
> + * Behaves exactly like its precursor but starts from the next vendor
> + * array in line. Useful when searching for the finish line of an
> + * arbitrary entry in a list of entries where each is composed of
> + * several vendor tuples and a specific token marks the beginning of
> + * a new entry block.

please try and reword such comments for people who didn't take part in
the development.

I really have no idea what this is about.

> + */
> +static int
> +avs_tplg_vendor_array_lookup_next(struct snd_soc_tplg_vendor_array *tuples,
> +				  u32 block_size, u32 token, u32 *offset)
> +{
> +	u32 tuples_size = le32_to_cpu(tuples->size);
> +	int ret;
> +
> +	if (tuples_size > block_size)
> +		return -EINVAL;
> +
> +	tuples = avs_tplg_vendor_array_next(tuples);
> +	block_size -= tuples_size;
> +
> +	ret = avs_tplg_vendor_array_lookup(tuples, block_size, token, offset);
> +	if (!ret)
> +		*offset += tuples_size;
> +	return ret;
> +}
> +
> +/*
> + * Scan provided block of tuples for the specified token which marks
> + * the boarder of an entry block. Behavior is similar to

boarder looks like a typo. Did you mean border? boundary? position?
location?

> + * avs_tplg_vendor_array_lookup() except 0 is also returned if no
> + * matching token has been found. In such case, returned @size is
> + * assigned to @block_size as the entire block belongs to the current
> + * entry.
> + *
> + * Returns 0 on success, error code otherwise.
> + */
> +static int
> +avs_tplg_vendor_entry_size(struct snd_soc_tplg_vendor_array *tuples,
> +			   u32 block_size, u32 entry_id_token, u32 *size)
> +{
> +	int ret;
> +
> +	ret = avs_tplg_vendor_array_lookup_next(tuples, block_size, entry_id_token, size);
> +	if (ret == -ENOENT) {
> +		*size = block_size;
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Vendor tuple parsing descriptor.
> + *
> + * @token: vendor specific token that identifies tuple
> + * @type: tuple type, one of SND_SOC_TPLG_TUPLE_TYPE_XXX
> + * @offset: offset of a struct's field to initialize
> + * @parse: parsing function, extracts and assigns value to object's field
> + */
> +struct avs_tplg_token_parser {
> +	enum avs_tplg_token token;
> +	u32 type;
> +	u32 offset;
> +	int (*parse)(struct snd_soc_component *comp, void *elem, void *object, u32 offset);
> +};
> +
> +static int
> +avs_parse_uuid_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
> +	guid_t *val = (guid_t *)((u8 *)object + offset);
> +
> +	guid_copy((guid_t *)val, (const guid_t *)&tuple->value);
> +
> +	return 0;
> +}
> +
> +static int
> +avs_parse_bool_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
> +	bool *val = (bool *)((u8 *)object + offset);
> +
> +	*val = le32_to_cpu(tuple->value);
> +
> +	return 0;
> +}
> +
> +static int
> +avs_parse_byte_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
> +	u8 *val = ((u8 *)object + offset);
> +
> +	*val = le32_to_cpu(tuple->value);
> +
> +	return 0;
> +}
> +
> +static int
> +avs_parse_short_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
> +	u16 *val = (u16 *)((u8 *)object + offset);
> +
> +	*val = le32_to_cpu(tuple->value);
> +
> +	return 0;
> +}
> +
> +static int
> +avs_parse_word_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_value_elem *tuple = elem;
> +	u32 *val = (u32 *)((u8 *)object + offset);
> +
> +	*val = le32_to_cpu(tuple->value);
> +
> +	return 0;
> +}
> +
> +static int
> +avs_parse_string_token(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_string_elem *tuple = elem;
> +	char *val = (char *)((u8 *)object + offset);
> +
> +	snprintf(val, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s", tuple->string);
> +
> +	return 0;
> +}
> +
> +static int avs_parse_uuid_tokens(struct snd_soc_component *comp, void *object,
> +				 const struct avs_tplg_token_parser *parsers, int count,
> +				 struct snd_soc_tplg_vendor_array *tuples)
> +{
> +	struct snd_soc_tplg_vendor_uuid_elem *tuple;
> +	int ret, i, j;
> +
> +	/* Parse element by element. */
> +	for (i = 0; i < le32_to_cpu(tuples->num_elems); i++) {
> +		tuple = &tuples->uuid[i];
> +
> +		for (j = 0; j < count; j++) {
> +			/* Ignore non-UUID tokens. */
> +			if (parsers[j].type != SND_SOC_TPLG_TUPLE_TYPE_UUID ||
> +			    parsers[j].token != le32_to_cpu(tuple->token))
> +				continue;
> +
> +			ret = parsers[j].parse(comp, tuple, object, parsers[j].offset);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int avs_parse_string_tokens(struct snd_soc_component *comp, void *object,
> +				   const struct avs_tplg_token_parser *parsers, int count,
> +				   struct snd_soc_tplg_vendor_array *tuples)
> +{
> +	struct snd_soc_tplg_vendor_string_elem *tuple;
> +	int ret, i, j;
> +
> +	/* Parse element by element. */
> +	for (i = 0; i < le32_to_cpu(tuples->num_elems); i++) {
> +		tuple = &tuples->string[i];
> +
> +		for (j = 0; j < count; j++) {
> +			/* Ignore non-string tokens. */
> +			if (parsers[j].type != SND_SOC_TPLG_TUPLE_TYPE_STRING ||
> +			    parsers[j].token != le32_to_cpu(tuple->token))
> +				continue;
> +
> +			ret = parsers[j].parse(comp, tuple, object, parsers[j].offset);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int avs_parse_word_tokens(struct snd_soc_component *comp, void *object,
> +				 const struct avs_tplg_token_parser *parsers, int count,
> +				 struct snd_soc_tplg_vendor_array *tuples)
> +{
> +	struct snd_soc_tplg_vendor_value_elem *tuple;
> +	int ret, i, j;
> +
> +	/* Parse element by element. */
> +	for (i = 0; i < le32_to_cpu(tuples->num_elems); i++) {
> +		tuple = &tuples->value[i];
> +
> +		for (j = 0; j < count; j++) {
> +			/* Ignore non-integer tokens. */
> +			if (!(parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_WORD ||
> +			      parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_SHORT ||
> +			      parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_BYTE ||
> +			      parsers[j].type == SND_SOC_TPLG_TUPLE_TYPE_BOOL))
> +				continue;
> +
> +			if (parsers[j].token != le32_to_cpu(tuple->token))
> +				continue;
> +
> +			ret = parsers[j].parse(comp, tuple, object, parsers[j].offset);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int avs_parse_tokens(struct snd_soc_component *comp, void *object,
> +			    const struct avs_tplg_token_parser *parsers, size_t count,
> +			    struct snd_soc_tplg_vendor_array *tuples, int priv_size)
> +{
> +	int array_size, ret;
> +
> +	while (priv_size > 0) {
> +		array_size = le32_to_cpu(tuples->size);
> +
> +		if (array_size <= 0) {
> +			dev_err(comp->dev, "invalid array size 0x%x\n", array_size);
> +			return -EINVAL;
> +		}
> +
> +		/* Make sure there is enough data before parsing. */
> +		priv_size -= array_size;
> +		if (priv_size < 0) {
> +			dev_err(comp->dev, "invalid array size 0x%x\n", array_size);
> +			return -EINVAL;
> +		}
> +
> +		switch (le32_to_cpu(tuples->type)) {
> +		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
> +			ret = avs_parse_uuid_tokens(comp, object, parsers, count, tuples);
> +			break;
> +		case SND_SOC_TPLG_TUPLE_TYPE_STRING:
> +			ret = avs_parse_string_tokens(comp, object, parsers, count, tuples);
> +			break;
> +		case SND_SOC_TPLG_TUPLE_TYPE_BOOL:
> +		case SND_SOC_TPLG_TUPLE_TYPE_BYTE:
> +		case SND_SOC_TPLG_TUPLE_TYPE_SHORT:
> +		case SND_SOC_TPLG_TUPLE_TYPE_WORD:
> +			ret = avs_parse_word_tokens(comp, object, parsers, count, tuples);

avs_parse_bool_token(struct snd_soc_component *comp, void *elem, void
*object, u32 offset)
avs_parse_byte_token(struct snd_soc_component *comp, void *elem, void
*object, u32 offset)
avs_parse_short_token(struct snd_soc_component *comp, void *elem, void
*object, u32 offset)

why did you introduce such helpers, if you only use word_tokens?

> +			break;
> +		default:
> +			dev_err(comp->dev, "unknown token type %d\n", tuples->type);
> +			ret = -EINVAL;
> +		}
> +
> +		if (ret) {
> +			dev_err(comp->dev, "parsing %ld tokens of %d type failed: %d\n",
> +				count, tuples->type, ret);
> +			return ret;
> +		}
> +
> +		tuples = avs_tplg_vendor_array_next(tuples);
> +	}
> +
> +	return 0;
> +}

> +static int parse_link_formatted_string(struct snd_soc_component *comp, void *elem,
> +				       void *object, u32 offset)
> +{
> +	struct snd_soc_tplg_vendor_string_elem *tuple = elem;
> +	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
> +	char *val = (char *)((u8 *)object + offset);
> +
> +	/*
> +	 * Dynamic naming - string formats, e.g.: ssp%d - supported only for
> +	 * topologies describing single device e.g.: an I2S codec on SSP0.
> +	 */

what are you trying to optimize here? the topology will contain the name
in all cases?

> +	if (hweight_long(mach->link_mask) != 1)
> +		return avs_parse_string_token(comp, elem, object, offset);
> +
> +	snprintf(val, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, tuple->string,
> +		 __ffs(mach->link_mask));
> +
> +	return 0;
> +}

> +struct avs_tplg {
> +	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	u32 version;

version of what and where does it come from (manifest)?

does this contain an ABI information? if yes, how is it defined?

> +	struct snd_soc_component *comp;
> +
> +	struct avs_tplg_library *libs;
> +	u32 num_libs;
> +	struct avs_audio_format *fmts;
> +	u32 num_fmts;
> +	struct avs_tplg_modcfg_base *modcfgs_base;
> +	u32 num_modcfgs_base;
> +};

Re: [RFC 02/13] ASoC: Intel: avs: Add topology parsing infrastructure

[Cezary Rojewski]

On 2022-02-25 6:20 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> AVS topology is split into two major parts: dictionaries - found within
>> ASoC topology manifest - and path templates - found within DAPM widget
> 
> what is a "path template"? this is the third time I review your patches
> and I have yet to find a description of all this.
> 
> If you introduce a new concept you really need to explain what problem
> you are trying to solve, why it's important and what other alternatives
> could be considered. Consider adding a Documentation file to explain
> what you are trying to accomplish.
> 
> Jumping to optimizations of memory footprint through dictionaries is too
> early.


Hello,

I don't believe it's early for optimization and such. ASoC topology 
feature has not been invented yesterday and most of the topology files 
we see used are far from perfect.

I've been trying to explaining "path template" on several occasions, 
also during our meeting last year. Now, there's no separate 
Documentation for "path template" as is not a new concept really, it's a 
different name for already existing thing. Every driver which makes use 
of ASoC topology needs to have a "path template". skylake-driver has a 
"path template", sof-driver has one too. Topology information does not 
match 1:1 to runtime, it never did. You use topology to describe how the 
stream shall look like in runtime, kernel takes that information and 
instantiates the runtime.

If you do not believe, please see the skylake-driver topology which is 
made of:
- ModuleType, ModuleResource, ModuleInterface (...) dictionaries
- Path and PathDescription

There two blocks looks very, very similar to:
- ModuleConfigBase, ModuleConfigExt (...) dictionaries
- Path and PathTemplate

which are supposedly 'new' in avs-driver. Yes, we provided several 
optimizations, but the "path template"/"path pattern"/"path description" 
was already there.

>> private data. Dictionaries job is to reduce the total amount of memory
>> occupied by topology elements. Rather than having every pipeline and
>> module carry its own information, each refers to specific entry in
>> specific dictionary by provided (from topology file) indexes. In
>> consequence, most struct avs_tplg_xxx are made out of pointers.
>> To support the above, range of parsing helpers for all value-types known
>> to ALSA: uuid, bool, byte, short, word and string are added. Additional
>> handlers help translate pointer-types and more complex objects such as
>> audio formats and module base configs.

...

>> +/*
>> + * Scan provided block of tuples for the specified token. If found,
>> + * @offset is updated with position at which first matching token is
>> + * located.
>> + *
>> + * Returns 0 on success, -ENOENT if not found and error code otherwise.
>> + */
>> +static int
>> +avs_tplg_vendor_array_lookup(struct snd_soc_tplg_vendor_array *tuples,
>> +			     u32 block_size, u32 token, u32 *offset)
>> +{
>> +	u32 pos = 0;
>> +
>> +	while (block_size > 0) {
>> +		struct snd_soc_tplg_vendor_value_elem *tuple;
>> +		u32 tuples_size = le32_to_cpu(tuples->size);
>> +
>> +		if (tuples_size > block_size)
>> +			return -EINVAL;
>> +
>> +		tuple = tuples->value;
>> +		if (le32_to_cpu(tuple->token) == token) {
>> +			*offset = pos;
>> +			return 0;
>> +		}
>> +
>> +		block_size -= tuples_size;
>> +		pos += tuples_size;
>> +		tuples = avs_tplg_vendor_array_next(tuples);
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>> +/*
>> + * See avs_tplg_vendor_array_lookup() for description.
>> + *
>> + * Behaves exactly like its precursor but starts from the next vendor
>> + * array in line. Useful when searching for the finish line of an
>> + * arbitrary entry in a list of entries where each is composed of
>> + * several vendor tuples and a specific token marks the beginning of
>> + * a new entry block.
> 
> please try and reword such comments for people who didn't take part in
> the development.
> 
> I really have no idea what this is about.


Please provide suggestion - "don't understand" does not help me in 
rewording the comment.

ASoC topology is not the easiest to digest feature in general. Comments 
found here assume the layout and organization of sections, such as 
vendor tokens and vendor tuples with ASoC topology file are known to the 
reader.

>> + */
>> +static int
>> +avs_tplg_vendor_array_lookup_next(struct snd_soc_tplg_vendor_array *tuples,
>> +				  u32 block_size, u32 token, u32 *offset)
>> +{
>> +	u32 tuples_size = le32_to_cpu(tuples->size);
>> +	int ret;
>> +
>> +	if (tuples_size > block_size)
>> +		return -EINVAL;
>> +
>> +	tuples = avs_tplg_vendor_array_next(tuples);
>> +	block_size -= tuples_size;
>> +
>> +	ret = avs_tplg_vendor_array_lookup(tuples, block_size, token, offset);
>> +	if (!ret)
>> +		*offset += tuples_size;
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Scan provided block of tuples for the specified token which marks
>> + * the boarder of an entry block. Behavior is similar to
> 
> boarder looks like a typo. Did you mean border? boundary? position?
> location?


Indeed, it is supposed to be "border". Thanks!

>> + * avs_tplg_vendor_array_lookup() except 0 is also returned if no
>> + * matching token has been found. In such case, returned @size is
>> + * assigned to @block_size as the entire block belongs to the current
>> + * entry.
>> + *
>> + * Returns 0 on success, error code otherwise.
>> + */
>> +static int
>> +avs_tplg_vendor_entry_size(struct snd_soc_tplg_vendor_array *tuples,
>> +			   u32 block_size, u32 entry_id_token, u32 *size)
>> +{
>> +	int ret;
>> +
>> +	ret = avs_tplg_vendor_array_lookup_next(tuples, block_size, entry_id_token, size);
>> +	if (ret == -ENOENT) {
>> +		*size = block_size;
>> +		ret = 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Vendor tuple parsing descriptor.
>> + *
>> + * @token: vendor specific token that identifies tuple
>> + * @type: tuple type, one of SND_SOC_TPLG_TUPLE_TYPE_XXX
>> + * @offset: offset of a struct's field to initialize
>> + * @parse: parsing function, extracts and assigns value to object's field
>> + */
>> +struct avs_tplg_token_parser {
>> +	enum avs_tplg_token token;
>> +	u32 type;
>> +	u32 offset;
>> +	int (*parse)(struct snd_soc_component *comp, void *elem, void *object, u32 offset);
>> +};


...

>> +static int avs_parse_tokens(struct snd_soc_component *comp, void *object,
>> +			    const struct avs_tplg_token_parser *parsers, size_t count,
>> +			    struct snd_soc_tplg_vendor_array *tuples, int priv_size)
>> +{
>> +	int array_size, ret;
>> +
>> +	while (priv_size > 0) {
>> +		array_size = le32_to_cpu(tuples->size);
>> +
>> +		if (array_size <= 0) {
>> +			dev_err(comp->dev, "invalid array size 0x%x\n", array_size);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Make sure there is enough data before parsing. */
>> +		priv_size -= array_size;
>> +		if (priv_size < 0) {
>> +			dev_err(comp->dev, "invalid array size 0x%x\n", array_size);
>> +			return -EINVAL;
>> +		}
>> +
>> +		switch (le32_to_cpu(tuples->type)) {
>> +		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
>> +			ret = avs_parse_uuid_tokens(comp, object, parsers, count, tuples);
>> +			break;
>> +		case SND_SOC_TPLG_TUPLE_TYPE_STRING:
>> +			ret = avs_parse_string_tokens(comp, object, parsers, count, tuples);
>> +			break;
>> +		case SND_SOC_TPLG_TUPLE_TYPE_BOOL:
>> +		case SND_SOC_TPLG_TUPLE_TYPE_BYTE:
>> +		case SND_SOC_TPLG_TUPLE_TYPE_SHORT:
>> +		case SND_SOC_TPLG_TUPLE_TYPE_WORD:
>> +			ret = avs_parse_word_tokens(comp, object, parsers, count, tuples);
> 
> avs_parse_bool_token(struct snd_soc_component *comp, void *elem, void
> *object, u32 offset)
> avs_parse_byte_token(struct snd_soc_component *comp, void *elem, void
> *object, u32 offset)
> avs_parse_short_token(struct snd_soc_component *comp, void *elem, void
> *object, u32 offset)
> 
> why did you introduce such helpers, if you only use word_tokens?


Huh? we do make use of all of these. Perhaps you missed these being used 
in the follow up patches (in this very series). This patch defines the 
parsing infrastructure so its declaration is separated from module and 
pipeline parsing details.

>> +			break;
>> +		default:
>> +			dev_err(comp->dev, "unknown token type %d\n", tuples->type);
>> +			ret = -EINVAL;
>> +		}
>> +
>> +		if (ret) {
>> +			dev_err(comp->dev, "parsing %ld tokens of %d type failed: %d\n",
>> +				count, tuples->type, ret);
>> +			return ret;
>> +		}
>> +
>> +		tuples = avs_tplg_vendor_array_next(tuples);
>> +	}
>> +
>> +	return 0;
>> +}
> 
>> +static int parse_link_formatted_string(struct snd_soc_component *comp, void *elem,
>> +				       void *object, u32 offset)
>> +{
>> +	struct snd_soc_tplg_vendor_string_elem *tuple = elem;
>> +	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
>> +	char *val = (char *)((u8 *)object + offset);
>> +
>> +	/*
>> +	 * Dynamic naming - string formats, e.g.: ssp%d - supported only for
>> +	 * topologies describing single device e.g.: an I2S codec on SSP0.
>> +	 */
> 
> what are you trying to optimize here? the topology will contain the name
> in all cases?


I'll probably separate the name%d part so it's not clouding the core 
part of topology parsing.

These if-statements are here to allow %d to be filled automatically by 
kernel for SSP boards with ->link_mask found in ACPI board descriptor.

Example for avs_rt298 with snd_soc_acpi_mach::link_mask=BIT(0):
1) Topology file for avs_rt298 provides widget with name: ssp%d_be
2) Runtime topology parsing formats that name to: ssp0_be

>> +	if (hweight_long(mach->link_mask) != 1)
>> +		return avs_parse_string_token(comp, elem, object, offset);
>> +
>> +	snprintf(val, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, tuple->string,
>> +		 __ffs(mach->link_mask));
>> +
>> +	return 0;
>> +}
> 
>> +struct avs_tplg {
>> +	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
>> +	u32 version;
> 
> version of what and where does it come from (manifest)?
> 
> does this contain an ABI information? if yes, how is it defined?


Yes, this one comes from topology manifest. Right now we decided to use 
single-digit versioning for simplicity, similarly to ASoC topology one.

>> +	struct snd_soc_component *comp;
>> +
>> +	struct avs_tplg_library *libs;
>> +	u32 num_libs;
>> +	struct avs_audio_format *fmts;
>> +	u32 num_fmts;
>> +	struct avs_tplg_modcfg_base *modcfgs_base;
>> +	u32 num_modcfgs_base;
>> +};

[RFC 03/13] ASoC: Intel: avs: Parse module-extension tuples

[Cezary Rojewski]

Anything that goes beyond module base config is an extension config. It
covers all fields for all specific module types available in ADSP
firmware. Add parsing helpers to support loading such information from
the topology file.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/topology.c | 297 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |  59 +++++++
 2 files changed, 356 insertions(+)

diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 4b8b415ca006..6c682fed9f5f 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -344,6 +344,7 @@ avs_parse_##name##_ptr(struct snd_soc_component *comp, void *elem, void *object,
 
 AVS_DEFINE_PTR_PARSER(audio_format, struct avs_audio_format, fmts);
 AVS_DEFINE_PTR_PARSER(modcfg_base, struct avs_tplg_modcfg_base, modcfgs_base);
+AVS_DEFINE_PTR_PARSER(modcfg_ext, struct avs_tplg_modcfg_ext, modcfgs_ext);
 
 static int
 parse_audio_format_bitfield(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
@@ -593,3 +594,299 @@ static int avs_tplg_parse_modcfgs_base(struct snd_soc_component *comp,
 				AVS_TKN_MODCFG_BASE_ID_U32,
 				modcfg_base_parsers, ARRAY_SIZE(modcfg_base_parsers));
 }
+
+static const struct avs_tplg_token_parser modcfg_ext_parsers[] = {
+	{
+		.token = AVS_TKN_MODCFG_EXT_TYPE_UUID,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_UUID,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, type),
+		.parse = avs_parse_uuid_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_CPR_OUT_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, copier.out_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_CPR_FEATURE_MASK_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, copier.feature_mask),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_CPR_VINDEX_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, copier.vindex),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_CPR_DMA_TYPE_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, copier.dma_type),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_CPR_DMABUFF_SIZE_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, copier.dma_buffer_size),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_CPR_BLOB_FMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, copier.blob_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_MICSEL_OUT_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, micsel.out_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_INTELWOV_CPC_LP_MODE_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, wov.cpc_lp_mode),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_SRC_OUT_FREQ_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, src.out_freq),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_MUX_REF_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, mux.ref_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_MUX_OUT_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, mux.out_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_AEC_REF_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, aec.ref_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_AEC_OUT_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, aec.out_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MODCFG_AEC_CPC_LP_MODE_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, aec.cpc_lp_mode),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_ASRC_OUT_FREQ_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, asrc.out_freq),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_ASRC_MODE_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, asrc.mode),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_ASRC_DISABLE_JITTER_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, asrc.disable_jitter_buffer),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_OUT_CHAN_CFG_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.out_channel_config),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_SELECT_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients_select),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_0_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[0]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_1_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[1]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_2_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[2]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_3_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[3]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_4_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[4]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_5_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[5]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_6_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[6]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_COEFF_7_S32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.coefficients[7]),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_UPDOWN_MIX_CHAN_MAP_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, updown_mix.channel_map),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_EXT_NUM_INPUT_PINS_U16,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_SHORT,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, generic.num_input_pins),
+		.parse = avs_parse_short_token,
+	},
+	{
+		.token = AVS_TKN_MODCFG_EXT_NUM_OUTPUT_PINS_U16,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_SHORT,
+		.offset = offsetof(struct avs_tplg_modcfg_ext, generic.num_output_pins),
+		.parse = avs_parse_short_token,
+	},
+};
+
+static const struct avs_tplg_token_parser pin_format_parsers[] = {
+	{
+		.token = AVS_TKN_PIN_FMT_INDEX_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pin_format, pin_index),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_PIN_FMT_IOBS_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pin_format, iobs),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_PIN_FMT_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pin_format, fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+};
+
+static int avs_tplg_parse_modcfg_ext(struct snd_soc_component *comp,
+				     struct avs_tplg_modcfg_ext *cfg,
+				     struct snd_soc_tplg_vendor_array *tuples,
+				     u32 block_size)
+{
+	u32 esize;
+	int ret;
+
+	/* See where pin block starts. */
+	ret = avs_tplg_vendor_entry_size(tuples, block_size,
+					 AVS_TKN_PIN_FMT_INDEX_U32, &esize);
+	if (ret)
+		return ret;
+
+	ret = avs_parse_tokens(comp, cfg, modcfg_ext_parsers,
+			       ARRAY_SIZE(modcfg_ext_parsers), tuples, esize);
+	if (ret)
+		return ret;
+
+	block_size -= esize;
+	/* Parse trailing in/out pin formats if any. */
+	if (block_size) {
+		struct avs_tplg_pin_format *pins;
+		u32 num_pins;
+
+		num_pins = cfg->generic.num_input_pins + cfg->generic.num_output_pins;
+		if (!num_pins)
+			return -EINVAL;
+
+		pins = devm_kcalloc(comp->card->dev, num_pins, sizeof(*pins), GFP_KERNEL);
+		if (!pins)
+			return -ENOMEM;
+
+		tuples = avs_tplg_vendor_array_at(tuples, esize);
+		ret = parse_dictionary_entries(comp, tuples, block_size,
+					       pins, num_pins, sizeof(*pins),
+					       AVS_TKN_PIN_FMT_INDEX_U32,
+					       pin_format_parsers,
+					       ARRAY_SIZE(pin_format_parsers));
+		if (ret)
+			return ret;
+		cfg->generic.pin_fmts = pins;
+	}
+
+	return 0;
+}
+
+static int avs_tplg_parse_modcfgs_ext(struct snd_soc_component *comp,
+				      struct snd_soc_tplg_vendor_array *tuples,
+				      u32 block_size)
+{
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg = acomp->tplg;
+	int ret, i;
+
+	ret = parse_dictionary_header(comp, tuples, (void **)&tplg->modcfgs_ext,
+				      &tplg->num_modcfgs_ext,
+				      sizeof(*tplg->modcfgs_ext),
+				      AVS_TKN_MANIFEST_NUM_MODCFGS_EXT_U32);
+	if (ret)
+		return ret;
+
+	block_size -= le32_to_cpu(tuples->size);
+	/* With header parsed, move on to parsing entries. */
+	tuples = avs_tplg_vendor_array_next(tuples);
+
+	for (i = 0; i < tplg->num_modcfgs_ext; i++) {
+		struct avs_tplg_modcfg_ext *cfg = &tplg->modcfgs_ext[i];
+		u32 esize;
+
+		ret = avs_tplg_vendor_entry_size(tuples, block_size,
+						 AVS_TKN_MODCFG_EXT_ID_U32, &esize);
+		if (ret)
+			return ret;
+
+		ret = avs_tplg_parse_modcfg_ext(comp, cfg, tuples, esize);
+		if (ret)
+			return ret;
+
+		block_size -= esize;
+		tuples = avs_tplg_vendor_array_at(tuples, esize);
+	}
+
+	return 0;
+}
diff --git a/sound/soc/intel/avs/topology.h b/sound/soc/intel/avs/topology.h
index a3ab5d15c9ee..ce51fd7a99de 100644
--- a/sound/soc/intel/avs/topology.h
+++ b/sound/soc/intel/avs/topology.h
@@ -27,6 +27,8 @@ struct avs_tplg {
 	u32 num_fmts;
 	struct avs_tplg_modcfg_base *modcfgs_base;
 	u32 num_modcfgs_base;
+	struct avs_tplg_modcfg_ext *modcfgs_ext;
+	u32 num_modcfgs_ext;
 };
 
 struct avs_tplg_library {
@@ -41,4 +43,61 @@ struct avs_tplg_modcfg_base {
 	u32 is_pages;
 };
 
+struct avs_tplg_pin_format {
+	u32 pin_index;
+	u32 iobs;
+	struct avs_audio_format *fmt;
+};
+
+struct avs_tplg_modcfg_ext {
+	guid_t type;
+
+	union {
+		struct {
+			u16 num_input_pins;
+			u16 num_output_pins;
+			struct avs_tplg_pin_format *pin_fmts;
+		} generic;
+		struct {
+			struct avs_audio_format *out_fmt;
+			struct avs_audio_format *blob_fmt; /* optional override */
+			u32 feature_mask;
+			union avs_virtual_index vindex;
+			u32 dma_type;
+			u32 dma_buffer_size;
+			u32 config_length;
+			/* config_data part of priv data */
+		} copier;
+		struct {
+			u32 out_channel_config;
+			u32 coefficients_select;
+			s32 coefficients[AVS_CHANNELS_MAX];
+			u32 channel_map;
+		} updown_mix;
+		struct {
+			u32 out_freq;
+		} src;
+		struct {
+			u32 out_freq;
+			u8 mode;
+			u8 disable_jitter_buffer;
+		} asrc;
+		struct {
+			u32 cpc_lp_mode;
+		} wov;
+		struct {
+			struct avs_audio_format *ref_fmt;
+			struct avs_audio_format *out_fmt;
+			u32 cpc_lp_mode;
+		} aec;
+		struct {
+			struct avs_audio_format *ref_fmt;
+			struct avs_audio_format *out_fmt;
+		} mux;
+		struct {
+			struct avs_audio_format *out_fmt;
+		} micsel;
+	};
+};
+
 #endif
-- 
2.25.1

Re: [RFC 03/13] ASoC: Intel: avs: Parse module-extension tuples

[Pierre-Louis Bossart]

> +struct avs_tplg_modcfg_ext {
> +	guid_t type;
> +
> +	union {
> +		struct {
> +			u16 num_input_pins;
> +			u16 num_output_pins;
> +			struct avs_tplg_pin_format *pin_fmts;
> +		} generic;
> +		struct {
> +			struct avs_audio_format *out_fmt;
> +			struct avs_audio_format *blob_fmt; /* optional override */
> +			u32 feature_mask;
> +			union avs_virtual_index vindex;
> +			u32 dma_type;
> +			u32 dma_buffer_size;
> +			u32 config_length;
> +			/* config_data part of priv data */
> +		} copier;
> +		struct {
> +			u32 out_channel_config;
> +			u32 coefficients_select;
> +			s32 coefficients[AVS_CHANNELS_MAX];
> +			u32 channel_map;
> +		} updown_mix;
> +		struct {
> +			u32 out_freq;
> +		} src;
> +		struct {
> +			u32 out_freq;
> +			u8 mode;
> +			u8 disable_jitter_buffer;
> +		} asrc;
> +		struct {
> +			u32 cpc_lp_mode;
> +		} wov;
> +		struct {
> +			struct avs_audio_format *ref_fmt;
> +			struct avs_audio_format *out_fmt;
> +			u32 cpc_lp_mode;
> +		} aec;
> +		struct {
> +			struct avs_audio_format *ref_fmt;
> +			struct avs_audio_format *out_fmt;
> +		} mux;
> +		struct {
> +			struct avs_audio_format *out_fmt;
> +		} micsel;
> +	};
> +};

I am struggling to reconcile the notion of 'extension' with a fixed
structure that deals with Intel-specific modules.

How would a 3rd party add their own modules and expose parameters/tokens
through the topology?

Re: [RFC 03/13] ASoC: Intel: avs: Parse module-extension tuples

[Cezary Rojewski]

On 2022-02-25 6:24 PM, Pierre-Louis Bossart wrote:
> 
>> +struct avs_tplg_modcfg_ext {
>> +	guid_t type;
>> +
>> +	union {
>> +		struct {
>> +			u16 num_input_pins;
>> +			u16 num_output_pins;
>> +			struct avs_tplg_pin_format *pin_fmts;
>> +		} generic;
>> +		struct {
>> +			struct avs_audio_format *out_fmt;
>> +			struct avs_audio_format *blob_fmt; /* optional override */
>> +			u32 feature_mask;
>> +			union avs_virtual_index vindex;
>> +			u32 dma_type;
>> +			u32 dma_buffer_size;
>> +			u32 config_length;
>> +			/* config_data part of priv data */
>> +		} copier;
>> +		struct {
>> +			u32 out_channel_config;
>> +			u32 coefficients_select;
>> +			s32 coefficients[AVS_CHANNELS_MAX];
>> +			u32 channel_map;
>> +		} updown_mix;
>> +		struct {
>> +			u32 out_freq;
>> +		} src;
>> +		struct {
>> +			u32 out_freq;
>> +			u8 mode;
>> +			u8 disable_jitter_buffer;
>> +		} asrc;
>> +		struct {
>> +			u32 cpc_lp_mode;
>> +		} wov;
>> +		struct {
>> +			struct avs_audio_format *ref_fmt;
>> +			struct avs_audio_format *out_fmt;
>> +			u32 cpc_lp_mode;
>> +		} aec;
>> +		struct {
>> +			struct avs_audio_format *ref_fmt;
>> +			struct avs_audio_format *out_fmt;
>> +		} mux;
>> +		struct {
>> +			struct avs_audio_format *out_fmt;
>> +		} micsel;
>> +	};
>> +};
> 
> I am struggling to reconcile the notion of 'extension' with a fixed
> structure that deals with Intel-specific modules.
> 
> How would a 3rd party add their own modules and expose parameters/tokens
> through the topology?


All vendor modules go into 'generic' bucket. Vendor cannot define any 
specific fields i.e. extend the 'generic' header structure. Everything 
that is specific to them has to go into so called payload that is part 
of almost every INIT_INSTANCE.


Regards,
Czarek

[RFC 04/13] ASoC: Intel: avs: Parse pplcfg and binding tuples

[Cezary Rojewski]

Stream on ADSP firmware is represented by one or more pipelines. Just
like modules, these are described by a config structure. Add parsing
helpers to support loading such information from the topology file.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/topology.c | 114 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |  29 +++++++++
 2 files changed, 143 insertions(+)

diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 6c682fed9f5f..7d454a0ea000 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -345,6 +345,8 @@ avs_parse_##name##_ptr(struct snd_soc_component *comp, void *elem, void *object,
 AVS_DEFINE_PTR_PARSER(audio_format, struct avs_audio_format, fmts);
 AVS_DEFINE_PTR_PARSER(modcfg_base, struct avs_tplg_modcfg_base, modcfgs_base);
 AVS_DEFINE_PTR_PARSER(modcfg_ext, struct avs_tplg_modcfg_ext, modcfgs_ext);
+AVS_DEFINE_PTR_PARSER(pplcfg, struct avs_tplg_pplcfg, pplcfgs);
+AVS_DEFINE_PTR_PARSER(binding, struct avs_tplg_binding, bindings);
 
 static int
 parse_audio_format_bitfield(struct snd_soc_component *comp, void *elem, void *object, u32 offset)
@@ -890,3 +892,115 @@ static int avs_tplg_parse_modcfgs_ext(struct snd_soc_component *comp,
 
 	return 0;
 }
+
+static const struct avs_tplg_token_parser pplcfg_parsers[] = {
+	{
+		.token = AVS_TKN_PPLCFG_REQ_SIZE_U16,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_SHORT,
+		.offset = offsetof(struct avs_tplg_pplcfg, req_size),
+		.parse = avs_parse_short_token,
+	},
+	{
+		.token = AVS_TKN_PPLCFG_PRIORITY_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_pplcfg, priority),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_PPLCFG_LOW_POWER_BOOL,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BOOL,
+		.offset = offsetof(struct avs_tplg_pplcfg, lp),
+		.parse = avs_parse_bool_token,
+	},
+	{
+		.token = AVS_TKN_PPLCFG_ATTRIBUTES_U16,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_SHORT,
+		.offset = offsetof(struct avs_tplg_pplcfg, attributes),
+		.parse = avs_parse_short_token,
+	},
+	{
+		.token = AVS_TKN_PPLCFG_TRIGGER_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pplcfg, trigger),
+		.parse = avs_parse_word_token,
+	},
+};
+
+static int avs_tplg_parse_pplcfgs(struct snd_soc_component *comp,
+				  struct snd_soc_tplg_vendor_array *tuples,
+				  u32 block_size)
+{
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg = acomp->tplg;
+
+	return parse_dictionary(comp, tuples, block_size, (void **)&tplg->pplcfgs,
+				&tplg->num_pplcfgs, sizeof(*tplg->pplcfgs),
+				AVS_TKN_MANIFEST_NUM_PPLCFGS_U32,
+				AVS_TKN_PPLCFG_ID_U32,
+				pplcfg_parsers, ARRAY_SIZE(pplcfg_parsers));
+}
+
+static const struct avs_tplg_token_parser binding_parsers[] = {
+	{
+		.token = AVS_TKN_BINDING_TARGET_TPLG_NAME_STRING,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_STRING,
+		.offset = offsetof(struct avs_tplg_binding, target_tplg_name),
+		.parse = parse_link_formatted_string,
+	},
+	{
+		.token = AVS_TKN_BINDING_TARGET_PATH_TMPL_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_binding, target_path_tmpl_id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_BINDING_TARGET_PPL_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_binding, target_ppl_id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_BINDING_TARGET_MOD_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_binding, target_mod_id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_BINDING_TARGET_MOD_PIN_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_binding, target_mod_pin),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_BINDING_MOD_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_binding, mod_id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_BINDING_MOD_PIN_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_binding, mod_pin),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_BINDING_IS_SINK_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_binding, is_sink),
+		.parse = avs_parse_byte_token,
+	},
+};
+
+static int avs_tplg_parse_bindings(struct snd_soc_component *comp,
+				   struct snd_soc_tplg_vendor_array *tuples,
+				   u32 block_size)
+{
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg = acomp->tplg;
+
+	return parse_dictionary(comp, tuples, block_size, (void **)&tplg->bindings,
+				&tplg->num_bindings, sizeof(*tplg->bindings),
+				AVS_TKN_MANIFEST_NUM_BINDINGS_U32,
+				AVS_TKN_BINDING_ID_U32,
+				binding_parsers, ARRAY_SIZE(binding_parsers));
+}
diff --git a/sound/soc/intel/avs/topology.h b/sound/soc/intel/avs/topology.h
index ce51fd7a99de..d23f4aba0bcc 100644
--- a/sound/soc/intel/avs/topology.h
+++ b/sound/soc/intel/avs/topology.h
@@ -29,6 +29,10 @@ struct avs_tplg {
 	u32 num_modcfgs_base;
 	struct avs_tplg_modcfg_ext *modcfgs_ext;
 	u32 num_modcfgs_ext;
+	struct avs_tplg_pplcfg *pplcfgs;
+	u32 num_pplcfgs;
+	struct avs_tplg_binding *bindings;
+	u32 num_bindings;
 };
 
 struct avs_tplg_library {
@@ -100,4 +104,29 @@ struct avs_tplg_modcfg_ext {
 	};
 };
 
+/* Specifies path behaviour during PCM ->trigger(START) commnand. */
+enum avs_tplg_trigger {
+	AVS_TPLG_TRIGGER_AUTO = 0,
+	AVS_TPLG_TRIGGER_USERSPACE = 1, /* via sysfs */
+};
+
+struct avs_tplg_pplcfg {
+	u16 req_size;
+	u8 priority;
+	bool lp;
+	u16 attributes;
+	enum avs_tplg_trigger trigger;
+};
+
+struct avs_tplg_binding {
+	char target_tplg_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+	u32 target_path_tmpl_id;
+	u32 target_ppl_id;
+	u32 target_mod_id;
+	u8 target_mod_pin;
+	u32 mod_id;
+	u8 mod_pin;
+	u8 is_sink;
+};
+
 #endif
-- 
2.25.1

Re: [RFC 04/13] ASoC: Intel: avs: Parse pplcfg and binding tuples

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> Stream on ADSP firmware is represented by one or more pipelines. Just

I have a lot of questions on this one line...

what is a 'stream'?

'stream' historically means 'direction' in ALSA.

Then we have sdw_stream, which describes how source and sink ports are
connected on a platform.

We also have DPCM front-ends, visible mostly through the PCM device they
expose to users.

In windows we have stream effects that are really meant to be on a
single dedicated pipeline.

other questions: can a stream represent data moving in different
directions, e.g. in full-duplex mode.

How would a loopback be described?

What happens when a data path is split (demux) or merged (mixer)?

How is a 'stream' different from a 'path template' that you referred to
in Patch RFC 02/13

You would need at least 10 lines of plain English to make sure no one
will misunderstand what 'stream' means.

> like modules, these are described by a config structure. Add parsing
> helpers to support loading such information from the topology file.
>
> +/* Specifies path behaviour during PCM ->trigger(START) commnand. */

typo: command.

> +enum avs_tplg_trigger {
> +	AVS_TPLG_TRIGGER_AUTO = 0,
> +	AVS_TPLG_TRIGGER_USERSPACE = 1, /* via sysfs */

The feedback in previous versions was that we should not expose any
sysfs interface that would interfere with decisions made by the driver.

This is very controversial and a major hurdle to upstream any of this.

Debugfs is fine, as suggested by Mark as well

"
If it's mainly used for debugging then it could be exposed through
debugfs with less worry.
"


> +};
> +
> +struct avs_tplg_pplcfg {
> +	u16 req_size;

what does this describe?

> +	u8 priority;
> +	bool lp;
> +	u16 attributes;
> +	enum avs_tplg_trigger trigger;
> +};
> +
> +struct avs_tplg_binding {
> +	char target_tplg_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	u32 target_path_tmpl_id;
> +	u32 target_ppl_id;
> +	u32 target_mod_id;
> +	u8 target_mod_pin;

you really need to elaborate on what a template is, and how you use a
template and a ppl id concurrently.

> +	u32 mod_id;
> +	u8 mod_pin;
> +	u8 is_sink;
> +};
> +
>  #endif

Re: [RFC 04/13] ASoC: Intel: avs: Parse pplcfg and binding tuples

[Cezary Rojewski]

On 2022-02-25 6:40 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> Stream on ADSP firmware is represented by one or more pipelines. Just
> 
> I have a lot of questions on this one line...
> 
> what is a 'stream'?
> 
> 'stream' historically means 'direction' in ALSA.


That ambiguity should be fixed long ago, it's probably the most 
frequently asked question/error done by ALSA newcomers.

Many drivers use 'stream' without implying direction e.g.: hda. It's 
also part of framework language anyway e.g.: substream (is that supposed 
to imply: subdirection?)

> Then we have sdw_stream, which describes how source and sink ports are
> connected on a platform.
> 
> We also have DPCM front-ends, visible mostly through the PCM device they
> expose to users.
> 
> In windows we have stream effects that are really meant to be on a
> single dedicated pipeline.
> 
> other questions: can a stream represent data moving in different
> directions, e.g. in full-duplex mode.
> 
> How would a loopback be described?
> 
> What happens when a data path is split (demux) or merged (mixer)?
> 
> How is a 'stream' different from a 'path template' that you referred to
> in Patch RFC 02/13
> 
> You would need at least 10 lines of plain English to make sure no one
> will misunderstand what 'stream' means.


If that's the case, then we should re-think how and when 'stream' is 
used within the kernel.

Now, everyone from Windows team is perfectly fine with using 'stream' as 
is, it's common there.
There are many types of effects, and the 'effect' subject has an 
ambiguity of its own but let's not have that subject here on the ALSA 
mailing list :)

I believe you would like a reword, as it seems my usage of 'stream' 
brought a ton of questions. 'Path' perhaps?

>> like modules, these are described by a config structure. Add parsing
>> helpers to support loading such information from the topology file.
>>
>> +/* Specifies path behaviour during PCM ->trigger(START) commnand. */
> 
> typo: command.


Ack, thanks!

>> +enum avs_tplg_trigger {
>> +	AVS_TPLG_TRIGGER_AUTO = 0,
>> +	AVS_TPLG_TRIGGER_USERSPACE = 1, /* via sysfs */
> 
> The feedback in previous versions was that we should not expose any
> sysfs interface that would interfere with decisions made by the driver.
> 
> This is very controversial and a major hurdle to upstream any of this.
> 
> Debugfs is fine, as suggested by Mark as well
> 
> "
> If it's mainly used for debugging then it could be exposed through
> debugfs with less worry.
> "


I'll remove 'USERSPACE' entry for now. I'll come back to that later on, 
in a different series.

>> +};
>> +
>> +struct avs_tplg_pplcfg {
>> +	u16 req_size;
> 
> what does this describe?


Pipeline configuration i.e. all the information required when sending 
CREATE_PIPELINE IPC.

>> +	u8 priority;
>> +	bool lp;
>> +	u16 attributes;
>> +	enum avs_tplg_trigger trigger;
>> +};
>> +
>> +struct avs_tplg_binding {
>> +	char target_tplg_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
>> +	u32 target_path_tmpl_id;
>> +	u32 target_ppl_id;
>> +	u32 target_mod_id;
>> +	u8 target_mod_pin;
> 
> you really need to elaborate on what a template is, and how you use a
> template and a ppl id concurrently.


As stated, such thing exists already, e.g.: in the skylake-driver. 
"Binding" here is called "Link" there. It's a different representation 
of the same thing.

Here we have all the information to without a question identify modules 
to bind during runtime.

>> +	u32 mod_id;
>> +	u8 mod_pin;
>> +	u8 is_sink;
>> +};
>> +
>>   #endif

[RFC 05/13] ASoC: Intel: avs: Parse pipeline and module tuples

[Cezary Rojewski]

Shape of a stream on DSP side, that is, the number and the layout of
its pipelines and modules is paramount for streaming to be efficient and
low power-consuming. Add parsing helpers to support loading such
information from the topology file.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/topology.c | 177 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |  31 ++++++
 2 files changed, 208 insertions(+)

diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 7d454a0ea000..8889ceae19b9 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -1004,3 +1004,180 @@ static int avs_tplg_parse_bindings(struct snd_soc_component *comp,
 				AVS_TKN_BINDING_ID_U32,
 				binding_parsers, ARRAY_SIZE(binding_parsers));
 }
+
+static const struct avs_tplg_token_parser module_parsers[] = {
+	{
+		.token = AVS_TKN_MOD_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_module, id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_MOD_MODCFG_BASE_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_module, cfg_base),
+		.parse = avs_parse_modcfg_base_ptr,
+	},
+	{
+		.token = AVS_TKN_MOD_IN_AFMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_module, in_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_MOD_CORE_ID_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_module, core_id),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_MOD_PROC_DOMAIN_U8,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_BYTE,
+		.offset = offsetof(struct avs_tplg_module, domain),
+		.parse = avs_parse_byte_token,
+	},
+	{
+		.token = AVS_TKN_MOD_MODCFG_EXT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_module, cfg_ext),
+		.parse = avs_parse_modcfg_ext_ptr,
+	},
+};
+
+static struct avs_tplg_module *
+avs_tplg_module_create(struct snd_soc_component *comp, struct avs_tplg_pipeline *owner,
+		       struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
+{
+	struct avs_tplg_module *module;
+	int ret;
+
+	module = devm_kzalloc(comp->card->dev, sizeof(*module), GFP_KERNEL);
+	if (!module)
+		return ERR_PTR(-ENOMEM);
+
+	ret = avs_parse_tokens(comp, module, module_parsers,
+			       ARRAY_SIZE(module_parsers), tuples, block_size);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	module->owner = owner;
+	INIT_LIST_HEAD(&module->node);
+
+	return module;
+}
+
+static const struct avs_tplg_token_parser pipeline_parsers[] = {
+	{
+		.token = AVS_TKN_PPL_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pipeline, id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_PPL_PPLCFG_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pipeline, cfg),
+		.parse = avs_parse_pplcfg_ptr,
+	},
+	{
+		.token = AVS_TKN_PPL_NUM_BINDING_IDS_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_pipeline, num_bindings),
+		.parse = avs_parse_word_token,
+	},
+};
+
+static const struct avs_tplg_token_parser bindings_parsers[] = {
+	{
+		.token = AVS_TKN_PPL_BINDING_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = 0, /* to treat pipeline->bindings as dictionary */
+		.parse = avs_parse_binding_ptr,
+	},
+};
+
+static struct avs_tplg_pipeline *
+avs_tplg_pipeline_create(struct snd_soc_component *comp, struct avs_tplg_path *owner,
+			 struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
+{
+	struct avs_tplg_pipeline *pipeline;
+	u32 modblk_size, offset;
+	int ret;
+
+	pipeline = devm_kzalloc(comp->card->dev, sizeof(*pipeline), GFP_KERNEL);
+	if (!pipeline)
+		return ERR_PTR(-ENOMEM);
+
+	pipeline->owner = owner;
+	INIT_LIST_HEAD(&pipeline->mod_list);
+
+	/* Pipeline header MUST be followed by at least one module. */
+	ret = avs_tplg_vendor_array_lookup(tuples, block_size,
+					   AVS_TKN_MOD_ID_U32, &offset);
+	if (!ret && !offset)
+		ret = -EINVAL;
+	if (ret)
+		return ERR_PTR(ret);
+
+	/* Process header which precedes module sections. */
+	ret = avs_parse_tokens(comp, pipeline, pipeline_parsers,
+			       ARRAY_SIZE(pipeline_parsers), tuples, offset);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	block_size -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	/* Optionally, binding sections follow module ones. */
+	ret = avs_tplg_vendor_array_lookup_next(tuples, block_size,
+						AVS_TKN_PPL_BINDING_ID_U32, &offset);
+	if (ret) {
+		if (ret != -ENOENT)
+			return ERR_PTR(ret);
+
+		/* Does header information match actual block layout? */
+		if (pipeline->num_bindings)
+			return ERR_PTR(-EINVAL);
+
+		modblk_size = block_size;
+	} else {
+		pipeline->bindings = devm_kcalloc(comp->card->dev, pipeline->num_bindings,
+						  sizeof(*pipeline->bindings), GFP_KERNEL);
+		if (!pipeline->bindings)
+			return ERR_PTR(-ENOMEM);
+
+		modblk_size = offset;
+	}
+
+	block_size -= modblk_size;
+	do {
+		struct avs_tplg_module *module;
+		u32 esize;
+
+		ret = avs_tplg_vendor_entry_size(tuples, modblk_size,
+						 AVS_TKN_MOD_ID_U32, &esize);
+		if (ret)
+			return ERR_PTR(ret);
+
+		module = avs_tplg_module_create(comp, pipeline, tuples, esize);
+		if (IS_ERR(module)) {
+			dev_err(comp->dev, "parse module failed: %ld\n",
+				PTR_ERR(module));
+			return ERR_CAST(module);
+		}
+
+		list_add_tail(&module->node, &pipeline->mod_list);
+		modblk_size -= esize;
+		tuples = avs_tplg_vendor_array_at(tuples, esize);
+	} while (modblk_size > 0);
+
+	/* What's left is optional range of bindings. */
+	ret = parse_dictionary_entries(comp, tuples, block_size, pipeline->bindings,
+				       pipeline->num_bindings, sizeof(*pipeline->bindings),
+				       AVS_TKN_PPL_BINDING_ID_U32,
+				       bindings_parsers, ARRAY_SIZE(bindings_parsers));
+	if (ret)
+		return ERR_PTR(ret);
+
+	return pipeline;
+}
diff --git a/sound/soc/intel/avs/topology.h b/sound/soc/intel/avs/topology.h
index d23f4aba0bcc..b68c73afcde3 100644
--- a/sound/soc/intel/avs/topology.h
+++ b/sound/soc/intel/avs/topology.h
@@ -129,4 +129,35 @@ struct avs_tplg_binding {
 	u8 is_sink;
 };
 
+struct avs_tplg_path {
+	u32 id;
+};
+
+struct avs_tplg_pipeline {
+	u32 id;
+
+	struct avs_tplg_pplcfg *cfg;
+	struct avs_tplg_binding **bindings;
+	u32 num_bindings;
+	struct list_head mod_list;
+
+	struct avs_tplg_path *owner;
+	/* Path pipelines management. */
+	struct list_head node;
+};
+
+struct avs_tplg_module {
+	u32 id;
+
+	struct avs_tplg_modcfg_base *cfg_base;
+	struct avs_audio_format *in_fmt;
+	u8 core_id;
+	u8 domain;
+	struct avs_tplg_modcfg_ext *cfg_ext;
+
+	struct avs_tplg_pipeline *owner;
+	/* Pipeline modules management. */
+	struct list_head node;
+};
+
 #endif
-- 
2.25.1

Re: [RFC 05/13] ASoC: Intel: avs: Parse pipeline and module tuples

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> Shape of a stream on DSP side, that is, the number and the layout of
> its pipelines and modules is paramount for streaming to be efficient and
> low power-consuming. Add parsing helpers to support loading such
> information from the topology file.

again what is a 'stream'?


> +struct avs_tplg_path {
> +	u32 id;
> +};

A concept that boils down to a single integer is really far from clear
to me. What does this represent really?

> +
> +struct avs_tplg_pipeline {
> +	u32 id;
> +
> +	struct avs_tplg_pplcfg *cfg;
> +	struct avs_tplg_binding **bindings;
> +	u32 num_bindings;
> +	struct list_head mod_list;
> +
> +	struct avs_tplg_path *owner;

the cardinality between path and pipeline is far from clear. When you
have topologies where the same data can be rendered on multiple outputs
and demuxed into an echo reference, then what is the 'path'?

Worst case all connected pipelines could be a single path with this
hierarchical definition of ownership, but is this desired?

What happens when the user uses switches to disconnects pipelines?

> +	/* Path pipelines management. */

what is a path pipeline?

> +	struct list_head node;
> +};
> +
> +struct avs_tplg_module {
> +	u32 id;

what is the definition of id? is this local to the scope of a pipeline?
global for the entire topology?
> +
> +	struct avs_tplg_modcfg_base *cfg_base;
> +	struct avs_audio_format *in_fmt;
> +	u8 core_id;
> +	u8 domain;
> +	struct avs_tplg_modcfg_ext *cfg_ext;
> +
> +	struct avs_tplg_pipeline *owner;
> +	/* Pipeline modules management. */
> +	struct list_head node;
> +};

I would expect all modules to be seen as DAPM widgets, no?

> +
>  #endif

Re: [RFC 05/13] ASoC: Intel: avs: Parse pipeline and module tuples

[Cezary Rojewski]

On 2022-02-25 7:51 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> Shape of a stream on DSP side, that is, the number and the layout of
>> its pipelines and modules is paramount for streaming to be efficient and
>> low power-consuming. Add parsing helpers to support loading such
>> information from the topology file.
> 
> again what is a 'stream'?


A collection of pipelines, usually connecting HOST (HDA DMA) gateway 
with LINK (GPDMA) gateway.

>> +struct avs_tplg_path {
>> +	u32 id;
>> +};
> 
> A concept that boils down to a single integer is really far from clear
> to me. What does this represent really?


Nah, the structure is much larger. Here, to have pipeline parsing 
separated from the rest, some stub needed to be provided.

>> +
>> +struct avs_tplg_pipeline {
>> +	u32 id;
>> +
>> +	struct avs_tplg_pplcfg *cfg;
>> +	struct avs_tplg_binding **bindings;
>> +	u32 num_bindings;
>> +	struct list_head mod_list;
>> +
>> +	struct avs_tplg_path *owner;
> 
> the cardinality between path and pipeline is far from clear. When you
> have topologies where the same data can be rendered on multiple outputs
> and demuxed into an echo reference, then what is the 'path'?
> 
> Worst case all connected pipelines could be a single path with this
> hierarchical definition of ownership, but is this desired?

Just like in other DSP driver cases, here topology states all the 
possibilities. It's not random by any means.

If you want given data to be rendered on multiple outputs, then you make 
use of NxFEs -> 1xBE which ASoC supports through re-parenting. Multiple 
FEs in topology would be leading to the very same BE widget. On firmware 
side you have copier which supports several outputs. You just choose 
different output pin for each FE.

> What happens when the user uses switches to disconnects pipelines?

"switches to disconnects pipelines"? How could user switch to a 
disconnected pipeline? Not following.

>> +	/* Path pipelines management. */
> 
> what is a path pipeline?

Does this question mean you want "Path" part of the comment to be removed?

>> +	struct list_head node;
>> +};
>> +
>> +struct avs_tplg_module {
>> +	u32 id;
> 
> what is the definition of id? is this local to the scope of a pipeline?
> global for the entire topology?

It's module ID, so it's local. Basically every structure starts here 
with 'id'.

>> +
>> +	struct avs_tplg_modcfg_base *cfg_base;
>> +	struct avs_audio_format *in_fmt;
>> +	u8 core_id;
>> +	u8 domain;
>> +	struct avs_tplg_modcfg_ext *cfg_ext;
>> +
>> +	struct avs_tplg_pipeline *owner;
>> +	/* Pipeline modules management. */
>> +	struct list_head node;
>> +};
> 
> I would expect all modules to be seen as DAPM widgets, no?

Makes no sense since module alone have no real impact on whether audio 
path should be powered or not. Only the entire "path" has any saying in 
that.

>> +
>>   #endif

[RFC 06/13] ASoC: Intel: avs: Parse path and path templates tuples

[Cezary Rojewski]

Path template is a pattern like its name suggests. It is tied to DAPM
widget which represents a FE or a BE and is used during path
instantiation when substream is opened for streaming. It carries a range
of available variants - actual implementation of a runtime path on
AudioDSP side. Only one variant of given path template can be
instantiated at a time and selection is based off of audio format
provided from userspace and currently selected one on the codec.

Add parsing helpers to support loading such information from the
topology file.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/topology.c | 158 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |  27 ++++++
 2 files changed, 185 insertions(+)

diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 8889ceae19b9..92f7b9a361e1 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -1181,3 +1181,161 @@ avs_tplg_pipeline_create(struct snd_soc_component *comp, struct avs_tplg_path *o
 
 	return pipeline;
 }
+
+static const struct avs_tplg_token_parser path_parsers[] = {
+	{
+		.token = AVS_TKN_PATH_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_path, id),
+		.parse = avs_parse_word_token,
+	},
+	{
+		.token = AVS_TKN_PATH_FE_FMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_path, fe_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+	{
+		.token = AVS_TKN_PATH_BE_FMT_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_path, be_fmt),
+		.parse = avs_parse_audio_format_ptr,
+	},
+};
+
+static struct avs_tplg_path *
+avs_tplg_path_create(struct snd_soc_component *comp, struct avs_tplg_path_template *owner,
+		     struct snd_soc_tplg_vendor_array *tuples, u32 block_size,
+		     const struct avs_tplg_token_parser *parsers, u32 num_parsers)
+{
+	struct avs_tplg_pipeline *pipeline;
+	struct avs_tplg_path *path;
+	u32 offset;
+	int ret;
+
+	path = devm_kzalloc(comp->card->dev, sizeof(*path), GFP_KERNEL);
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	path->owner = owner;
+	INIT_LIST_HEAD(&path->ppl_list);
+	INIT_LIST_HEAD(&path->node);
+
+	/* Path header MAY be followed by one or more pipelines. */
+	ret = avs_tplg_vendor_array_lookup(tuples, block_size,
+					   AVS_TKN_PPL_ID_U32, &offset);
+	if (ret == -ENOENT)
+		offset = block_size;
+	else if (ret)
+		return ERR_PTR(ret);
+	else if (!offset)
+		return ERR_PTR(-EINVAL);
+
+	/* Process header which precedes pipeline sections. */
+	ret = avs_parse_tokens(comp, path, parsers, num_parsers, tuples, offset);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	block_size -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+	while (block_size > 0) {
+		u32 esize;
+
+		ret = avs_tplg_vendor_entry_size(tuples, block_size,
+						 AVS_TKN_PPL_ID_U32, &esize);
+		if (ret)
+			return ERR_PTR(ret);
+
+		pipeline = avs_tplg_pipeline_create(comp, path, tuples, esize);
+		if (IS_ERR(pipeline)) {
+			dev_err(comp->dev, "parse pipeline failed: %ld\n",
+				PTR_ERR(pipeline));
+			return ERR_CAST(pipeline);
+		}
+
+		list_add_tail(&pipeline->node, &path->ppl_list);
+		block_size -= esize;
+		tuples = avs_tplg_vendor_array_at(tuples, esize);
+	}
+
+	return path;
+}
+
+static const struct avs_tplg_token_parser path_tmpl_parsers[] = {
+	{
+		.token = AVS_TKN_PATH_TMPL_ID_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg_path_template, id),
+		.parse = avs_parse_word_token,
+	},
+};
+
+static int parse_path_template(struct snd_soc_component *comp,
+			       struct snd_soc_tplg_vendor_array *tuples, u32 block_size,
+			       struct avs_tplg_path_template *template,
+			       const struct avs_tplg_token_parser *tmpl_tokens, u32 num_tmpl_tokens,
+			       const struct avs_tplg_token_parser *path_tokens, u32 num_path_tokens)
+{
+	struct avs_tplg_path *path;
+	u32 offset;
+	int ret;
+
+	/* Path template header MUST be followed by at least one path variant. */
+	ret = avs_tplg_vendor_array_lookup(tuples, block_size,
+					   AVS_TKN_PATH_ID_U32, &offset);
+	if (ret)
+		return ret;
+
+	/* Process header which precedes path variants sections. */
+	ret = avs_parse_tokens(comp, template, tmpl_tokens, num_tmpl_tokens, tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	block_size -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+	do {
+		u32 esize;
+
+		ret = avs_tplg_vendor_entry_size(tuples, block_size,
+						 AVS_TKN_PATH_ID_U32, &esize);
+		if (ret)
+			return ret;
+
+		path = avs_tplg_path_create(comp, template, tuples, esize, path_tokens,
+					    num_path_tokens);
+		if (IS_ERR(path)) {
+			dev_err(comp->dev, "parse path failed: %ld\n", PTR_ERR(path));
+			return PTR_ERR(path);
+		}
+
+		list_add_tail(&path->node, &template->path_list);
+		block_size -= esize;
+		tuples = avs_tplg_vendor_array_at(tuples, esize);
+	} while (block_size > 0);
+
+	return 0;
+}
+
+static struct avs_tplg_path_template *
+avs_tplg_path_template_create(struct snd_soc_component *comp, struct avs_tplg *owner,
+			      struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
+{
+	struct avs_tplg_path_template *template;
+	int ret;
+
+	template = devm_kzalloc(comp->card->dev, sizeof(*template), GFP_KERNEL);
+	if (!template)
+		return ERR_PTR(-ENOMEM);
+
+	template->owner = owner; /* Used when building sysfs hierarchy. */
+	INIT_LIST_HEAD(&template->path_list);
+	INIT_LIST_HEAD(&template->node);
+
+	ret = parse_path_template(comp, tuples, block_size, template, path_tmpl_parsers,
+				  ARRAY_SIZE(path_tmpl_parsers), path_parsers,
+				  ARRAY_SIZE(path_parsers));
+	if (ret)
+		return ERR_PTR(ret);
+
+	return template;
+}
diff --git a/sound/soc/intel/avs/topology.h b/sound/soc/intel/avs/topology.h
index b68c73afcde3..383931ab1d77 100644
--- a/sound/soc/intel/avs/topology.h
+++ b/sound/soc/intel/avs/topology.h
@@ -33,6 +33,8 @@ struct avs_tplg {
 	u32 num_pplcfgs;
 	struct avs_tplg_binding *bindings;
 	u32 num_bindings;
+
+	struct list_head path_tmpl_list;
 };
 
 struct avs_tplg_library {
@@ -129,8 +131,33 @@ struct avs_tplg_binding {
 	u8 is_sink;
 };
 
+struct avs_tplg_path_template_id {
+	u32 id;
+	char tplg_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+};
+
+struct avs_tplg_path_template {
+	u32 id;
+
+	struct list_head path_list;
+
+	struct avs_tplg *owner;
+	/* Driver path templates management. */
+	struct list_head node;
+};
+
 struct avs_tplg_path {
 	u32 id;
+
+	/* Path format requirements. */
+	struct avs_audio_format *fe_fmt;
+	struct avs_audio_format *be_fmt;
+
+	struct list_head ppl_list;
+
+	struct avs_tplg_path_template *owner;
+	/* Path template path-variants management. */
+	struct list_head node;
 };
 
 struct avs_tplg_pipeline {
-- 
2.25.1

Re: [RFC 06/13] ASoC: Intel: avs: Parse path and path templates tuples

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> Path template is a pattern like its name suggests. It is tied to DAPM

the name really doesn't suggest anything to me, and I have no idea what
a 'pattern' represents for graph management.

You really ought to uplevel and expose the concepts earlier

> widget which represents a FE or a BE and is used during path

'a widget which represents a FE or a BE'. I do not see a 1:1
relationship between widget and FE/BE. these are different concepts/levels.

> instantiation when substream is opened for streaming. It carries a range
> of available variants - actual implementation of a runtime path on
> AudioDSP side. Only one variant of given path template can be
> instantiated at a time and selection is based off of audio format
> provided from userspace and currently selected one on the codec.

well, the last part is the fundamental problem that I am trying to explain.

The codec rate is not necessarily fixed as you seem to assume. There are
many cases where the codec rate can be changed depending on the audio
format changes happening in the DSP.

It is an invalid assumption to assume the codec rate is selected
arbitrarily. It's often the case but that's more of a DPCM limitation
than a design guiding principle we should build on.


> +static struct avs_tplg_path_template *
> +avs_tplg_path_template_create(struct snd_soc_component *comp, struct avs_tplg *owner,
> +			      struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
> +{
> +	struct avs_tplg_path_template *template;
> +	int ret;
> +
> +	template = devm_kzalloc(comp->card->dev, sizeof(*template), GFP_KERNEL);
> +	if (!template)
> +		return ERR_PTR(-ENOMEM);
> +
> +	template->owner = owner; /* Used when building sysfs hierarchy. */

sysfs is a showstopper if the intent is to let userspace modify the
hierarchy.

Even for read-only information, it's not clear to me what application
would ever read this information. debugfs seems more appropriate?

please clarify what the intended use might be.


> +	INIT_LIST_HEAD(&template->path_list);
> +	INIT_LIST_HEAD(&template->node);
> +
> +	ret = parse_path_template(comp, tuples, block_size, template, path_tmpl_parsers,
> +				  ARRAY_SIZE(path_tmpl_parsers), path_parsers,
> +				  ARRAY_SIZE(path_parsers));
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return template;
> +}

>  struct avs_tplg_path {
>  	u32 id;
> +
> +	/* Path format requirements. */
> +	struct avs_audio_format *fe_fmt;
> +	struct avs_audio_format *be_fmt;

this doesn't seem quite right or assumes a very narrow set of DPCM uses.

First I am not sure why there is only one format possible on both FE and
BE sides. If you have multiples paths to represent all possible
combinations of FE and BE formats, then it will become really confusing
really quickly.

This representation would also not scale if there are multiple FEs are
connected to the same BE, or conversely one FE is connected to multiple
BEs. It's also quite possible that the different BE and FE have
different formats, e.g. you could mix 24 and 32 bit data.

In addition, it is a really bad assumption to think that the BE is
independent of the FE. In cases where its format can be adjusted, we
would want constraints to be identified and select the 'best' possible
BE format.

> +
> +	struct list_head ppl_list;
> +
> +	struct avs_tplg_path_template *owner;
> +	/* Path template path-variants management. */
> +	struct list_head node;
>  };
>  
>  struct avs_tplg_pipeline {

Re: [RFC 06/13] ASoC: Intel: avs: Parse path and path templates tuples

[Cezary Rojewski]

On 2022-02-25 8:09 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> Path template is a pattern like its name suggests. It is tied to DAPM
> 
> the name really doesn't suggest anything to me, and I have no idea what
> a 'pattern' represents for graph management.
> 
> You really ought to uplevel and expose the concepts earlier


Again, such 'concept' already exists in kernel since skylake-driver. 
Topology never matched runtime 1:1, you are going to have some kind of 
template or pattern that you later convert into actual runtime stream.

Please state what would you like to see here as nether the ASoC topology 
nor the "template/pattern" is new here and I'm having trouble 
understanding what's need to be added.

>> widget which represents a FE or a BE and is used during path
> 
> 'a widget which represents a FE or a BE'. I do not see a 1:1
> relationship between widget and FE/BE. these are different concepts/levels.


Huh? For skylake-driver you have a widget per module which together make 
FE or BE. We simplified this here as duplicating widgets for no reason 
is not good.

>> instantiation when substream is opened for streaming. It carries a range
>> of available variants - actual implementation of a runtime path on
>> AudioDSP side. Only one variant of given path template can be
>> instantiated at a time and selection is based off of audio format
>> provided from userspace and currently selected one on the codec.
> 
> well, the last part is the fundamental problem that I am trying to explain.
> 
> The codec rate is not necessarily fixed as you seem to assume. There are
> many cases where the codec rate can be changed depending on the audio
> format changes happening in the DSP.
> 
> It is an invalid assumption to assume the codec rate is selected
> arbitrarily. It's often the case but that's more of a DPCM limitation
> than a design guiding principle we should build on.

That case is perfectly fine and is supported by the topology implemented 
here. I don't understand what's the issue here. Perhaps something is not 
worded correctly in the description.

>> +static struct avs_tplg_path_template *
>> +avs_tplg_path_template_create(struct snd_soc_component *comp, struct avs_tplg *owner,
>> +			      struct snd_soc_tplg_vendor_array *tuples, u32 block_size)
>> +{
>> +	struct avs_tplg_path_template *template;
>> +	int ret;
>> +
>> +	template = devm_kzalloc(comp->card->dev, sizeof(*template), GFP_KERNEL);
>> +	if (!template)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	template->owner = owner; /* Used when building sysfs hierarchy. */
> 
> sysfs is a showstopper if the intent is to let userspace modify the
> hierarchy.
> 
> Even for read-only information, it's not clear to me what application
> would ever read this information. debugfs seems more appropriate?
> 
> please clarify what the intended use might be.


I'll drop the 'owner' part and move it into a separate subject. The 
intent is not to modify the hierarchy, it's for having something to hook 
to to read info during runtime from DPS.

>> +	INIT_LIST_HEAD(&template->path_list);
>> +	INIT_LIST_HEAD(&template->node);
>> +
>> +	ret = parse_path_template(comp, tuples, block_size, template, path_tmpl_parsers,
>> +				  ARRAY_SIZE(path_tmpl_parsers), path_parsers,
>> +				  ARRAY_SIZE(path_parsers));
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	return template;
>> +}
> 
>>   struct avs_tplg_path {
>>   	u32 id;
>> +
>> +	/* Path format requirements. */
>> +	struct avs_audio_format *fe_fmt;
>> +	struct avs_audio_format *be_fmt;
> 
> this doesn't seem quite right or assumes a very narrow set of DPCM uses.
> 
> First I am not sure why there is only one format possible on both FE and
> BE sides. If you have multiples paths to represent all possible
> combinations of FE and BE formats, then it will become really confusing
> really quickly.
> 
> This representation would also not scale if there are multiple FEs are
> connected to the same BE, or conversely one FE is connected to multiple
> BEs. It's also quite possible that the different BE and FE have
> different formats, e.g. you could mix 24 and 32 bit data.
> 
> In addition, it is a really bad assumption to think that the BE is
> independent of the FE. In cases where its format can be adjusted, we
> would want constraints to be identified and select the 'best' possible
> BE format.

struct avs_path_path_template can have a large list of struct 
avs_tplg_path defined, so it's not limited to one format. Remember that 
each format combination has its implication in form of need for 
different modules being engaged, changes in number of pipelines running 
and so on. And there's no running away from that. So, regardless of how 
you layout the struct which represents a "path" each combination will 
need a separate instance of it for its representation. Otherwise we 
enter the world where kernel driver has additional logic implemented so 
it modifies 'path' layouts on the fly. And that's a very dangerous path, 
especially when considering long term maintenance and backward 
compatibility subject.

Why would it not scale if multiple FEs are connected to the same BE? FE 
paths attached to single BE can have SRC/UpDownMixers modules within 
them to help with conversions. Remember that you have to take 
copier/mixin/mixout/fw modules limitations into account. You cannot just 
do random stuff and expect firmware to cope with that.

Sure, we want to select the best possible format. That's why you don't 
have to have different FE/BE format. It's a flexible approach.

>> +
>> +	struct list_head ppl_list;
>> +
>> +	struct avs_tplg_path_template *owner;
>> +	/* Path template path-variants management. */
>> +	struct list_head node;
>>   };
>>   
>>   struct avs_tplg_pipeline {

[RFC 07/13] ASoC: Intel: avs: Add topology loading operations

[Cezary Rojewski]

AVS topology is split into two major parts: dictionaries - found within
ASoC topology manifest - and path templates - found within DAPM widget
private data. Add custom handlers for a range of operations available
in struct snd_soc_tplg_ops to allow for actually loading the topology
file.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/Kconfig        |   1 +
 sound/soc/intel/avs/Makefile   |   3 +-
 sound/soc/intel/avs/avs.h      |   2 +
 sound/soc/intel/avs/topology.c | 259 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |   5 +
 5 files changed, 269 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 96aa702d831d..f0688c3d76a7 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -216,6 +216,7 @@ config SND_SOC_INTEL_AVS
 	depends on SND_SOC_INTEL_SKYLAKE_FAMILY=n
 	default n
 	select SND_SOC_ACPI
+	select SND_SOC_TOPOLOGY
 	select SND_HDA_EXT_CORE
 	select SND_HDA_DSP_LOADER
 	help
diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile
index f842bfc5e97e..5e12a3a89e64 100644
--- a/sound/soc/intel/avs/Makefile
+++ b/sound/soc/intel/avs/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-snd-soc-avs-objs := dsp.o ipc.o messages.o utils.o core.o loader.o
+snd-soc-avs-objs := dsp.o ipc.o messages.o utils.o core.o loader.o \
+		    topology.o
 snd-soc-avs-objs += cldma.o
 
 obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index 61842720c894..b0c17c45d7c6 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -237,4 +237,6 @@ struct avs_soc_component {
 #define to_avs_soc_component(comp) \
 	container_of(comp, struct avs_soc_component, base)
 
+extern const struct snd_soc_dai_ops avs_dai_fe_ops;
+
 #endif /* __SOUND_SOC_INTEL_AVS_H */
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 92f7b9a361e1..d7390359c39c 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -6,6 +6,7 @@
 //          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
 //
 
+#include <linux/firmware.h>
 #include <linux/uuid.h>
 #include <sound/soc.h>
 #include <sound/soc-acpi.h>
@@ -14,6 +15,8 @@
 #include "avs.h"
 #include "topology.h"
 
+const struct snd_soc_dai_ops avs_dai_fe_ops;
+
 /* Get pointer to vendor array at the specified offset. */
 #define avs_tplg_vendor_array_at(array, offset) \
 	((struct snd_soc_tplg_vendor_array *)((u8 *)array + offset))
@@ -1339,3 +1342,259 @@ avs_tplg_path_template_create(struct snd_soc_component *comp, struct avs_tplg *o
 
 	return template;
 }
+
+static int avs_route_load(struct snd_soc_component *comp, int index,
+			  struct snd_soc_dapm_route *route)
+{
+	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
+	size_t len = SNDRV_CTL_ELEM_ID_NAME_MAXLEN;
+	char buf[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+	u32 port;
+
+	/* See parse_link_formatted_string() for dynamic naming when(s). */
+	if (hweight_long(mach->link_mask) == 1) {
+		port = __ffs(mach->link_mask);
+
+		snprintf(buf, len, route->source, port);
+		strncpy((char *)route->source, buf, len);
+		snprintf(buf, len, route->sink, port);
+		strncpy((char *)route->sink, buf, len);
+		if (route->control) {
+			snprintf(buf, len, route->control, port);
+			strncpy((char *)route->control, buf, len);
+		}
+	}
+
+	return 0;
+}
+
+static int avs_widget_load(struct snd_soc_component *comp, int index,
+			   struct snd_soc_dapm_widget *w,
+			   struct snd_soc_tplg_dapm_widget *dw)
+{
+	struct snd_soc_acpi_mach *mach;
+	struct avs_tplg_path_template *template;
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	struct avs_tplg *tplg;
+
+	if (!le32_to_cpu(dw->priv.size))
+		return 0;
+
+	tplg = acomp->tplg;
+	mach = dev_get_platdata(comp->card->dev);
+
+	/* See parse_link_formatted_string() for dynamic naming when(s). */
+	if (hweight_long(mach->link_mask) == 1) {
+		kfree(w->name);
+		/* ->name is freed later by soc_tplg_dapm_widget_create() */
+		w->name = kasprintf(GFP_KERNEL, dw->name, __ffs(mach->link_mask));
+		if (!w->name)
+			return -ENOMEM;
+	}
+
+	template = avs_tplg_path_template_create(comp, tplg, dw->priv.array,
+						 le32_to_cpu(dw->priv.size));
+	if (IS_ERR(template)) {
+		dev_err(comp->dev, "widget %s load failed: %ld\n", dw->name,
+			PTR_ERR(template));
+		return PTR_ERR(template);
+	}
+
+	w->priv = template; /* link path information to widget */
+	list_add_tail(&template->node, &tplg->path_tmpl_list);
+	return 0;
+}
+
+static int avs_dai_load(struct snd_soc_component *comp, int index,
+			struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm,
+			struct snd_soc_dai *dai)
+{
+	if (pcm)
+		dai_drv->ops = &avs_dai_fe_ops;
+	return 0;
+}
+
+static int avs_link_load(struct snd_soc_component *comp, int index, struct snd_soc_dai_link *link,
+			 struct snd_soc_tplg_link_config *cfg)
+{
+	/* Stream control handled by IPCs. */
+	link->nonatomic = true;
+
+	if (!link->no_pcm) {
+		/* Open LINK (BE) pipes last and close them first to prevent xruns. */
+		link->trigger[0] = SND_SOC_DPCM_TRIGGER_PRE;
+		link->trigger[1] = SND_SOC_DPCM_TRIGGER_PRE;
+	}
+
+	return 0;
+}
+
+static const struct avs_tplg_token_parser manifest_parsers[] = {
+	{
+		.token = AVS_TKN_MANIFEST_NAME_STRING,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_STRING,
+		.offset = offsetof(struct avs_tplg, name),
+		.parse = parse_link_formatted_string,
+	},
+	{
+		.token = AVS_TKN_MANIFEST_VERSION_U32,
+		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
+		.offset = offsetof(struct avs_tplg, version),
+		.parse = avs_parse_word_token,
+	},
+};
+
+static int avs_manifest(struct snd_soc_component *comp, int index,
+			struct snd_soc_tplg_manifest *manifest)
+{
+	struct snd_soc_tplg_vendor_array *tuples = manifest->priv.array;
+	struct avs_soc_component *acomp = to_avs_soc_component(comp);
+	size_t remaining = le32_to_cpu(manifest->priv.size);
+	u32 offset;
+	int ret;
+
+	ret = avs_tplg_vendor_array_lookup(tuples, remaining,
+					   AVS_TKN_MANIFEST_NUM_LIBRARIES_U32, &offset);
+	/* Manifest MUST begin with a header. */
+	if (!ret && !offset)
+		ret = -EINVAL;
+	if (ret) {
+		dev_err(comp->dev, "incorrect manifest format: %d\n", ret);
+		return ret;
+	}
+
+	/* Process header which precedes any of the dictionaries. */
+	ret = avs_parse_tokens(comp, acomp->tplg, manifest_parsers,
+			       ARRAY_SIZE(manifest_parsers), tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	remaining -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	ret = avs_tplg_vendor_array_lookup(tuples, remaining,
+					   AVS_TKN_MANIFEST_NUM_AFMTS_U32, &offset);
+	if (ret) {
+		dev_err(comp->dev, "audio formats lookup failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Libraries dictionary. */
+	ret = avs_tplg_parse_libraries(comp, tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	remaining -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	ret = avs_tplg_vendor_array_lookup(tuples, remaining,
+					   AVS_TKN_MANIFEST_NUM_MODCFGS_BASE_U32, &offset);
+	if (ret) {
+		dev_err(comp->dev, "modcfgs_base lookup failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Audio formats dictionary. */
+	ret = avs_tplg_parse_audio_formats(comp, tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	remaining -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	ret = avs_tplg_vendor_array_lookup(tuples, remaining,
+					   AVS_TKN_MANIFEST_NUM_MODCFGS_EXT_U32, &offset);
+	if (ret) {
+		dev_err(comp->dev, "modcfgs_ext lookup failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Module configs-base dictionary. */
+	ret = avs_tplg_parse_modcfgs_base(comp, tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	remaining -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	ret = avs_tplg_vendor_array_lookup(tuples, remaining,
+					   AVS_TKN_MANIFEST_NUM_PPLCFGS_U32, &offset);
+	if (ret) {
+		dev_err(comp->dev, "pplcfgs lookup failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Module configs-ext dictionary. */
+	ret = avs_tplg_parse_modcfgs_ext(comp, tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	remaining -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	ret = avs_tplg_vendor_array_lookup(tuples, remaining,
+					   AVS_TKN_MANIFEST_NUM_BINDINGS_U32, &offset);
+	if (ret) {
+		dev_err(comp->dev, "bindings lookup failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Pipeline configs dictionary. */
+	ret = avs_tplg_parse_pplcfgs(comp, tuples, offset);
+	if (ret < 0)
+		return ret;
+
+	remaining -= offset;
+	tuples = avs_tplg_vendor_array_at(tuples, offset);
+
+	/* Bindings dictionary. */
+	return avs_tplg_parse_bindings(comp, tuples, remaining);
+}
+
+static struct snd_soc_tplg_ops avs_tplg_ops = {
+	.dapm_route_load	= avs_route_load,
+	.widget_load		= avs_widget_load,
+	.dai_load		= avs_dai_load,
+	.link_load		= avs_link_load,
+	.manifest		= avs_manifest,
+};
+
+struct avs_tplg *avs_tplg_new(struct snd_soc_component *comp)
+{
+	struct avs_tplg *tplg;
+
+	tplg = devm_kzalloc(comp->card->dev, sizeof(*tplg), GFP_KERNEL);
+	if (!tplg)
+		return NULL;
+
+	tplg->comp = comp;
+	INIT_LIST_HEAD(&tplg->path_tmpl_list);
+
+	return tplg;
+}
+
+int avs_load_topology(struct snd_soc_component *comp, const char *filename)
+{
+	const struct firmware *fw;
+	int ret;
+
+	ret = request_firmware(&fw, filename, comp->dev);
+	if (ret < 0) {
+		dev_err(comp->dev, "request topology \"%s\" failed: %d\n", filename, ret);
+		return ret;
+	}
+
+	ret = snd_soc_tplg_component_load(comp, &avs_tplg_ops, fw);
+	if (ret < 0)
+		dev_err(comp->dev, "load topology \"%s\" failed: %d\n", filename, ret);
+
+	release_firmware(fw);
+	return ret;
+}
+
+int avs_remove_topology(struct snd_soc_component *comp)
+{
+	snd_soc_tplg_component_remove(comp);
+
+	return 0;
+}
diff --git a/sound/soc/intel/avs/topology.h b/sound/soc/intel/avs/topology.h
index 383931ab1d77..6058d868f802 100644
--- a/sound/soc/intel/avs/topology.h
+++ b/sound/soc/intel/avs/topology.h
@@ -187,4 +187,9 @@ struct avs_tplg_module {
 	struct list_head node;
 };
 
+struct avs_tplg *avs_tplg_new(struct snd_soc_component *comp);
+
+int avs_load_topology(struct snd_soc_component *comp, const char *filename);
+int avs_remove_topology(struct snd_soc_component *comp);
+
 #endif
-- 
2.25.1

Re: [RFC 07/13] ASoC: Intel: avs: Add topology loading operations

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> AVS topology is split into two major parts: dictionaries - found within
> ASoC topology manifest - and path templates - found within DAPM widget
> private data. 

Well, one would hope that the path templates are initially represented
in the topology and set when parsing the topology.

If not, who defines that those 'path templates' are?

It's also unclear which 'DAPM widget' you are referring to?


> +static int avs_route_load(struct snd_soc_component *comp, int index,
> +			  struct snd_soc_dapm_route *route)

I have to ask: what is the difference between stream, path, path
template, route?

> +{
> +	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
> +	size_t len = SNDRV_CTL_ELEM_ID_NAME_MAXLEN;
> +	char buf[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	u32 port;
> +
> +	/* See parse_link_formatted_string() for dynamic naming when(s). */
> +	if (hweight_long(mach->link_mask) == 1) {
> +		port = __ffs(mach->link_mask);
> +
> +		snprintf(buf, len, route->source, port);
> +		strncpy((char *)route->source, buf, len);
> +		snprintf(buf, len, route->sink, port);
> +		strncpy((char *)route->sink, buf, len);
> +		if (route->control) {
> +			snprintf(buf, len, route->control, port);
> +			strncpy((char *)route->control, buf, len);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int avs_widget_load(struct snd_soc_component *comp, int index,
> +			   struct snd_soc_dapm_widget *w,
> +			   struct snd_soc_tplg_dapm_widget *dw)
> +{
> +	struct snd_soc_acpi_mach *mach;
> +	struct avs_tplg_path_template *template;
> +	struct avs_soc_component *acomp = to_avs_soc_component(comp);
> +	struct avs_tplg *tplg;
> +
> +	if (!le32_to_cpu(dw->priv.size))
> +		return 0;
> +
> +	tplg = acomp->tplg;
> +	mach = dev_get_platdata(comp->card->dev);
> +
> +	/* See parse_link_formatted_string() for dynamic naming when(s). */
> +	if (hweight_long(mach->link_mask) == 1) {
> +		kfree(w->name);
> +		/* ->name is freed later by soc_tplg_dapm_widget_create() */

->name? missing something here
w->name?

> +		w->name = kasprintf(GFP_KERNEL, dw->name, __ffs(mach->link_mask));
> +		if (!w->name)
> +			return -ENOMEM;
> +	}
> +
> +	template = avs_tplg_path_template_create(comp, tplg, dw->priv.array,
> +						 le32_to_cpu(dw->priv.size));
> +	if (IS_ERR(template)) {
> +		dev_err(comp->dev, "widget %s load failed: %ld\n", dw->name,
> +			PTR_ERR(template));
> +		return PTR_ERR(template);
> +	}
> +
> +	w->priv = template; /* link path information to widget */
> +	list_add_tail(&template->node, &tplg->path_tmpl_list);
> +	return 0;
> +}
> +
> +static int avs_dai_load(struct snd_soc_component *comp, int index,
> +			struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm,
> +			struct snd_soc_dai *dai)
> +{
> +	if (pcm)
> +		dai_drv->ops = &avs_dai_fe_ops;
> +	return 0;
> +}
> +
> +static int avs_link_load(struct snd_soc_component *comp, int index, struct snd_soc_dai_link *link,
> +			 struct snd_soc_tplg_link_config *cfg)
> +{
> +	/* Stream control handled by IPCs. */
> +	link->nonatomic = true;

if this routine also takes care of BE dailinks, then it's not quite
correct to set nonatomic here.

Should this be set only within the test below?

> +
> +	if (!link->no_pcm) {
> +		/* Open LINK (BE) pipes last and close them first to prevent xruns. */
> +		link->trigger[0] = SND_SOC_DPCM_TRIGGER_PRE;
> +		link->trigger[1] = SND_SOC_DPCM_TRIGGER_PRE;
> +	}
> +
> +	return 0;
> +}

Re: [RFC 07/13] ASoC: Intel: avs: Add topology loading operations

[Cezary Rojewski]

On 2022-02-25 8:17 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> AVS topology is split into two major parts: dictionaries - found within
>> ASoC topology manifest - and path templates - found within DAPM widget
>> private data.
> 
> Well, one would hope that the path templates are initially represented
> in the topology and set when parsing the topology.
> 
> If not, who defines that those 'path templates' are?
> 
> It's also unclear which 'DAPM widget' you are referring to?

Just like skylake-driver, avs-driver has several dictionaries which 
provide a list of primitive elements each (e.g. module configs) so the 
more 'advanced' structures such as struct avs_tplg_path_template can 
refer to them later.

Yes, path templates are represented in the topology file and are set to 
instance of struct avs_tplg_path_template each when the file is being 
parsed.

DAPM widget - widget which represents either FE or BE path.

>> +static int avs_route_load(struct snd_soc_component *comp, int index,
>> +			  struct snd_soc_dapm_route *route)
> 
> I have to ask: what is the difference between stream, path, path
> template, route?

That's a ->route_load() topology operation. So, the route in known 
upfront from ASoC topology documentation.

All others were already explained earlier in the series as it's not the 
first time question is being asked. Trying to keep the number of 
'threads' in check so it's easier to follow.

>> +{
>> +	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
>> +	size_t len = SNDRV_CTL_ELEM_ID_NAME_MAXLEN;
>> +	char buf[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
>> +	u32 port;
>> +
>> +	/* See parse_link_formatted_string() for dynamic naming when(s). */
>> +	if (hweight_long(mach->link_mask) == 1) {
>> +		port = __ffs(mach->link_mask);
>> +
>> +		snprintf(buf, len, route->source, port);
>> +		strncpy((char *)route->source, buf, len);
>> +		snprintf(buf, len, route->sink, port);
>> +		strncpy((char *)route->sink, buf, len);
>> +		if (route->control) {
>> +			snprintf(buf, len, route->control, port);
>> +			strncpy((char *)route->control, buf, len);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int avs_widget_load(struct snd_soc_component *comp, int index,
>> +			   struct snd_soc_dapm_widget *w,
>> +			   struct snd_soc_tplg_dapm_widget *dw)
>> +{
>> +	struct snd_soc_acpi_mach *mach;
>> +	struct avs_tplg_path_template *template;
>> +	struct avs_soc_component *acomp = to_avs_soc_component(comp);
>> +	struct avs_tplg *tplg;
>> +
>> +	if (!le32_to_cpu(dw->priv.size))
>> +		return 0;
>> +
>> +	tplg = acomp->tplg;
>> +	mach = dev_get_platdata(comp->card->dev);
>> +
>> +	/* See parse_link_formatted_string() for dynamic naming when(s). */
>> +	if (hweight_long(mach->link_mask) == 1) {
>> +		kfree(w->name);
>> +		/* ->name is freed later by soc_tplg_dapm_widget_create() */
> 
> ->name? missing something here
> w->name?

Ack, thanks!

>> +		w->name = kasprintf(GFP_KERNEL, dw->name, __ffs(mach->link_mask));
>> +		if (!w->name)
>> +			return -ENOMEM;
>> +	}
>> +
>> +	template = avs_tplg_path_template_create(comp, tplg, dw->priv.array,
>> +						 le32_to_cpu(dw->priv.size));
>> +	if (IS_ERR(template)) {
>> +		dev_err(comp->dev, "widget %s load failed: %ld\n", dw->name,
>> +			PTR_ERR(template));
>> +		return PTR_ERR(template);
>> +	}
>> +
>> +	w->priv = template; /* link path information to widget */
>> +	list_add_tail(&template->node, &tplg->path_tmpl_list);
>> +	return 0;
>> +}
>> +
>> +static int avs_dai_load(struct snd_soc_component *comp, int index,
>> +			struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm,
>> +			struct snd_soc_dai *dai)
>> +{
>> +	if (pcm)
>> +		dai_drv->ops = &avs_dai_fe_ops;
>> +	return 0;
>> +}
>> +
>> +static int avs_link_load(struct snd_soc_component *comp, int index, struct snd_soc_dai_link *link,
>> +			 struct snd_soc_tplg_link_config *cfg)
>> +{
>> +	/* Stream control handled by IPCs. */
>> +	link->nonatomic = true;
> 
> if this routine also takes care of BE dailinks, then it's not quite
> correct to set nonatomic here.
> 
> Should this be set only within the test below?

Hmm.. You're right, there are just FE links being loaded here. Guess 
this one should be moved into the if-statement below.

>> +
>> +	if (!link->no_pcm) {
>> +		/* Open LINK (BE) pipes last and close them first to prevent xruns. */
>> +		link->trigger[0] = SND_SOC_DPCM_TRIGGER_PRE;
>> +		link->trigger[1] = SND_SOC_DPCM_TRIGGER_PRE;
>> +	}
>> +
>> +	return 0;
>> +}
> 

[RFC 08/13] ASoC: Intel: avs: Declare path and its components

[Cezary Rojewski]

Declare representatives for all crucial elements which stream on ADSP
side is made of. That covers pipelines and modules subject which are
presented by struct avs_path_pipeline and avs_path_module respetively.
While struct avs_path_binding and struct avs_path do not represent any
object on firmware side directly, they are needed to help track the
interconnections and membership of every pipeline and module created.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/path.h | 60 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 sound/soc/intel/avs/path.h

diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
new file mode 100644
index 000000000000..ecfb687fdf36
--- /dev/null
+++ b/sound/soc/intel/avs/path.h
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright(c) 2021 Intel Corporation. All rights reserved.
+ *
+ * Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+ *          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+ */
+
+#ifndef __SOUND_SOC_INTEL_AVS_PATH_H
+#define __SOUND_SOC_INTEL_AVS_PATH_H
+
+#include <linux/list.h>
+#include "avs.h"
+#include "topology.h"
+
+struct avs_path {
+	u32 dma_id;
+	struct list_head ppl_list;
+	u32 state;
+
+	struct avs_tplg_path *template;
+	struct avs_dev *owner;
+	/* device path management */
+	struct list_head node;
+};
+
+struct avs_path_pipeline {
+	u8 instance_id;
+	struct list_head mod_list;
+	struct list_head binding_list;
+
+	struct avs_tplg_pipeline *template;
+	struct avs_path *owner;
+	/* path pipelines management */
+	struct list_head node;
+};
+
+struct avs_path_module {
+	u16 module_id;
+	u16 instance_id;
+
+	struct avs_tplg_module *template;
+	struct avs_path_pipeline *owner;
+	/* pipeline modules management */
+	struct list_head node;
+};
+
+struct avs_path_binding {
+	struct avs_path_module *source;
+	u8 source_pin;
+	struct avs_path_module *sink;
+	u8 sink_pin;
+
+	struct avs_tplg_binding *template;
+	struct avs_path_pipeline *owner;
+	/* pipeline bindings management */
+	struct list_head node;
+};
+
+#endif
-- 
2.25.1

Re: [RFC 08/13] ASoC: Intel: avs: Declare path and its components

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> Declare representatives for all crucial elements which stream on ADSP
> side is made of. That covers pipelines and modules subject which are
> presented by struct avs_path_pipeline and avs_path_module respetively.

respectively

> While struct avs_path_binding and struct avs_path do not represent any
> object on firmware side directly, they are needed to help track the
> interconnections and membership of every pipeline and module created.

> +struct avs_path {
> +	u32 dma_id;

that is very surprising...

This would seem to limit the concept of an avs path to a single host DMA
channel, which somewhat contradicts that there can be multiple pipelines
in the same path, or that a path can contain mixers.

And even if this is a single dma, what does this represent? the
stream_tag? the BE DMA for SSP/DMIC?

Please clarify the concepts first, it's frustrating to discover this at
patch 8.

> +	struct list_head ppl_list;
> +	u32 state;
> +
> +	struct avs_tplg_path *template;
> +	struct avs_dev *owner;
> +	/* device path management */
> +	struct list_head node;
> +};

> +struct avs_path_binding {
> +	struct avs_path_module *source;
> +	u8 source_pin;
> +	struct avs_path_module *sink;
> +	u8 sink_pin;
> +
> +	struct avs_tplg_binding *template;

I must admit not clearly seeing how the definitions of
'avs_path_binding' and 'avs_tplg_binding' are related.


More specifically, having a template for something that directly
connects a source to a sink is far from intuitive.

> +	struct avs_path_pipeline *owner;
> +	/* pipeline bindings management */
> +	struct list_head node;
> +};
> +
> +#endif

Re: [RFC 08/13] ASoC: Intel: avs: Declare path and its components

[Cezary Rojewski]

On 2022-02-25 8:25 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> Declare representatives for all crucial elements which stream on ADSP
>> side is made of. That covers pipelines and modules subject which are
>> presented by struct avs_path_pipeline and avs_path_module respetively.
> 
> respectively

Ack.

>> While struct avs_path_binding and struct avs_path do not represent any
>> object on firmware side directly, they are needed to help track the
>> interconnections and membership of every pipeline and module created.
> 
>> +struct avs_path {
>> +	u32 dma_id;
> 
> that is very surprising...
> 
> This would seem to limit the concept of an avs path to a single host DMA
> channel, which somewhat contradicts that there can be multiple pipelines
> in the same path, or that a path can contain mixers.
> 
> And even if this is a single dma, what does this represent? the
> stream_tag? the BE DMA for SSP/DMIC?
> 
> Please clarify the concepts first, it's frustrating to discover this at
> patch 8.

A single path is tied to either FE or BE. So at most to a single, 
user-visible endpoint if it's FE. If there are more FEs, then we move to 
NxFE <-> 1xBE scenario. You can have many pipelines forming the path - 
most of the pipelines do not contain module connected to any gateway 
(HDA/SSP/DMIC etc.) anyway.

Yes, dma_id represents any DMA i.e. HDA stream (DMA), SSP port, DMIC 
fifo etc. And it's not a concept, it's just a member of a structure. 
Similar field exists in skylake-driver topology too (it's called "port").

>> +	struct list_head ppl_list;
>> +	u32 state;
>> +
>> +	struct avs_tplg_path *template;
>> +	struct avs_dev *owner;
>> +	/* device path management */
>> +	struct list_head node;
>> +};
> 
>> +struct avs_path_binding {
>> +	struct avs_path_module *source;
>> +	u8 source_pin;
>> +	struct avs_path_module *sink;
>> +	u8 sink_pin;
>> +
>> +	struct avs_tplg_binding *template;
> 
> I must admit not clearly seeing how the definitions of
> 'avs_path_binding' and 'avs_tplg_binding' are related.
> 
> 
> More specifically, having a template for something that directly
> connects a source to a sink is far from intuitive.

The IDs found within the topology file do not reflect the IDs that are 
going to be assigned dynamically during runtime streaming. We do not 
specify e.g.: pipeline id=8 is made of copier id=3 and copier id=7. 
Firmware has limited number of modules that can be instantiated per type 
so static assignment is a big no no.

>> +	struct avs_path_pipeline *owner;
>> +	/* pipeline bindings management */
>> +	struct list_head node;
>> +};
>> +
>> +#endif

Re: [RFC 08/13] ASoC: Intel: avs: Declare path and its components

[Pierre-Louis Bossart]

>>> +struct avs_path {
>>> +    u32 dma_id;
>>
>> that is very surprising...
>>
>> This would seem to limit the concept of an avs path to a single host DMA
>> channel, which somewhat contradicts that there can be multiple pipelines
>> in the same path, or that a path can contain mixers.
>>
>> And even if this is a single dma, what does this represent? the
>> stream_tag? the BE DMA for SSP/DMIC?
>>
>> Please clarify the concepts first, it's frustrating to discover this at
>> patch 8.
> 
> A single path is tied to either FE or BE. So at most to a single, 
> user-visible endpoint if it's FE. If there are more FEs, then we move to 
> NxFE <-> 1xBE scenario. You can have many pipelines forming the path - 
> most of the pipelines do not contain module connected to any gateway 
> (HDA/SSP/DMIC etc.) anyway.

This should have been explained in the cover letter.

Assuming that there's a single Back-End that can handle all possible 
routing cases is a very narrow interpretation of how DPCM is supposed to 
be used, and it adds quite a few opens on routing changes that can't be 
handled with regular triggers. What happens when not all interfaces are 
handled by the DSP 'gateway' is also interesting.

[RFC 09/13] ASoC: Intel: avs: Path creation and freeing

[Cezary Rojewski]

To implement ASoC PCM operations, DSP path handling is needed. With path
template concept present, information carried by topology file can be
converted into runtime path representation. Each may be composed of
several pipelines and each pipeline can contain a number of processing
modules inside. Number of templates and variants found within topology
may vastly outnumber the total amount of pipelines and modules supported
by AudioDSP firmware simultaneously (in runtime) so none of the IDs are
specified in the topology. These are assigned dynamically when needed
and account for limitations described by FIRMWARE_CONFIG and
HARDWARE_CONFIG basefw parameters.

Paths are created on ->hw_params() and are freed on ->hw_free() ALSA PCM
operations. This choice is based on firmware expectations - need for
complete set of information when attempting to instantiate pipelines and
modules on AudioDSP side. With DMA and audio format provided, search
mechanism tests all path variants available in given path template until
a matching variant is found. Once found, information already available
is combined with all avs_tplg_* pieces pointed by matching path variant.
This finally allows to begin a cascade of IPCs which goes is to reserve
resources and prepare DSP for upcoming audio streaming.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/Makefile |   2 +-
 sound/soc/intel/avs/avs.h    |   6 +
 sound/soc/intel/avs/path.c   | 287 +++++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/path.h   |   6 +
 4 files changed, 300 insertions(+), 1 deletion(-)
 create mode 100644 sound/soc/intel/avs/path.c

diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile
index 5e12a3a89e64..952f51977656 100644
--- a/sound/soc/intel/avs/Makefile
+++ b/sound/soc/intel/avs/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 snd-soc-avs-objs := dsp.o ipc.o messages.o utils.o core.o loader.o \
-		    topology.o
+		    topology.o path.o
 snd-soc-avs-objs += cldma.o
 
 obj-$(CONFIG_SND_SOC_INTEL_AVS) += snd-soc-avs.o
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index b0c17c45d7c6..313001b0455f 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -91,6 +91,12 @@ struct avs_dev {
 	char **lib_names;
 
 	struct completion fw_ready;
+
+	struct list_head comp_list;
+	struct mutex comp_list_mutex;
+	struct list_head path_list;
+	spinlock_t path_list_lock;
+	struct mutex path_mutex;
 };
 
 /* from hda_bus to avs_dev */
diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
new file mode 100644
index 000000000000..272d868bedc9
--- /dev/null
+++ b/sound/soc/intel/avs/path.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2021 Intel Corporation. All rights reserved.
+//
+// Authors: Cezary Rojewski <cezary.rojewski@intel.com>
+//          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
+//
+
+#include <sound/intel-nhlt.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include "avs.h"
+#include "path.h"
+#include "topology.h"
+
+static bool avs_test_hw_params(struct snd_pcm_hw_params *params,
+			       struct avs_audio_format *fmt)
+{
+	return (params_rate(params) == fmt->sampling_freq &&
+		params_channels(params) == fmt->num_channels &&
+		params_physical_width(params) == fmt->bit_depth &&
+		params_width(params) == fmt->valid_bit_depth);
+}
+
+static struct avs_tplg_path *
+avs_path_find_variant(struct avs_dev *adev,
+		      struct avs_tplg_path_template *template,
+		      struct snd_pcm_hw_params *fe_params,
+		      struct snd_pcm_hw_params *be_params)
+{
+	struct avs_tplg_path *variant;
+
+	list_for_each_entry(variant, &template->path_list, node) {
+		dev_dbg(adev->dev, "check FE rate %d chn %d vbd %d bd %d\n",
+			variant->fe_fmt->sampling_freq, variant->fe_fmt->num_channels,
+			variant->fe_fmt->valid_bit_depth, variant->fe_fmt->bit_depth);
+		dev_dbg(adev->dev, "check BE rate %d chn %d vbd %d bd %d\n",
+			variant->be_fmt->sampling_freq, variant->be_fmt->num_channels,
+			variant->be_fmt->valid_bit_depth, variant->be_fmt->bit_depth);
+
+		if (variant->fe_fmt && avs_test_hw_params(fe_params, variant->fe_fmt) &&
+		    variant->be_fmt && avs_test_hw_params(be_params, variant->be_fmt))
+			return variant;
+	}
+
+	return NULL;
+}
+
+static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	kfree(mod);
+}
+
+static struct avs_path_module *
+avs_path_module_create(struct avs_dev *adev,
+		       struct avs_path_pipeline *owner,
+		       struct avs_tplg_module *template)
+{
+	struct avs_path_module *mod;
+	int module_id;
+
+	module_id = avs_get_module_id(adev, &template->cfg_ext->type);
+	if (module_id < 0)
+		return ERR_PTR(module_id);
+
+	mod = kzalloc(sizeof(*mod), GFP_KERNEL);
+	if (!mod)
+		return ERR_PTR(-ENOMEM);
+
+	mod->template = template;
+	mod->module_id = module_id;
+	mod->owner = owner;
+	INIT_LIST_HEAD(&mod->node);
+
+	return mod;
+}
+
+static void avs_path_binding_free(struct avs_dev *adev, struct avs_path_binding *binding)
+{
+	kfree(binding);
+}
+
+static struct avs_path_binding *avs_path_binding_create(struct avs_dev *adev,
+							struct avs_path_pipeline *owner,
+							struct avs_tplg_binding *t)
+{
+	struct avs_path_binding *binding;
+
+	binding = kzalloc(sizeof(*binding), GFP_KERNEL);
+	if (!binding)
+		return ERR_PTR(-ENOMEM);
+
+	binding->template = t;
+	binding->owner = owner;
+	INIT_LIST_HEAD(&binding->node);
+
+	return binding;
+}
+
+static void avs_path_pipeline_free(struct avs_dev *adev,
+				   struct avs_path_pipeline *ppl)
+{
+	struct avs_path_binding *binding, *bsave;
+	struct avs_path_module *mod, *save;
+
+	list_for_each_entry_safe(binding, bsave, &ppl->binding_list, node) {
+		list_del(&binding->node);
+		avs_path_binding_free(adev, binding);
+	}
+
+	avs_dsp_delete_pipeline(adev, ppl->instance_id);
+
+	/* Unload resources occupied by owned modules */
+	list_for_each_entry_safe(mod, save, &ppl->mod_list, node) {
+		avs_dsp_delete_module(adev, mod->module_id, mod->instance_id,
+				      mod->owner->instance_id,
+				      mod->template->core_id);
+		avs_path_module_free(adev, mod);
+	}
+
+	list_del(&ppl->node);
+	kfree(ppl);
+}
+
+static struct avs_path_pipeline *
+avs_path_pipeline_create(struct avs_dev *adev, struct avs_path *owner,
+			 struct avs_tplg_pipeline *template)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_tplg_pplcfg *cfg = template->cfg;
+	struct avs_tplg_module *tmod;
+	int ret, i;
+
+	ppl = kzalloc(sizeof(*ppl), GFP_KERNEL);
+	if (!ppl)
+		return ERR_PTR(-ENOMEM);
+
+	ppl->template = template;
+	ppl->owner = owner;
+	INIT_LIST_HEAD(&ppl->binding_list);
+	INIT_LIST_HEAD(&ppl->mod_list);
+	INIT_LIST_HEAD(&ppl->node);
+
+	ret = avs_dsp_create_pipeline(adev, cfg->req_size, cfg->priority,
+				      cfg->lp, cfg->attributes,
+				      &ppl->instance_id);
+	if (ret) {
+		dev_err(adev->dev, "error creating pipeline %d\n", ret);
+		kfree(ppl);
+		return ERR_PTR(ret);
+	}
+
+	list_for_each_entry(tmod, &template->mod_list, node) {
+		struct avs_path_module *mod;
+
+		mod = avs_path_module_create(adev, ppl, tmod);
+		if (IS_ERR(mod)) {
+			ret = PTR_ERR(mod);
+			dev_err(adev->dev, "error creating module %d\n", ret);
+			goto init_err;
+		}
+
+		list_add_tail(&mod->node, &ppl->mod_list);
+	}
+
+	for (i = 0; i < template->num_bindings; i++) {
+		struct avs_path_binding *binding;
+
+		binding = avs_path_binding_create(adev, ppl, template->bindings[i]);
+		if (IS_ERR(binding)) {
+			ret = PTR_ERR(binding);
+			dev_err(adev->dev, "error creating binding %d\n", ret);
+			goto init_err;
+		}
+
+		list_add_tail(&binding->node, &ppl->binding_list);
+	}
+
+	return ppl;
+
+init_err:
+	avs_path_pipeline_free(adev, ppl);
+	return ERR_PTR(ret);
+}
+
+static int avs_path_init(struct avs_dev *adev, struct avs_path *path,
+			 struct avs_tplg_path *template, u32 dma_id)
+{
+	struct avs_tplg_pipeline *tppl;
+
+	path->owner = adev;
+	path->template = template;
+	path->dma_id = dma_id;
+	INIT_LIST_HEAD(&path->ppl_list);
+	INIT_LIST_HEAD(&path->node);
+
+	/* create all the pipelines */
+	list_for_each_entry(tppl, &template->ppl_list, node) {
+		struct avs_path_pipeline *ppl;
+
+		ppl = avs_path_pipeline_create(adev, path, tppl);
+		if (IS_ERR(ppl))
+			return PTR_ERR(ppl);
+
+		list_add_tail(&ppl->node, &path->ppl_list);
+	}
+
+	spin_lock(&adev->path_list_lock);
+	list_add_tail(&path->node, &adev->path_list);
+	spin_unlock(&adev->path_list_lock);
+
+	return 0;
+}
+
+static void avs_path_free_unlocked(struct avs_path *path)
+{
+	struct avs_path_pipeline *ppl, *save;
+
+	spin_lock(&path->owner->path_list_lock);
+	list_del(&path->node);
+	spin_unlock(&path->owner->path_list_lock);
+
+	list_for_each_entry_safe(ppl, save, &path->ppl_list, node)
+		avs_path_pipeline_free(path->owner, ppl);
+
+	kfree(path);
+}
+
+static struct avs_path *avs_path_create_unlocked(struct avs_dev *adev, u32 dma_id,
+						 struct avs_tplg_path *template)
+{
+	struct avs_soc_component *acomp;
+	struct avs_path *path;
+	int ret;
+
+	acomp = to_avs_soc_component(template->owner->owner->comp);
+
+	path = kzalloc(sizeof(*path), GFP_KERNEL);
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	ret = avs_path_init(adev, path, template, dma_id);
+	if (ret < 0)
+		goto err;
+
+	path->state = AVS_PPL_STATE_INVALID;
+	return path;
+err:
+	avs_path_free_unlocked(path);
+	return ERR_PTR(ret);
+}
+
+void avs_path_free(struct avs_path *path)
+{
+	struct avs_dev *adev = path->owner;
+
+	mutex_lock(&adev->path_mutex);
+	avs_path_free_unlocked(path);
+	mutex_unlock(&adev->path_mutex);
+}
+
+struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
+				 struct avs_tplg_path_template *template,
+				 struct snd_pcm_hw_params *fe_params,
+				 struct snd_pcm_hw_params *be_params)
+{
+	struct avs_tplg_path *variant;
+	struct avs_path *path;
+
+	variant = avs_path_find_variant(adev, template, fe_params, be_params);
+	if (!variant) {
+		dev_err(adev->dev, "no matching variant found\n");
+		return ERR_PTR(-ENOENT);
+	}
+
+	/* Serialize path and its components creation. */
+	mutex_lock(&adev->path_mutex);
+	/* Satisfy needs of avs_path_find_tplg(). */
+	mutex_lock(&adev->comp_list_mutex);
+
+	path = avs_path_create_unlocked(adev, dma_id, variant);
+
+	mutex_unlock(&adev->comp_list_mutex);
+	mutex_unlock(&adev->path_mutex);
+
+	return path;
+}
diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
index ecfb687fdf36..3843e5a062a1 100644
--- a/sound/soc/intel/avs/path.h
+++ b/sound/soc/intel/avs/path.h
@@ -57,4 +57,10 @@ struct avs_path_binding {
 	struct list_head node;
 };
 
+void avs_path_free(struct avs_path *path);
+struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
+				 struct avs_tplg_path_template *template,
+				 struct snd_pcm_hw_params *fe_params,
+				 struct snd_pcm_hw_params *be_params);
+
 #endif
-- 
2.25.1

Re: [RFC 09/13] ASoC: Intel: avs: Path creation and freeing

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> To implement ASoC PCM operations, DSP path handling is needed. With path
> template concept present, information carried by topology file can be
> converted into runtime path representation. Each may be composed of
> several pipelines and each pipeline can contain a number of processing

it's not quite clear how you can have different pipelines with a single
dma_is per path?

> modules inside. Number of templates and variants found within topology
> may vastly outnumber the total amount of pipelines and modules supported
> by AudioDSP firmware simultaneously (in runtime) so none of the IDs are
> specified in the topology. These are assigned dynamically when needed
> and account for limitations described by FIRMWARE_CONFIG and
> HARDWARE_CONFIG basefw parameters.

It's already quite hard to create a topology using static IDs that will
work, this dynamic creation adds an element of risk and validation,
doesn't it?

Can you clarify how you validated this dynamic capability?

> Paths are created on ->hw_params() and are freed on ->hw_free() ALSA PCM
> operations. This choice is based on firmware expectations - need for

So a path seems to be completely tied to an FE then?

That would mean that a 'dai pipeline' with 'mixin-dai-copier' would not
be managed by an avs-path, and would somehow need to be autonomously
created?

You really need to clarify how basic topologies with
mixers/demux/loopbacks are handled.

> complete set of information when attempting to instantiate pipelines and
> modules on AudioDSP side. With DMA and audio format provided, search
> mechanism tests all path variants available in given path template until
> a matching variant is found. Once found, information already available
> is combined with all avs_tplg_* pieces pointed by matching path variant.
> This finally allows to begin a cascade of IPCs which goes is to reserve

this sentence makes no sense.

did you mean 'which goals'?

> resources and prepare DSP for upcoming audio streaming.

> +static struct avs_tplg_path *
> +avs_path_find_variant(struct avs_dev *adev,
> +		      struct avs_tplg_path_template *template,
> +		      struct snd_pcm_hw_params *fe_params,
> +		      struct snd_pcm_hw_params *be_params)
> +{
> +	struct avs_tplg_path *variant;
> +
> +	list_for_each_entry(variant, &template->path_list, node) {
> +		dev_dbg(adev->dev, "check FE rate %d chn %d vbd %d bd %d\n",
> +			variant->fe_fmt->sampling_freq, variant->fe_fmt->num_channels,
> +			variant->fe_fmt->valid_bit_depth, variant->fe_fmt->bit_depth);
> +		dev_dbg(adev->dev, "check BE rate %d chn %d vbd %d bd %d\n",
> +			variant->be_fmt->sampling_freq, variant->be_fmt->num_channels,
> +			variant->be_fmt->valid_bit_depth, variant->be_fmt->bit_depth);
> +
> +		if (variant->fe_fmt && avs_test_hw_params(fe_params, variant->fe_fmt) &&
> +		    variant->be_fmt && avs_test_hw_params(be_params, variant->be_fmt))
> +			return variant;
> +	}

This matching between FE and BE formats is the key problem with this
patchset IMHO.

We need the ability to reconfigure the BE based on constraint matching,
not exact match with a fixed value!

> +
> +	return NULL;
> +}
> +
> +static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod)
> +{
> +	kfree(mod);
> +}
> +
> +static struct avs_path_module *
> +avs_path_module_create(struct avs_dev *adev,
> +		       struct avs_path_pipeline *owner,
> +		       struct avs_tplg_module *template)

so you have templates for paths AND modules?

Completely lost...

I'll skip the rest of this patch.

Re: [RFC 09/13] ASoC: Intel: avs: Path creation and freeing

[Cezary Rojewski]

On 2022-02-25 8:36 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> To implement ASoC PCM operations, DSP path handling is needed. With path
>> template concept present, information carried by topology file can be
>> converted into runtime path representation. Each may be composed of
>> several pipelines and each pipeline can contain a number of processing
> 
> it's not quite clear how you can have different pipelines with a single
> dma_is per path?

Pipelines do not need to have a module that is connected to a gateway.

Layout of a path is for architecture to decide i.e. one takes 
spreadsheet of all the scenarios to be supported and developers/archs 
discuss/decide what's the best and optimal way to shape the paths that 
implement all the possible scenarios.

>> modules inside. Number of templates and variants found within topology
>> may vastly outnumber the total amount of pipelines and modules supported
>> by AudioDSP firmware simultaneously (in runtime) so none of the IDs are
>> specified in the topology. These are assigned dynamically when needed
>> and account for limitations described by FIRMWARE_CONFIG and
>> HARDWARE_CONFIG basefw parameters.
> 
> It's already quite hard to create a topology using static IDs that will
> work, this dynamic creation adds an element of risk and validation,
> doesn't it?
> 
> Can you clarify how you validated this dynamic capability?

Static ID assignment i.e. taking IDs found in the topology file directly 
when instantiating pipelines/modules in runtime is asking for trouble. 
Firmware has its limitations in terms of number of pipelines/modules 
supported simultaneously. Driver has to take these into account.

Here, we send an IPC to obtain all the limitations before any stream 
could be opened for streaming.

>> Paths are created on ->hw_params() and are freed on ->hw_free() ALSA PCM
>> operations. This choice is based on firmware expectations - need for
> 
> So a path seems to be completely tied to an FE then?
> 
> That would mean that a 'dai pipeline' with 'mixin-dai-copier' would not
> be managed by an avs-path, and would somehow need to be autonomously
> created?
> 
> You really need to clarify how basic topologies with
> mixers/demux/loopbacks are handled.

Path is either tied to a single FE or a BE. Don't understand what's 
difficult with handling mixin/copier pipeliens or loopback scenario 
here. We have all the tools necessary to do the job, no?

>> complete set of information when attempting to instantiate pipelines and
>> modules on AudioDSP side. With DMA and audio format provided, search
>> mechanism tests all path variants available in given path template until
>> a matching variant is found. Once found, information already available
>> is combined with all avs_tplg_* pieces pointed by matching path variant.
>> This finally allows to begin a cascade of IPCs which goes is to reserve
> 
> this sentence makes no sense.
> 
> did you mean 'which goals'?

Ack, thanks!

>> resources and prepare DSP for upcoming audio streaming.
> 
>> +static struct avs_tplg_path *
>> +avs_path_find_variant(struct avs_dev *adev,
>> +		      struct avs_tplg_path_template *template,
>> +		      struct snd_pcm_hw_params *fe_params,
>> +		      struct snd_pcm_hw_params *be_params)
>> +{
>> +	struct avs_tplg_path *variant;
>> +
>> +	list_for_each_entry(variant, &template->path_list, node) {
>> +		dev_dbg(adev->dev, "check FE rate %d chn %d vbd %d bd %d\n",
>> +			variant->fe_fmt->sampling_freq, variant->fe_fmt->num_channels,
>> +			variant->fe_fmt->valid_bit_depth, variant->fe_fmt->bit_depth);
>> +		dev_dbg(adev->dev, "check BE rate %d chn %d vbd %d bd %d\n",
>> +			variant->be_fmt->sampling_freq, variant->be_fmt->num_channels,
>> +			variant->be_fmt->valid_bit_depth, variant->be_fmt->bit_depth);
>> +
>> +		if (variant->fe_fmt && avs_test_hw_params(fe_params, variant->fe_fmt) &&
>> +		    variant->be_fmt && avs_test_hw_params(be_params, variant->be_fmt))
>> +			return variant;
>> +	}
> 
> This matching between FE and BE formats is the key problem with this
> patchset IMHO.
> 
> We need the ability to reconfigure the BE based on constraint matching,
> not exact match with a fixed value!

We need to take into account what's set on the codec side too, can't 
just ignore it. Topology is written for concrete configuration, so we 
can crease a file that supports all possible configurations exposed by 
given codec.

>> +
>> +	return NULL;
>> +}
>> +
>> +static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod)
>> +{
>> +	kfree(mod);
>> +}
>> +
>> +static struct avs_path_module *
>> +avs_path_module_create(struct avs_dev *adev,
>> +		       struct avs_path_pipeline *owner,
>> +		       struct avs_tplg_module *template)
> 
> so you have templates for paths AND modules?
> 
> Completely lost...
> 
> I'll skip the rest of this patch.

All the objects here have topology and runtime representation both. 
Again, you cannot just take what's within a static topology file that 
knows nothing about firmware limitations and expect success.

Don't understand what's surprising here. skylake-driver also has a 
separate representation for module within topology and a separate one 
for runtime. Nothing new here.


Regards,
Czarek

Re: [RFC 09/13] ASoC: Intel: avs: Path creation and freeing

[Cezary Rojewski]

On 2022-03-21 6:19 PM, Cezary Rojewski wrote:
> We need to take into account what's set on the codec side too, can't 
> just ignore it. Topology is written for concrete configuration, so we 
> can crease a file that supports all possible configurations exposed by 
> given codec.

By "Topology is written for concrete configuration" I meant 
platform-level configuration e.g.: Skylake with single rt286 codec in 
I2S mode.


Regards,
Czarek

[RFC 10/13] ASoC: Intel: avs: Path state management

[Cezary Rojewski]

Add functions to ease with state changing of all objects found in the
path. Each represents either a BIND/UNBIND or SET_PIPEPILE_STATE IPC.

DSP pipelines follow simple state machine scheme:
CREATE -> RESET -> PAUSE -> RUNNING -> PAUSE -> RESET -> DELETE

There is no STOP, PAUSE serves that purpose instead.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/path.c | 130 +++++++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/path.h |   5 ++
 2 files changed, 135 insertions(+)

diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
index 272d868bedc9..c6db3f091e66 100644
--- a/sound/soc/intel/avs/path.c
+++ b/sound/soc/intel/avs/path.c
@@ -285,3 +285,133 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
 
 	return path;
 }
+
+int avs_path_bind(struct avs_path *path)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_dev *adev = path->owner;
+	int ret;
+
+	list_for_each_entry(ppl, &path->ppl_list, node) {
+		struct avs_path_binding *binding;
+
+		list_for_each_entry(binding, &ppl->binding_list, node) {
+			struct avs_path_module *source, *sink;
+
+			source = binding->source;
+			sink = binding->sink;
+
+			ret = avs_ipc_bind(adev, source->module_id,
+					   source->instance_id, sink->module_id,
+					   sink->instance_id, binding->sink_pin,
+					   binding->source_pin);
+			if (ret) {
+				dev_err(adev->dev, "bind path failed: %d\n", ret);
+				return AVS_IPC_RET(ret);
+			}
+		}
+	}
+
+	return 0;
+}
+
+int avs_path_unbind(struct avs_path *path)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_dev *adev = path->owner;
+	int ret;
+
+	list_for_each_entry(ppl, &path->ppl_list, node) {
+		struct avs_path_binding *binding;
+
+		list_for_each_entry(binding, &ppl->binding_list, node) {
+			struct avs_path_module *source, *sink;
+
+			source = binding->source;
+			sink = binding->sink;
+
+			ret = avs_ipc_unbind(adev, source->module_id,
+					     source->instance_id, sink->module_id,
+					     sink->instance_id, binding->sink_pin,
+					     binding->source_pin);
+			if (ret) {
+				dev_err(adev->dev, "unbind path failed: %d\n", ret);
+				return AVS_IPC_RET(ret);
+			}
+		}
+	}
+
+	return 0;
+}
+
+int avs_path_reset(struct avs_path *path)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_dev *adev = path->owner;
+	int ret;
+
+	if (path->state == AVS_PPL_STATE_RESET)
+		return 0;
+
+	list_for_each_entry(ppl, &path->ppl_list, node) {
+		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
+						 AVS_PPL_STATE_RESET);
+		if (ret) {
+			dev_err(adev->dev, "reset path failed: %d\n", ret);
+			path->state = AVS_PPL_STATE_INVALID;
+			return AVS_IPC_RET(ret);
+		}
+	}
+
+	path->state = AVS_PPL_STATE_RESET;
+	return 0;
+}
+
+int avs_path_pause(struct avs_path *path)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_dev *adev = path->owner;
+	int ret;
+
+	if (path->state == AVS_PPL_STATE_PAUSED)
+		return 0;
+
+	list_for_each_entry_reverse(ppl, &path->ppl_list, node) {
+		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
+						 AVS_PPL_STATE_PAUSED);
+		if (ret) {
+			dev_err(adev->dev, "pause path failed: %d\n", ret);
+			path->state = AVS_PPL_STATE_INVALID;
+			return AVS_IPC_RET(ret);
+		}
+	}
+
+	path->state = AVS_PPL_STATE_PAUSED;
+	return 0;
+}
+
+int avs_path_run(struct avs_path *path, int trigger)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_dev *adev = path->owner;
+	int ret;
+
+	if (path->state == AVS_PPL_STATE_RUNNING && trigger == AVS_TPLG_TRIGGER_AUTO)
+		return 0;
+
+	list_for_each_entry(ppl, &path->ppl_list, node) {
+		if (ppl->template->cfg->trigger != trigger)
+			continue;
+
+		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
+						 AVS_PPL_STATE_RUNNING);
+		if (ret) {
+			dev_err(adev->dev, "run path failed: %d\n", ret);
+			path->state = AVS_PPL_STATE_INVALID;
+			return AVS_IPC_RET(ret);
+		}
+	}
+
+	path->state = AVS_PPL_STATE_RUNNING;
+	return 0;
+}
diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
index 3843e5a062a1..04a06473f04b 100644
--- a/sound/soc/intel/avs/path.h
+++ b/sound/soc/intel/avs/path.h
@@ -62,5 +62,10 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
 				 struct avs_tplg_path_template *template,
 				 struct snd_pcm_hw_params *fe_params,
 				 struct snd_pcm_hw_params *be_params);
+int avs_path_bind(struct avs_path *path);
+int avs_path_unbind(struct avs_path *path);
+int avs_path_reset(struct avs_path *path);
+int avs_path_pause(struct avs_path *path);
+int avs_path_run(struct avs_path *path, int trigger);
 
 #endif
-- 
2.25.1

Re: [RFC 10/13] ASoC: Intel: avs: Path state management

[Pierre-Louis Bossart]

On 2/7/22 07:25, Cezary Rojewski wrote:
> Add functions to ease with state changing of all objects found in the
> path. Each represents either a BIND/UNBIND or SET_PIPEPILE_STATE IPC.

SET_PIPELINE?

> DSP pipelines follow simple state machine scheme:
> CREATE -> RESET -> PAUSE -> RUNNING -> PAUSE -> RESET -> DELETE>
> There is no STOP, PAUSE serves that purpose instead.

that's not fully correct, the STOP can be handled in two steps due to
DMA programming flows.

> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/intel/avs/path.c | 130 +++++++++++++++++++++++++++++++++++++
>  sound/soc/intel/avs/path.h |   5 ++
>  2 files changed, 135 insertions(+)
> 
> diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
> index 272d868bedc9..c6db3f091e66 100644
> --- a/sound/soc/intel/avs/path.c
> +++ b/sound/soc/intel/avs/path.c
> @@ -285,3 +285,133 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
>  
>  	return path;
>  }
> +
> +int avs_path_bind(struct avs_path *path)
> +{
> +	struct avs_path_pipeline *ppl;
> +	struct avs_dev *adev = path->owner;
> +	int ret;
> +
> +	list_for_each_entry(ppl, &path->ppl_list, node) {
> +		struct avs_path_binding *binding;
> +
> +		list_for_each_entry(binding, &ppl->binding_list, node) {
> +			struct avs_path_module *source, *sink;
> +
> +			source = binding->source;
> +			sink = binding->sink;
> +
> +			ret = avs_ipc_bind(adev, source->module_id,
> +					   source->instance_id, sink->module_id,
> +					   sink->instance_id, binding->sink_pin,
> +					   binding->source_pin);
> +			if (ret) {
> +				dev_err(adev->dev, "bind path failed: %d\n", ret);
> +				return AVS_IPC_RET(ret);

so what happens for all the previously bound path?

Is there an error-unwinding loop somewhere?

> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int avs_path_unbind(struct avs_path *path)
> +{
> +	struct avs_path_pipeline *ppl;
> +	struct avs_dev *adev = path->owner;
> +	int ret;
> +
> +	list_for_each_entry(ppl, &path->ppl_list, node) {
> +		struct avs_path_binding *binding;
> +
> +		list_for_each_entry(binding, &ppl->binding_list, node) {
> +			struct avs_path_module *source, *sink;
> +
> +			source = binding->source;
> +			sink = binding->sink;
> +
> +			ret = avs_ipc_unbind(adev, source->module_id,
> +					     source->instance_id, sink->module_id,
> +					     sink->instance_id, binding->sink_pin,
> +					     binding->source_pin);
> +			if (ret) {
> +				dev_err(adev->dev, "unbind path failed: %d\n", ret);
> +				return AVS_IPC_RET(ret);

what happens then? reboot?

> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int avs_path_reset(struct avs_path *path)
> +{
> +	struct avs_path_pipeline *ppl;
> +	struct avs_dev *adev = path->owner;
> +	int ret;
> +
> +	if (path->state == AVS_PPL_STATE_RESET)
> +		return 0;
> +
> +	list_for_each_entry(ppl, &path->ppl_list, node) {
> +		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
> +						 AVS_PPL_STATE_RESET);
> +		if (ret) {
> +			dev_err(adev->dev, "reset path failed: %d\n", ret);
> +			path->state = AVS_PPL_STATE_INVALID;
> +			return AVS_IPC_RET(ret);
> +		}
> +	}
> +
> +	path->state = AVS_PPL_STATE_RESET;
> +	return 0;
> +}
> +
> +int avs_path_pause(struct avs_path *path)
> +{
> +	struct avs_path_pipeline *ppl;
> +	struct avs_dev *adev = path->owner;
> +	int ret;
> +
> +	if (path->state == AVS_PPL_STATE_PAUSED)
> +		return 0;
> +
> +	list_for_each_entry_reverse(ppl, &path->ppl_list, node) {

does the order actually matter?

> +		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
> +						 AVS_PPL_STATE_PAUSED);
> +		if (ret) {
> +			dev_err(adev->dev, "pause path failed: %d\n", ret);
> +			path->state = AVS_PPL_STATE_INVALID;
> +			return AVS_IPC_RET(ret);
> +		}
> +	}
> +
> +	path->state = AVS_PPL_STATE_PAUSED;
> +	return 0;
> +}

it looks like you could use a helper since the flows are identical.

> +
> +int avs_path_run(struct avs_path *path, int trigger)
> +{
> +	struct avs_path_pipeline *ppl;
> +	struct avs_dev *adev = path->owner;
> +	int ret;
> +
> +	if (path->state == AVS_PPL_STATE_RUNNING && trigger == AVS_TPLG_TRIGGER_AUTO)
> +		return 0;
> +
> +	list_for_each_entry(ppl, &path->ppl_list, node) {
> +		if (ppl->template->cfg->trigger != trigger)
> +			continue;
> +
> +		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
> +						 AVS_PPL_STATE_RUNNING);
> +		if (ret) {
> +			dev_err(adev->dev, "run path failed: %d\n", ret);
> +			path->state = AVS_PPL_STATE_INVALID;
> +			return AVS_IPC_RET(ret);
> +		}
> +	}
> +
> +	path->state = AVS_PPL_STATE_RUNNING;
> +	return 0;
> +}
> diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
> index 3843e5a062a1..04a06473f04b 100644
> --- a/sound/soc/intel/avs/path.h
> +++ b/sound/soc/intel/avs/path.h
> @@ -62,5 +62,10 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
>  				 struct avs_tplg_path_template *template,
>  				 struct snd_pcm_hw_params *fe_params,
>  				 struct snd_pcm_hw_params *be_params);
> +int avs_path_bind(struct avs_path *path);
> +int avs_path_unbind(struct avs_path *path);
> +int avs_path_reset(struct avs_path *path);
> +int avs_path_pause(struct avs_path *path);
> +int avs_path_run(struct avs_path *path, int trigger);
>  
>  #endif

Re: [RFC 10/13] ASoC: Intel: avs: Path state management

[Cezary Rojewski]

On 2022-02-25 8:42 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> Add functions to ease with state changing of all objects found in the
>> path. Each represents either a BIND/UNBIND or SET_PIPEPILE_STATE IPC.
> 
> SET_PIPELINE?

A typo! Thanks for spotting this out!

>> DSP pipelines follow simple state machine scheme:
>> CREATE -> RESET -> PAUSE -> RUNNING -> PAUSE -> RESET -> DELETE>
>> There is no STOP, PAUSE serves that purpose instead.
> 
> that's not fully correct, the STOP can be handled in two steps due to
> DMA programming flows.

 From DSP perspective, there's no stop. Literally one cannot send 
SET_PIPELINE_STATE with STOP as a requested state.

>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/intel/avs/path.c | 130 +++++++++++++++++++++++++++++++++++++
>>   sound/soc/intel/avs/path.h |   5 ++
>>   2 files changed, 135 insertions(+)
>>
>> diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
>> index 272d868bedc9..c6db3f091e66 100644
>> --- a/sound/soc/intel/avs/path.c
>> +++ b/sound/soc/intel/avs/path.c
>> @@ -285,3 +285,133 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
>>   
>>   	return path;
>>   }
>> +
>> +int avs_path_bind(struct avs_path *path)
>> +{
>> +	struct avs_path_pipeline *ppl;
>> +	struct avs_dev *adev = path->owner;
>> +	int ret;
>> +
>> +	list_for_each_entry(ppl, &path->ppl_list, node) {
>> +		struct avs_path_binding *binding;
>> +
>> +		list_for_each_entry(binding, &ppl->binding_list, node) {
>> +			struct avs_path_module *source, *sink;
>> +
>> +			source = binding->source;
>> +			sink = binding->sink;
>> +
>> +			ret = avs_ipc_bind(adev, source->module_id,
>> +					   source->instance_id, sink->module_id,
>> +					   sink->instance_id, binding->sink_pin,
>> +					   binding->source_pin);
>> +			if (ret) {
>> +				dev_err(adev->dev, "bind path failed: %d\n", ret);
>> +				return AVS_IPC_RET(ret);
> 
> so what happens for all the previously bound path?
> 
> Is there an error-unwinding loop somewhere?

This is a good question. It's an unknown ground. Usually if anything 
wrong happens, all the pipelines that are part of the path would be 
forcefully deleted. What I can do, is add unwinding and check with 
validation using some unusual scenarios to see if no regression occurs.

Not promising anything though - see below.

>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int avs_path_unbind(struct avs_path *path)
>> +{
>> +	struct avs_path_pipeline *ppl;
>> +	struct avs_dev *adev = path->owner;
>> +	int ret;
>> +
>> +	list_for_each_entry(ppl, &path->ppl_list, node) {
>> +		struct avs_path_binding *binding;
>> +
>> +		list_for_each_entry(binding, &ppl->binding_list, node) {
>> +			struct avs_path_module *source, *sink;
>> +
>> +			source = binding->source;
>> +			sink = binding->sink;
>> +
>> +			ret = avs_ipc_unbind(adev, source->module_id,
>> +					     source->instance_id, sink->module_id,
>> +					     sink->instance_id, binding->sink_pin,
>> +					     binding->source_pin);
>> +			if (ret) {
>> +				dev_err(adev->dev, "unbind path failed: %d\n", ret);
>> +				return AVS_IPC_RET(ret);
> 
> what happens then? reboot?

Yeah, unfortunately when an IPC fails, it's usually for a very "bad" 
reason and only DSP recovery can help here.

>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int avs_path_reset(struct avs_path *path)
>> +{
>> +	struct avs_path_pipeline *ppl;
>> +	struct avs_dev *adev = path->owner;
>> +	int ret;
>> +
>> +	if (path->state == AVS_PPL_STATE_RESET)
>> +		return 0;
>> +
>> +	list_for_each_entry(ppl, &path->ppl_list, node) {
>> +		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
>> +						 AVS_PPL_STATE_RESET);
>> +		if (ret) {
>> +			dev_err(adev->dev, "reset path failed: %d\n", ret);
>> +			path->state = AVS_PPL_STATE_INVALID;
>> +			return AVS_IPC_RET(ret);
>> +		}
>> +	}
>> +
>> +	path->state = AVS_PPL_STATE_RESET;
>> +	return 0;
>> +}
>> +
>> +int avs_path_pause(struct avs_path *path)
>> +{
>> +	struct avs_path_pipeline *ppl;
>> +	struct avs_dev *adev = path->owner;
>> +	int ret;
>> +
>> +	if (path->state == AVS_PPL_STATE_PAUSED)
>> +		return 0;
>> +
>> +	list_for_each_entry_reverse(ppl, &path->ppl_list, node) {
> 
> does the order actually matter?

Yes, it does. We do here what's recommended.

>> +		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
>> +						 AVS_PPL_STATE_PAUSED);
>> +		if (ret) {
>> +			dev_err(adev->dev, "pause path failed: %d\n", ret);
>> +			path->state = AVS_PPL_STATE_INVALID;
>> +			return AVS_IPC_RET(ret);
>> +		}
>> +	}
>> +
>> +	path->state = AVS_PPL_STATE_PAUSED;
>> +	return 0;
>> +}
> 
> it looks like you could use a helper since the flows are identical.


I did try doing that in the past but the readability got hurt : (
avs_path_run() has an additional check when compared to the other two. 
avs_path_pause() and avs_path_reset() to the things in the opposite 
order. So, I still believe it's not good to provide a helper for these. 
If you are seeing something I'm not, please feel free to correct me.

>> +
>> +int avs_path_run(struct avs_path *path, int trigger)
>> +{
>> +	struct avs_path_pipeline *ppl;
>> +	struct avs_dev *adev = path->owner;
>> +	int ret;
>> +
>> +	if (path->state == AVS_PPL_STATE_RUNNING && trigger == AVS_TPLG_TRIGGER_AUTO)
>> +		return 0;
>> +
>> +	list_for_each_entry(ppl, &path->ppl_list, node) {
>> +		if (ppl->template->cfg->trigger != trigger)
>> +			continue;
>> +
>> +		ret = avs_ipc_set_pipeline_state(adev, ppl->instance_id,
>> +						 AVS_PPL_STATE_RUNNING);
>> +		if (ret) {
>> +			dev_err(adev->dev, "run path failed: %d\n", ret);
>> +			path->state = AVS_PPL_STATE_INVALID;
>> +			return AVS_IPC_RET(ret);
>> +		}
>> +	}
>> +
>> +	path->state = AVS_PPL_STATE_RUNNING;
>> +	return 0;
>> +}
>> diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
>> index 3843e5a062a1..04a06473f04b 100644
>> --- a/sound/soc/intel/avs/path.h
>> +++ b/sound/soc/intel/avs/path.h
>> @@ -62,5 +62,10 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
>>   				 struct avs_tplg_path_template *template,
>>   				 struct snd_pcm_hw_params *fe_params,
>>   				 struct snd_pcm_hw_params *be_params);
>> +int avs_path_bind(struct avs_path *path);
>> +int avs_path_unbind(struct avs_path *path);
>> +int avs_path_reset(struct avs_path *path);
>> +int avs_path_pause(struct avs_path *path);
>> +int avs_path_run(struct avs_path *path, int trigger);
>>   
>>   #endif

[RFC 11/13] ASoC: Intel: avs: Arm paths after creating them

[Cezary Rojewski]

Creating the pipelines and instantiating the modules alone is
insufficient to have a fully operational stream. Before it can be run,
stream components need to be bound. Add arming functions to ensure all
necessary operations are completed before path is yielded back to the
avs_path_create() caller.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/path.c | 180 +++++++++++++++++++++++++++++++++++++
 1 file changed, 180 insertions(+)

diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
index c6db3f091e66..abeb6921fcce 100644
--- a/sound/soc/intel/avs/path.c
+++ b/sound/soc/intel/avs/path.c
@@ -13,6 +13,73 @@
 #include "path.h"
 #include "topology.h"
 
+/* Must be called with adev->comp_list_mutex held. */
+static struct avs_tplg *
+avs_path_find_tplg(struct avs_dev *adev, const char *name)
+{
+	struct avs_soc_component *acomp;
+
+	list_for_each_entry(acomp, &adev->comp_list, node)
+		if (!strcmp(acomp->tplg->name, name))
+			return acomp->tplg;
+	return NULL;
+}
+
+static struct avs_path_module *
+avs_path_find_module(struct avs_path_pipeline *ppl, u32 template_id)
+{
+	struct avs_path_module *mod;
+
+	list_for_each_entry(mod, &ppl->mod_list, node)
+		if (mod->template->id == template_id)
+			return mod;
+	return NULL;
+}
+
+static struct avs_path_pipeline *
+avs_path_find_pipeline(struct avs_path *path, u32 template_id)
+{
+	struct avs_path_pipeline *ppl;
+
+	list_for_each_entry(ppl, &path->ppl_list, node)
+		if (ppl->template->id == template_id)
+			return ppl;
+	return NULL;
+}
+
+static struct avs_path *
+avs_path_find_path(struct avs_dev *adev, const char *name, u32 template_id)
+{
+	struct avs_tplg_path_template *pos, *template = NULL;
+	struct avs_tplg *tplg;
+	struct avs_path *path;
+
+	tplg = avs_path_find_tplg(adev, name);
+	if (!tplg)
+		return NULL;
+
+	list_for_each_entry(pos, &tplg->path_tmpl_list, node) {
+		if (pos->id == template_id) {
+			template = pos;
+			break;
+		}
+	}
+	if (!template)
+		return NULL;
+
+	spin_lock(&adev->path_list_lock);
+	/* Only one variant of given path template may be instantiated at a time. */
+	list_for_each_entry(path, &adev->path_list, node) {
+		if (path->template->owner == template) {
+			spin_unlock(&adev->path_list_lock);
+			return path;
+		}
+	}
+
+	spin_unlock(&adev->path_list_lock);
+	return NULL;
+}
+
 static bool avs_test_hw_params(struct snd_pcm_hw_params *params,
 			       struct avs_audio_format *fmt)
 {
@@ -75,6 +142,58 @@ avs_path_module_create(struct avs_dev *adev,
 	return mod;
 }
 
+static int avs_path_binding_arm(struct avs_dev *adev, struct avs_path_binding *binding)
+{
+	struct avs_path_module *this_mod, *target_mod;
+	struct avs_path_pipeline *target_ppl;
+	struct avs_path *target_path;
+	struct avs_tplg_binding *t;
+
+	t = binding->template;
+	this_mod = avs_path_find_module(binding->owner,
+					t->mod_id);
+	if (!this_mod) {
+		dev_err(adev->dev, "path mod %d not found\n", t->mod_id);
+		return -EINVAL;
+	}
+
+	/* update with target_tplg_name too */
+	target_path = avs_path_find_path(adev, t->target_tplg_name,
+					 t->target_path_tmpl_id);
+	if (!target_path) {
+		dev_err(adev->dev, "target path %s:%d not found\n",
+			t->target_tplg_name, t->target_path_tmpl_id);
+		return -EINVAL;
+	}
+
+	target_ppl = avs_path_find_pipeline(target_path,
+					    t->target_ppl_id);
+	if (!target_ppl) {
+		dev_err(adev->dev, "target ppl %d not found\n", t->target_ppl_id);
+		return -EINVAL;
+	}
+
+	target_mod = avs_path_find_module(target_ppl, t->target_mod_id);
+	if (!target_mod) {
+		dev_err(adev->dev, "target mod %d not found\n", t->target_mod_id);
+		return -EINVAL;
+	}
+
+	if (t->is_sink) {
+		binding->sink = this_mod;
+		binding->sink_pin = t->mod_pin;
+		binding->source = target_mod;
+		binding->source_pin = t->target_mod_pin;
+	} else {
+		binding->sink = target_mod;
+		binding->sink_pin = t->target_mod_pin;
+		binding->source = this_mod;
+		binding->source_pin = t->mod_pin;
+	}
+
+	return 0;
+}
+
 static void avs_path_binding_free(struct avs_dev *adev, struct avs_path_binding *binding)
 {
 	kfree(binding);
@@ -97,6 +216,38 @@ static struct avs_path_binding *avs_path_binding_create(struct avs_dev *adev,
 	return binding;
 }
 
+static int avs_path_pipeline_arm(struct avs_dev *adev,
+				 struct avs_path_pipeline *ppl)
+{
+	struct avs_path_module *mod;
+
+	list_for_each_entry(mod, &ppl->mod_list, node) {
+		struct avs_path_module *source, *sink;
+		int ret;
+
+		/*
+		 * Only one module (so it's implicitly last) or it is the last
+		 * one, either way we don't have next module to bind it to.
+		 */
+		if (mod == list_last_entry(&ppl->mod_list,
+					   struct avs_path_module, node))
+			break;
+
+		/* bind current module to next module on list */
+		source = mod;
+		sink = list_next_entry(mod, node);
+		if (!source || !sink)
+			return -EINVAL;
+
+		ret = avs_ipc_bind(adev, source->module_id, source->instance_id,
+				   sink->module_id, sink->instance_id, 0, 0);
+		if (ret)
+			return AVS_IPC_RET(ret);
+	}
+
+	return 0;
+}
+
 static void avs_path_pipeline_free(struct avs_dev *adev,
 				   struct avs_path_pipeline *ppl)
 {
@@ -212,6 +363,31 @@ static int avs_path_init(struct avs_dev *adev, struct avs_path *path,
 	return 0;
 }
 
+static int avs_path_arm(struct avs_dev *adev, struct avs_path *path)
+{
+	struct avs_path_pipeline *ppl;
+	struct avs_path_binding *binding;
+	int ret;
+
+	list_for_each_entry(ppl, &path->ppl_list, node) {
+		/*
+		 * Arm all ppl bindings before binding internal modules
+		 * as it costs no IPCs which isn't true for the latter.
+		 */
+		list_for_each_entry(binding, &ppl->binding_list, node) {
+			ret = avs_path_binding_arm(adev, binding);
+			if (ret < 0)
+				return ret;
+		}
+
+		ret = avs_path_pipeline_arm(adev, ppl);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static void avs_path_free_unlocked(struct avs_path *path)
 {
 	struct avs_path_pipeline *ppl, *save;
@@ -243,6 +419,10 @@ static struct avs_path *avs_path_create_unlocked(struct avs_dev *adev, u32 dma_i
 	if (ret < 0)
 		goto err;
 
+	ret = avs_path_arm(adev, path);
+	if (ret < 0)
+		goto err;
+
 	path->state = AVS_PPL_STATE_INVALID;
 	return path;
 err:
-- 
2.25.1

[RFC 12/13] ASoC: Intel: avs: Prepare modules before bindings them

[Cezary Rojewski]

When binding modules to pins other than pin0, sometimes additional
preparations need to be made, depending on the module type.
Add function that prepares modules when necessary before binding them.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/path.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
index abeb6921fcce..b5c0f89add4f 100644
--- a/sound/soc/intel/avs/path.c
+++ b/sound/soc/intel/avs/path.c
@@ -466,6 +466,37 @@ struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id,
 	return path;
 }
 
+static int avs_path_bind_prepare(struct avs_dev *adev,
+				 struct avs_path_binding *binding)
+{
+	const struct avs_audio_format *src_fmt, *sink_fmt;
+	struct avs_tplg_module *tsource = binding->source->template;
+	struct avs_path_module *source = binding->source;
+	int ret;
+
+	/*
+	 * only copier modules about to be bound
+	 * to output pin other than 0 need preparation
+	 */
+	if (!binding->source_pin)
+		return 0;
+	if (!guid_equal(&tsource->cfg_ext->type, &AVS_COPIER_MOD_UUID))
+		return 0;
+
+	src_fmt = tsource->in_fmt;
+	sink_fmt = binding->sink->template->in_fmt;
+
+	ret = avs_ipc_copier_set_sink_format(adev, source->module_id,
+					     source->instance_id, binding->source_pin,
+					     src_fmt, sink_fmt);
+	if (ret) {
+		dev_err(adev->dev, "config copier failed: %d\n", ret);
+		return AVS_IPC_RET(ret);
+	}
+
+	return 0;
+}
+
 int avs_path_bind(struct avs_path *path)
 {
 	struct avs_path_pipeline *ppl;
@@ -481,6 +512,10 @@ int avs_path_bind(struct avs_path *path)
 			source = binding->source;
 			sink = binding->sink;
 
+			ret = avs_path_bind_prepare(adev, binding);
+			if (ret < 0)
+				return ret;
+
 			ret = avs_ipc_bind(adev, source->module_id,
 					   source->instance_id, sink->module_id,
 					   sink->instance_id, binding->sink_pin,
-- 
2.25.1

[RFC 13/13] ASoC: Intel: avs: Configure modules according to their type

[Cezary Rojewski]

Each module on DSP side serves a processing purpose. Depending on its
purpose, it needs different information during its initialization. Add
functions responsible for creating instances of specific module types
given the information coming from the topology file.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/Kconfig    |   1 +
 sound/soc/intel/avs/avs.h  |   1 +
 sound/soc/intel/avs/path.c | 378 ++++++++++++++++++++++++++++++++++++-
 sound/soc/intel/avs/path.h |   1 +
 4 files changed, 380 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index f0688c3d76a7..b01c492d3514 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -219,6 +219,7 @@ config SND_SOC_INTEL_AVS
 	select SND_SOC_TOPOLOGY
 	select SND_HDA_EXT_CORE
 	select SND_HDA_DSP_LOADER
+	select SND_INTEL_NHLT
 	help
 	  Enable support for Intel(R) cAVS 1.5 platforms with DSP
 	  capabilities. This includes Skylake, Kabylake, Amberlake and
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index 313001b0455f..4da8e16280fc 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -92,6 +92,7 @@ struct avs_dev {
 
 	struct completion fw_ready;
 
+	struct nhlt_acpi_table *nhlt;
 	struct list_head comp_list;
 	struct mutex comp_list_mutex;
 	struct list_head path_list;
diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c
index b5c0f89add4f..8c5d2672a081 100644
--- a/sound/soc/intel/avs/path.c
+++ b/sound/soc/intel/avs/path.c
@@ -113,6 +113,375 @@ avs_path_find_variant(struct avs_dev *adev,
 	return NULL;
 }
 
+__maybe_unused
+static bool avs_dma_type_is_host(u32 dma_type)
+{
+	return dma_type == AVS_DMA_HDA_HOST_OUTPUT ||
+	       dma_type == AVS_DMA_HDA_HOST_INPUT;
+}
+
+__maybe_unused
+static bool avs_dma_type_is_link(u32 dma_type)
+{
+	return !avs_dma_type_is_host(dma_type);
+}
+
+__maybe_unused
+static bool avs_dma_type_is_output(u32 dma_type)
+{
+	return dma_type == AVS_DMA_HDA_HOST_OUTPUT ||
+	       dma_type == AVS_DMA_HDA_LINK_OUTPUT ||
+	       dma_type == AVS_DMA_I2S_LINK_OUTPUT;
+}
+
+__maybe_unused
+static bool avs_dma_type_is_input(u32 dma_type)
+{
+	return !avs_dma_type_is_output(dma_type);
+}
+
+static int avs_copier_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct nhlt_acpi_table *nhlt = adev->nhlt;
+	struct avs_tplg_module *t = mod->template;
+	struct avs_copier_cfg *cfg;
+	struct nhlt_specific_cfg *ep_blob;
+	union avs_connector_node_id node_id = {0};
+	size_t cfg_size, data_size = 0;
+	void *data = NULL;
+	u32 dma_type;
+	int ret;
+
+	dma_type = t->cfg_ext->copier.dma_type;
+	node_id.dma_type = dma_type;
+
+	switch (dma_type) {
+		struct avs_audio_format *fmt;
+		int direction;
+
+	case AVS_DMA_I2S_LINK_OUTPUT:
+	case AVS_DMA_I2S_LINK_INPUT:
+		if (avs_dma_type_is_input(dma_type))
+			direction = SNDRV_PCM_STREAM_CAPTURE;
+		else
+			direction = SNDRV_PCM_STREAM_PLAYBACK;
+
+		if (t->cfg_ext->copier.blob_fmt)
+			fmt = t->cfg_ext->copier.blob_fmt;
+		else if (direction == SNDRV_PCM_STREAM_CAPTURE)
+			fmt = t->in_fmt;
+		else
+			fmt = t->cfg_ext->copier.out_fmt;
+
+		ep_blob = intel_nhlt_get_endpoint_blob(adev->dev,
+			nhlt, t->cfg_ext->copier.vindex.i2s.instance,
+			NHLT_LINK_SSP, fmt->valid_bit_depth, fmt->bit_depth,
+			fmt->num_channels, fmt->sampling_freq, direction,
+			NHLT_DEVICE_I2S);
+		if (!ep_blob) {
+			dev_err(adev->dev, "no I2S ep_blob found\n");
+			return -ENOENT;
+		}
+
+		data = ep_blob->caps;
+		data_size = ep_blob->size;
+		/* I2S gateway's vindex is statically assigned in topology */
+		node_id.vindex = t->cfg_ext->copier.vindex.val;
+
+		break;
+
+	case AVS_DMA_DMIC_LINK_INPUT:
+		direction = SNDRV_PCM_STREAM_CAPTURE;
+
+		if (t->cfg_ext->copier.blob_fmt)
+			fmt = t->cfg_ext->copier.blob_fmt;
+		else
+			fmt = t->in_fmt;
+
+		ep_blob = intel_nhlt_get_endpoint_blob(adev->dev, nhlt, 0,
+				NHLT_LINK_DMIC, fmt->valid_bit_depth,
+				fmt->bit_depth, fmt->num_channels,
+				fmt->sampling_freq, direction, NHLT_DEVICE_DMIC);
+		if (!ep_blob) {
+			dev_err(adev->dev, "no DMIC ep_blob found\n");
+			return -ENOENT;
+		}
+
+		data = ep_blob->caps;
+		data_size = ep_blob->size;
+		/* DMIC gateway's vindex is statically assigned in topology */
+		node_id.vindex = t->cfg_ext->copier.vindex.val;
+
+		break;
+
+	case AVS_DMA_HDA_HOST_OUTPUT:
+	case AVS_DMA_HDA_HOST_INPUT:
+		/* HOST gateway's vindex is dynamically assigned with DMA id */
+		node_id.vindex = mod->owner->owner->dma_id;
+		break;
+
+	case AVS_DMA_HDA_LINK_OUTPUT:
+	case AVS_DMA_HDA_LINK_INPUT:
+		node_id.vindex = t->cfg_ext->copier.vindex.val |
+				 mod->owner->owner->dma_id;
+		break;
+
+	case INVALID_OBJECT_ID:
+	default:
+		node_id = INVALID_NODE_ID;
+		break;
+	}
+
+	cfg_size = sizeof(*cfg) + data_size;
+	/* Every config-BLOB contains gateway attributes. */
+	if (data_size)
+		cfg_size -= sizeof(cfg->gtw_cfg.config.attrs);
+
+	cfg = kzalloc(cfg_size, GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	cfg->base.cpc = t->cfg_base->cpc;
+	cfg->base.ibs = t->cfg_base->ibs;
+	cfg->base.obs = t->cfg_base->obs;
+	cfg->base.is_pages = t->cfg_base->is_pages;
+	cfg->base.audio_fmt = *t->in_fmt;
+	cfg->out_fmt = *t->cfg_ext->copier.out_fmt;
+	cfg->feature_mask = t->cfg_ext->copier.feature_mask;
+	cfg->gtw_cfg.node_id = node_id;
+	cfg->gtw_cfg.dma_buffer_size = t->cfg_ext->copier.dma_buffer_size;
+	/* config_length in DWORDs */
+	cfg->gtw_cfg.config_length = DIV_ROUND_UP(data_size, 4);
+	if (data)
+		memcpy(&cfg->gtw_cfg.config, data, data_size);
+
+	mod->gtw_attrs = cfg->gtw_cfg.config.attrs;
+
+	ret = avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				  t->core_id, t->domain, cfg, cfg_size,
+				  &mod->instance_id);
+	kfree(cfg);
+	return ret;
+}
+
+static int avs_updown_mix_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_updown_mixer_cfg cfg;
+	int i;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.out_channel_config = t->cfg_ext->updown_mix.out_channel_config;
+	cfg.coefficients_select = t->cfg_ext->updown_mix.coefficients_select;
+	for (i = 0; i < AVS_CHANNELS_MAX; i++)
+		cfg.coefficients[i] = t->cfg_ext->updown_mix.coefficients[i];
+	cfg.channel_map = t->cfg_ext->updown_mix.channel_map;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_src_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_src_cfg cfg;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.out_freq = t->cfg_ext->src.out_freq;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_asrc_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_asrc_cfg cfg;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.out_freq = t->cfg_ext->asrc.out_freq;
+	cfg.mode = t->cfg_ext->asrc.mode;
+	cfg.disable_jitter_buffer = t->cfg_ext->asrc.disable_jitter_buffer;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_aec_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_aec_cfg cfg;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.ref_fmt = *t->cfg_ext->aec.ref_fmt;
+	cfg.out_fmt = *t->cfg_ext->aec.out_fmt;
+	cfg.cpc_lp_mode = t->cfg_ext->aec.cpc_lp_mode;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_mux_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_mux_cfg cfg;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.ref_fmt = *t->cfg_ext->mux.ref_fmt;
+	cfg.out_fmt = *t->cfg_ext->mux.out_fmt;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_wov_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_wov_cfg cfg;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.cpc_lp_mode = t->cfg_ext->wov.cpc_lp_mode;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_micsel_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_micsel_cfg cfg;
+
+	cfg.base.cpc = t->cfg_base->cpc;
+	cfg.base.ibs = t->cfg_base->ibs;
+	cfg.base.obs = t->cfg_base->obs;
+	cfg.base.is_pages = t->cfg_base->is_pages;
+	cfg.base.audio_fmt = *t->in_fmt;
+	cfg.out_fmt = *t->cfg_ext->micsel.out_fmt;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_modbase_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_modcfg_base cfg;
+
+	cfg.cpc = t->cfg_base->cpc;
+	cfg.ibs = t->cfg_base->ibs;
+	cfg.obs = t->cfg_base->obs;
+	cfg.is_pages = t->cfg_base->is_pages;
+	cfg.audio_fmt = *t->in_fmt;
+
+	return avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				   t->core_id, t->domain, &cfg, sizeof(cfg),
+				   &mod->instance_id);
+}
+
+static int avs_modext_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	struct avs_tplg_module *t = mod->template;
+	struct avs_tplg_modcfg_ext *tcfg = t->cfg_ext;
+	struct avs_modcfg_ext *cfg;
+	size_t cfg_size, num_pins;
+	int ret, i;
+
+	num_pins = tcfg->generic.num_input_pins + tcfg->generic.num_output_pins;
+	cfg_size = sizeof(*cfg) + sizeof(*cfg->pin_fmts) * num_pins;
+
+	cfg = kzalloc(cfg_size, GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	cfg->base.cpc = t->cfg_base->cpc;
+	cfg->base.ibs = t->cfg_base->ibs;
+	cfg->base.obs = t->cfg_base->obs;
+	cfg->base.is_pages = t->cfg_base->is_pages;
+	cfg->base.audio_fmt = *t->in_fmt;
+	cfg->num_input_pins = tcfg->generic.num_input_pins;
+	cfg->num_output_pins = tcfg->generic.num_output_pins;
+
+	/* configure pin formats */
+	for (i = 0; i < num_pins; i++) {
+		struct avs_tplg_pin_format *tpin = &tcfg->generic.pin_fmts[i];
+		struct avs_pin_format *pin = &cfg->pin_fmts[i];
+
+		pin->pin_index = tpin->pin_index;
+		pin->iobs = tpin->iobs;
+		pin->audio_fmt = *tpin->fmt;
+	}
+
+	ret = avs_dsp_init_module(adev, mod->module_id, mod->owner->instance_id,
+				  t->core_id, t->domain, cfg, cfg_size,
+				  &mod->instance_id);
+	kfree(cfg);
+	return ret;
+}
+
+static int avs_path_module_type_create(struct avs_dev *adev, struct avs_path_module *mod)
+{
+	const guid_t *type = &mod->template->cfg_ext->type;
+
+	if (guid_equal(type, &AVS_MIXIN_MOD_UUID) ||
+	    guid_equal(type, &AVS_MIXOUT_MOD_UUID) ||
+	    guid_equal(type, &AVS_KPBUFF_MOD_UUID))
+		return avs_modbase_create(adev, mod);
+	if (guid_equal(type, &AVS_COPIER_MOD_UUID))
+		return avs_copier_create(adev, mod);
+	if (guid_equal(type, &AVS_MICSEL_MOD_UUID))
+		return avs_micsel_create(adev, mod);
+	if (guid_equal(type, &AVS_MUX_MOD_UUID))
+		return avs_mux_create(adev, mod);
+	if (guid_equal(type, &AVS_UPDWMIX_MOD_UUID))
+		return avs_updown_mix_create(adev, mod);
+	if (guid_equal(type, &AVS_SRCINTC_MOD_UUID))
+		return avs_src_create(adev, mod);
+	if (guid_equal(type, &AVS_AEC_MOD_UUID))
+		return avs_aec_create(adev, mod);
+	if (guid_equal(type, &AVS_ASRC_MOD_UUID))
+		return avs_asrc_create(adev, mod);
+	if (guid_equal(type, &AVS_INTELWOV_MOD_UUID))
+		return avs_wov_create(adev, mod);
+
+	if (guid_equal(type, &AVS_PROBE_MOD_UUID)) {
+		dev_err(adev->dev, "Probe module can't be instantiated by topology");
+		return -EINVAL;
+	}
+
+	return avs_modext_create(adev, mod);
+}
+
 static void avs_path_module_free(struct avs_dev *adev, struct avs_path_module *mod)
 {
 	kfree(mod);
@@ -124,7 +493,7 @@ avs_path_module_create(struct avs_dev *adev,
 		       struct avs_tplg_module *template)
 {
 	struct avs_path_module *mod;
-	int module_id;
+	int module_id, ret;
 
 	module_id = avs_get_module_id(adev, &template->cfg_ext->type);
 	if (module_id < 0)
@@ -139,6 +508,13 @@ avs_path_module_create(struct avs_dev *adev,
 	mod->owner = owner;
 	INIT_LIST_HEAD(&mod->node);
 
+	ret = avs_path_module_type_create(adev, mod);
+	if (ret) {
+		dev_err(adev->dev, "module-type create failed: %d\n", ret);
+		kfree(mod);
+		return ERR_PTR(ret);
+	}
+
 	return mod;
 }
 
diff --git a/sound/soc/intel/avs/path.h b/sound/soc/intel/avs/path.h
index 04a06473f04b..197222c5e008 100644
--- a/sound/soc/intel/avs/path.h
+++ b/sound/soc/intel/avs/path.h
@@ -38,6 +38,7 @@ struct avs_path_pipeline {
 struct avs_path_module {
 	u16 module_id;
 	u16 instance_id;
+	union avs_gtw_attributes gtw_attrs;
 
 	struct avs_tplg_module *template;
 	struct avs_path_pipeline *owner;
-- 
2.25.1

Re: [RFC 00/13] ASoC: Intel: avs: Topology and path management

[Pierre-Louis Bossart]

> A continuation of avs-driver initial series [1]. This chapter covers
> path management and topology parsing part which was ealier path of the
> main series. The two patches that represented these two subjects in the
> initial series, have been split into many to allow for easier review and
> discussion.
> 
> AVS topology is split into two major parts: dictionaries - found within
> ASoC topology manifest - and path templates - found within DAPM widget
> private data. Dictionaries job is to reduce the total amount of memory
> occupied by topology elements. Rather than having every pipeline and
> module carry its own information, each refers to specific entry in
> specific dictionary by provided (from topology file) indexes. In
> consequence, most struct avs_tplg_xxx are made out of pointers.
> 
> A 'path' represents a DSP side of audio stream in runtime - is a logical
> container for pipelines which are themselves composed of modules -
> processing units. Due to high range of possible audio format
> combinations, there can be more variants of given path (and thus, its
> pipelines and modules) than total number of pipelines and module
> instances which firmware supports concurrently, all the instance IDs are
> allocated dynamically with help of IDA interface. 'Path templates' come
> from topology file and describe a pattern which is later used to
> actually create runtime 'path'.

This is one of the most 'dense' patchsets I've reviewed in a long time.
While the code looks mostly good, I am afraid the directions and scope
are rather unclear. Now that you've split the basic parts out,
ironically it makes the intent of this patchset really difficult to grasp.

The first order of business is really to clarify the concepts:

What is a 'stream'? what is a 'path'? what is a 'path template'? What is
a 'module template'? What topologies are supported? what is the link
between a path and FE? how does this interoperate or duplicate DPAM? why
does a path rely on a single DMA? What would happen if there is no host
PCM, e.g. for a beep tone generated in firmware? How would this work for
a firmware capture pipeline that only sends notification on acoustic
events and no PCM data?

I have reviewed this set of patches three times already and this set of
concepts are still nebulous to me, please refer to my detailed comments.

You really need to describe in layman's terms what the problem statement
is, what your solution tries to fix, what other options you considered,
what cases you didn't handle, etc. Put yourself if the shoes of someone
that is not part of your team and has no prior exposure to the cAVS
firmware design.

I would recommend that you use the Windows documentation [1] to provide
ascii-art examples with hierarchical mixing, multiple outputs and
loopbacks on what a 'path' really is and how the concept of template helps.

But at a more fundamental level, the main concern I have is with the BE
use: this patchset assumes the BE configuration is fixed, and that's a
very very strong limitation. It's clearly not right for e.g. Bluetooth
offload where multiple profiles need to be supported. It's also not
right when the codec or receiver can adjust to multiple hw_params, which
could lead to simplifications such as removal of unnecessary
SRC/downmixers, etc.

We absolutely want the capability to reconfigure the BE by using
constraint matching between FE hw_params requested by applications and
what the hardware can support. If your solution solved that problem you
would have my full support. But if we're adding a rather complicated
framework on top of a known limitation, then that's really a missed
opportunity to do things better collectively.

[1]
https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/audio-processing-object-architecture

Re: [RFC 00/13] ASoC: Intel: avs: Topology and path management

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Fri, Feb 25, 2022 at 03:35:12PM -0600, Pierre-Louis Bossart wrote:

> But at a more fundamental level, the main concern I have is with the BE
> use: this patchset assumes the BE configuration is fixed, and that's a
> very very strong limitation. It's clearly not right for e.g. Bluetooth
> offload where multiple profiles need to be supported. It's also not
> right when the codec or receiver can adjust to multiple hw_params, which
> could lead to simplifications such as removal of unnecessary
> SRC/downmixers, etc.

> We absolutely want the capability to reconfigure the BE by using
> constraint matching between FE hw_params requested by applications and
> what the hardware can support. If your solution solved that problem you
> would have my full support. But if we're adding a rather complicated
> framework on top of a known limitation, then that's really a missed
> opportunity to do things better collectively.

On the one hand everything you say here is true.  On the other hand I
*did* suggest starting off with something with stripped down
functionality and then building up from that to make things more
digestable so some of this could be the result of that approach (I've
not got enough recollection of previous serieses to confirm), obviously
fixing the output is also quite a common thing for DSP based systems to
do just in general.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]