[RFC,04/13] ASoC: Intel: avs: Parse pplcfg and binding tuples

Stream on ADSP firmware is represented by one or more pipelines. Just
like modules, these are described by a config structure. Add parsing
helpers to support loading such information from the topology file.

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/topology.c | 114 +++++++++++++++++++++++++++++++++
 sound/soc/intel/avs/topology.h |  29 +++++++++
 2 files changed, 143 insertions(+)

On 2/7/22 07:25, Cezary Rojewski wrote:
> Stream on ADSP firmware is represented by one or more pipelines. Just

I have a lot of questions on this one line...

what is a 'stream'?

'stream' historically means 'direction' in ALSA.

Then we have sdw_stream, which describes how source and sink ports are
connected on a platform.

We also have DPCM front-ends, visible mostly through the PCM device they
expose to users.

In windows we have stream effects that are really meant to be on a
single dedicated pipeline.

other questions: can a stream represent data moving in different
directions, e.g. in full-duplex mode.

How would a loopback be described?

What happens when a data path is split (demux) or merged (mixer)?

How is a 'stream' different from a 'path template' that you referred to
in Patch RFC 02/13

You would need at least 10 lines of plain English to make sure no one
will misunderstand what 'stream' means.

> like modules, these are described by a config structure. Add parsing
> helpers to support loading such information from the topology file.
>
> +/* Specifies path behaviour during PCM ->trigger(START) commnand. */

typo: command.

> +enum avs_tplg_trigger {
> +	AVS_TPLG_TRIGGER_AUTO = 0,
> +	AVS_TPLG_TRIGGER_USERSPACE = 1, /* via sysfs */

The feedback in previous versions was that we should not expose any
sysfs interface that would interfere with decisions made by the driver.

This is very controversial and a major hurdle to upstream any of this.

Debugfs is fine, as suggested by Mark as well

"
If it's mainly used for debugging then it could be exposed through
debugfs with less worry.
"


> +};
> +
> +struct avs_tplg_pplcfg {
> +	u16 req_size;

what does this describe?

> +	u8 priority;
> +	bool lp;
> +	u16 attributes;
> +	enum avs_tplg_trigger trigger;
> +};
> +
> +struct avs_tplg_binding {
> +	char target_tplg_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	u32 target_path_tmpl_id;
> +	u32 target_ppl_id;
> +	u32 target_mod_id;
> +	u8 target_mod_pin;

you really need to elaborate on what a template is, and how you use a
template and a ppl id concurrently.

> +	u32 mod_id;
> +	u8 mod_pin;
> +	u8 is_sink;
> +};
> +
>  #endif

On 2022-02-25 6:40 PM, Pierre-Louis Bossart wrote:
> On 2/7/22 07:25, Cezary Rojewski wrote:
>> Stream on ADSP firmware is represented by one or more pipelines. Just
> 
> I have a lot of questions on this one line...
> 
> what is a 'stream'?
> 
> 'stream' historically means 'direction' in ALSA.


That ambiguity should be fixed long ago, it's probably the most 
frequently asked question/error done by ALSA newcomers.

Many drivers use 'stream' without implying direction e.g.: hda. It's 
also part of framework language anyway e.g.: substream (is that supposed 
to imply: subdirection?)

> Then we have sdw_stream, which describes how source and sink ports are
> connected on a platform.
> 
> We also have DPCM front-ends, visible mostly through the PCM device they
> expose to users.
> 
> In windows we have stream effects that are really meant to be on a
> single dedicated pipeline.
> 
> other questions: can a stream represent data moving in different
> directions, e.g. in full-duplex mode.
> 
> How would a loopback be described?
> 
> What happens when a data path is split (demux) or merged (mixer)?
> 
> How is a 'stream' different from a 'path template' that you referred to
> in Patch RFC 02/13
> 
> You would need at least 10 lines of plain English to make sure no one
> will misunderstand what 'stream' means.


If that's the case, then we should re-think how and when 'stream' is 
used within the kernel.

Now, everyone from Windows team is perfectly fine with using 'stream' as 
is, it's common there.
There are many types of effects, and the 'effect' subject has an 
ambiguity of its own but let's not have that subject here on the ALSA 
mailing list :)

I believe you would like a reword, as it seems my usage of 'stream' 
brought a ton of questions. 'Path' perhaps?

>> like modules, these are described by a config structure. Add parsing
>> helpers to support loading such information from the topology file.
>>
>> +/* Specifies path behaviour during PCM ->trigger(START) commnand. */
> 
> typo: command.


Ack, thanks!

>> +enum avs_tplg_trigger {
>> +	AVS_TPLG_TRIGGER_AUTO = 0,
>> +	AVS_TPLG_TRIGGER_USERSPACE = 1, /* via sysfs */
> 
> The feedback in previous versions was that we should not expose any
> sysfs interface that would interfere with decisions made by the driver.
> 
> This is very controversial and a major hurdle to upstream any of this.
> 
> Debugfs is fine, as suggested by Mark as well
> 
> "
> If it's mainly used for debugging then it could be exposed through
> debugfs with less worry.
> "


I'll remove 'USERSPACE' entry for now. I'll come back to that later on, 
in a different series.

>> +};
>> +
>> +struct avs_tplg_pplcfg {
>> +	u16 req_size;
> 
> what does this describe?


Pipeline configuration i.e. all the information required when sending 
CREATE_PIPELINE IPC.

>> +	u8 priority;
>> +	bool lp;
>> +	u16 attributes;
>> +	enum avs_tplg_trigger trigger;
>> +};
>> +
>> +struct avs_tplg_binding {
>> +	char target_tplg_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
>> +	u32 target_path_tmpl_id;
>> +	u32 target_ppl_id;
>> +	u32 target_mod_id;
>> +	u8 target_mod_pin;
> 
> you really need to elaborate on what a template is, and how you use a
> template and a ppl id concurrently.


As stated, such thing exists already, e.g.: in the skylake-driver. 
"Binding" here is called "Link" there. It's a different representation 
of the same thing.

Here we have all the information to without a question identify modules 
to bind during runtime.

>> +	u32 mod_id;
>> +	u8 mod_pin;
>> +	u8 is_sink;
>> +};
>> +
>>   #endif