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;
+};