[REVIEW ONLY] topology 2.0 #3183

lyakh · 2020-07-14T14:57:41Z

This is a very early proposal of what Topology v2.0 files could eventually look like. For review and commenting only for now.

Topology 2.0 file format is an extension of the ALSA configuration language. It uses the same basic syntax of "key-value pairs" of the language but adds features, required to be able to define complex objects by re-using definitions of component objects with different attributes. This replaces the SOF m4 topology macro library. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2020-07-15T09:48:36Z

Forgot to add yesterday: a list of new ALSA configuration keywords (might be missing some)

array
availability { mandatory, optional, automatic }
default
enum
graph
max
min
pipelines
range
struct
subtree
template
type
# in general "DefineX" declares a block for "SectionX" except for "abstract"
# definitions like "Attribute," "Template," "Type"
DefineAttribute
DefineBE
DefineControlMixer
DefineHWConfig
DefinePCM
DefinePCMCapabilities
DefineTemplate
DefineType
DefineWidget

lgirdwood

Can you make the changes and share a pipeline like

Host0 --> buffer0 --> volume0 --> buffer1 --> SSP0

We will need to have a global attributes and per widget/component/section attributes that should be prefixed to avoid collisions.

lgirdwood · 2020-07-16T09:15:15Z

tools/topology/v2/attributes.conf

