| Message ID | 20220207132532.3782412-1-cezary.rojewski@intel.com |
|---|---|
| Headers | show |
| Series | ASoC: Intel: avs: Topology and path management | expand |
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; > +};
> +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?
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 {
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; > +}
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.
> 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
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.
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; >> +};
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
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 {
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; >> +} >
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
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