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