+# adding ".optional" or ".mandatory" to the array name
+
+DefineType.format {
+	enum [


Can we name space this and use the "" here. e.g.

DefineType."pcm.format" {

Since "format" can be used for multiple items.

this is a type definition, not an attribute declaration. PCM and DAI formats are the same, aren't they?

lgirdwood · 2020-07-16T09:15:53Z

tools/topology/v2/attributes.conf

+	]
+}
+
+DefineType.time_domain {


ditto "pipeline.time_domain"

lgirdwood · 2020-07-16T09:17:12Z

tools/topology/v2/attributes.conf

+
+DefineType.time_domain {
+	enum [
+		"dma"


We should use the # here to comment each field. i.e.
"dma" #pipeline is scheduled via DMA interrupts

lgirdwood · 2020-07-16T09:28:32Z

tools/topology/v2/attributes.conf

+	type array.tuple
+}
+
+DefineAttribute.format {


This overlaps with DefineType. I much rather reduce the number of new keywords so

DefineAttribute."pcm.format" { type enum values [ s16le s24le [ default s16le }

but we will have several attributes of type "format." Without defining the type "format" we'll have to repeat the same enum every time. Even worse with much longer enums like widget types.

This will be global and in a global pcm.conf header. PCM format will be used in lots of places so why would we have to repeat. Can you provide an example ?

@lgirdwood Even in minimal examples we have two formats: a pipeline format and a DAI format, so we need at least two attributes of type "format" - see pipeline_format and dai_format in this PR

lgirdwood · 2020-07-16T09:29:14Z

tools/topology/v2/attributes.conf

+	]
+}
+
+DefineAttribute.tokens {


Dont need to define tokens - it's low level implementation detail only.

This is a "tokens" attribute for cases where higher level configuration files (e.g. machine level) pass their specific token arrays to lower level configuration files (pipelines). This is currently used for example to define pipelines:

dnl W_PIPELINE(stream, period, priority, core, initiator, platform) ^^^^^^^^ define(`W_PIPELINE', ... `SectionWidget."'N_PIPELINE($1)`" {' ... ` data [' ` "'N_PIPELINE($1)`_data"' ` "'$6`"' ^^^^^^ ` ]' `}')

which then is called as

W_PIPELINE(N_DAI_OUT, SCHEDULE_PERIOD, SCHEDULE_PRIORITY, SCHEDULE_CORE, SCHEDULE_TIME_DOMAIN, pipe_dai_schedule_plat) ^^^^^^^^^^^^^^^^^^^^^^

and pipe_dai_schedule_plat is an array of tokens, defined in bxt.m4 or similar. So far I don't have a good idea how to avoid such token array passing. The only thing we can make easier is make arrays of word- and string-tokens a built-in type, without this defining them directly as a structure of an array of string tokens and an array of word tokens becomes really painful.

But we are not using SectionWidget or token array directly - we abstract all tokens with Attributes.

@lgirdwood we do that where possible, yes. E.g. for volume, where it's known in advance which tokens must be supplied. But here it can be any tokens that the respective platform decides to apply. So I don't see how we can replace them with fixed attributes?

lgirdwood · 2020-07-16T09:50:41Z

tools/topology/v2/buffer.conf

+
+# A buffer block, most attributes inherited from "Widget"
+DefineWidget."buffer" {
+	attributes [


can we make these attribute mandatory and optional in the widget definition. Some widgets will only have mandatory but some will have both e.g. for buffer "size" is mandatory but "ignore_underrun" is optional and false by default.

lgirdwood · 2020-07-16T09:53:55Z

tools/topology/v2/common.conf

+		"siggen"
+		"src"
+	]
+}


Dont need this, it's low level and encoded in the CC.

don't we add new widget types from time to time? Do we really want to patch the compiler every time we do that?

lgirdwood · 2020-07-16T11:49:46Z

tools/topology/v2/dai.conf

+	default_hw_conf_id $id
+
+	# FIXME we need concatenation
+	hw_configs [


Why do you need to concat this ?

hw_configs [ config1 config2 ]

those aren't multiple configurations, that's just one configuration, generated by concatenating those two strings:

`SectionBE."'$4`" {' ... ` hw_configs [' ` "'$1$2`"' ` ]'

lgirdwood · 2020-07-16T11:51:20Z

tools/topology/v2/dai.conf

+}
+
+# SectionHWConfig
+DefineHWConfig {


DefineHWConfig."hw_cfg.ssp" etc would support per DAI type configs.

Not sure I understand correctly what you mean. This is dai.conf, so this is a generic definition of a SectionHWConfig component. "hw_cfg." seems to be unneeded - it just repeats DefineHWConfig. As for "ssp" - that would mean, that we have to instantiate SectionHWConfig components in machine configuration files. In fact this is exactly what my latest version does. But then we also have to define them per DAI type - as you say. What do we gain that way? So far we don't seem to have to do that? But that can be done, I think, let's see if we need it.

lgirdwood · 2020-07-16T11:56:28Z

tools/topology/v2/machine.conf

@@ -0,0 +1,80 @@
+# Instantiate a subtree - all the pipelines, connected to one DAI
+subtree {


I would expect the machine file to contain

DeclarePipeline."pipe-name1" { } DeclarePipeline."pipe-name2" { } DeclareDAI."dai-name1" { }

It should be representing the same data as contained in the machine M4 today.

"Declare" in your code above means instantiation? I use "new." for it now. But yes, that's also what I do.
As far as I understand, the whole DSP audio topology can be divided into "DAI groups." I.e. each DAI can have a group of pipelines associated with it, but different DAIs never link. This seems to be what I see in most pipelines ATM but in fact I suspect this isn't a general rule? There can be some pass-through paths from a DMIC to a playback DAI, right? Do we have any such examples or any other examples, where different DAIs link together?
Currently in my notation a "subtree" is such an isolated part of the topology graph and it can include several pipelines, but only one DAI. The difference to the current m4 situation, where machine files just contain collections of pipelines, DAIs, backends, etc. is, that to make that approach work our m4 files make a heavy use of string-matching, i.e. concatenation. That's how DAIs, their configurations, graphs and pipelines are matched. To avoid that and to make explicit separation of graph parts I use "subtrees." E.g. each such subtree defines global parameters like dai_format etc. Without separating subtrees this would be impossible.

lyakh · 2020-07-16T13:31:53Z

Can you make the changes and share a pipeline like
Host0 --> buffer0 --> volume0 --> buffer1 --> SSP0
We will need to have a global attributes and per widget/component/section attributes that should be prefixed to avoid collisions.

@lgirdwood you want SSP over HDA? I now have exactly that example, but with HDA because their configuration is easier, it doesn't include capture and some other extensions.

dbaluta · 2020-07-16T14:34:09Z

@lyakh a short introduction and context for this PR would be useful to have a better feel of why we are doing topology 2.0

plbossart · 2020-07-16T15:39:35Z

Can you make the changes and share a pipeline like
Host0 --> buffer0 --> volume0 --> buffer1 --> SSP0

That's precisely what I recommended we avoid last week!
Please pretty please only think of such cases for testing, this is not scalable for actual firmware graphs.

lyakh · 2020-07-17T11:52:57Z

Can you make the changes and share a pipeline like
Host0 --> buffer0 --> volume0 --> buffer1 --> SSP0
That's precisely what I recommended we avoid last week!
Please pretty please only think of such cases for testing, this is not scalable for actual firmware graphs.

@plbossart yes, this is testing so far, but at least an example graph of the kind you're thinking about would be helpful. You mentioned, that sof-byt-codec.m4 was a good example, which defines the topology as

# PCM0 -----> volume -------v
#                            low latency mixer ----> volume ---->  SSP2
# PCM1 -----> volume -------^
# PCM0 <---- Volume <---- SSP2

where pcm0->volume, pcm1->volume, mixer->volume->SSP2, and pcm0<-volume<-SSP2 are 4 pipelines. Is it really a goal to not have pipelines, containing both a PCM and a DAI? We will make this possible, but for now there might indeed be a slight implicit assumption, that one such pipeline exists in each "pipeline group," but sure, I'll make sure to remove that assumption.

lyakh · 2020-07-17T13:17:30Z

@lyakh a short introduction and context for this PR would be useful to have a better feel of why we are doing topology 2.0

@dbaluta this is a rather early stage yet: we still haven't quite converged even on a prototype of a language definition and I'm just starting very tentative coding.

The reason to try and replace the present SOF m4 topology library is the inconvenience of dealing with parameters. It's very difficult or impossible to verify macro parameters, which makes their use very error-prone. We want to fix those issues, make working with topology files more natural and add further compile-time topology checks. For this we propose to extend the present ALSA configuration language by adding primitives, that will allow us to define topology components and then to use those definitions to build specific topologies - similar to how this is done now with m4.

lgirdwood · 2020-07-20T09:19:20Z

Can you make the changes and share a pipeline like
Host0 --> buffer0 --> volume0 --> buffer1 --> SSP0
That's precisely what I recommended we avoid last week!
Please pretty please only think of such cases for testing, this is not scalable for actual firmware graphs.

Can you link in your bug/pr for reference here to remind me. I cant remember if this was to resolve some driver issues (sorry not tracking these atm).

dbaluta · 2020-07-20T11:07:15Z

@lyakh a short introduction and context for this PR would be useful to have a better feel of why we are doing topology 2.0

@dbaluta this is a rather early stage yet: we still haven't quite converged even on a prototype of a language definition and I'm just starting very tentative coding.

The reason to try and replace the present SOF m4 topology library is the inconvenience of dealing with parameters. It's very difficult or impossible to verify macro parameters, which makes their use very error-prone. We want to fix those issues, make working with topology files more natural and add further compile-time topology checks. For this we propose to extend the present ALSA configuration language by adding primitives, that will allow us to define topology components and then to use those definitions to build specific topologies - similar to how this is done now with m4.

@lyakh thanks for explanation. Make sure this is also present in commit messages / PR description.

lyakh · 2020-07-20T13:43:32Z

There is a problem with this proposal: in ALSA configuration language '.' is used to describe parent-child relationships. And in this PR constructs like "DefineWidget.pipeline" or "new.widget.pcm" don't really describe such relationships. Moreover, in a fragment like

new.widget.pipeline {
	format s16le
	new.widget.buffer {...}
}

the "pipeline" widget will have 2 children: "format" and "new" which doesn't seem to make good sense.

lgirdwood · 2020-07-21T13:42:19Z

@lyakh please see #3211. This is what I'm looking for.

ranj063 · 2020-11-19T19:14:55Z

tools/topology/v2/pipe-volume-playback.conf

@@ -0,0 +1,162 @@
+# "DefineTemplate" doesn't seem to be needed


@lyakh I think we should have DefineTemplate{}. Ideally we should have DefineTemplate{} follow all Define*{} for that kind. For example, DefineMixerControl{} should have a DefineTemplate.mixercontrol{}, with default values so that we can instantiate a mixercontrol with that template and modfied attributes (if needed) easily.

@ranj063 #3211 is the one to look at.

paulstelian97 · 2021-03-29T09:02:55Z

A new pull request was probably in order anyway, but now you have to do it because the base branch "master" is gone.

lyakh requested review from lgirdwood and plbossart July 14, 2020 14:59

lgirdwood requested changes Jul 16, 2020

View reviewed changes

ranj063 reviewed Nov 19, 2020

View reviewed changes

lgirdwood closed this Mar 28, 2021

lgirdwood deleted the branch thesofproject:master March 28, 2021 13:44

		@@ -0,0 +1,80 @@
		# Instantiate a subtree - all the pipelines, connected to one DAI
		subtree {

		@@ -0,0 +1,162 @@
		# "DefineTemplate" doesn't seem to be needed

[REVIEW ONLY] topology 2.0 #3183

[REVIEW ONLY] topology 2.0 #3183

Uh oh!

Conversation

lyakh commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jul 16, 2020

Uh oh!

dbaluta commented Jul 16, 2020

Uh oh!

plbossart commented Jul 16, 2020

Uh oh!

lyakh commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Jul 17, 2020

Uh oh!

lgirdwood commented Jul 20, 2020

Uh oh!

dbaluta commented Jul 20, 2020

Uh oh!

lyakh commented Jul 20, 2020

Uh oh!

lgirdwood commented Jul 21, 2020

Uh oh!

ranj063 Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jul 14, 2020 •

edited

Loading

lyakh commented Jul 15, 2020 •

edited

Loading

lyakh commented Jul 17, 2020 •

edited

Loading

ranj063 Nov 19, 2020 •

edited

Loading