[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 {