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