Skip to content

Conversation

@lgirdwood
Copy link
Member

About

This is a high level keyword extension on top of the existing ALSA conf
topology format designed to:

  1. Simplify the ALSA conf topology definitions by providing high level
    "classes" so topology designers need to write less config for common
    object definitions.

  2. Allow simple reuse of objects. Define once and reuse (like M4) with
    the ability to alter objects configuration attributes from defaults.

  3. Allow data type and value verification. This is not done today and
    frequently crops up in FW bug reports.

Common Topology Classes

Topology today has some common classes that are often reused throughout
with slightly altered configurations. i.e. widgets (components), pipelines,
dais and controls.

This PR introduces the high level concept of reusable "class" like
definitions that can be used to create topology objects e.g.

a) ClassComponent - creates a class for widgets that can be instantiated
by parent objects. Used to create new widget objects (components).
See volume.conf and buffer.conf

b) ClassPipeline - creates a class for pipelines that can be instanciated
by parent objects. See pipeline.conf

c) ClassDAI - like 1 and 2 for DAIs. See ssp.conf

These classes are all instantiated by parent objects. See pipeline.conf
and machine.conf

Common Topology Attributes

Topology defines a lot of attributes per object with different types and
constraints. Today there is no easy way to validate type or constraints
and this can lead to many hard to find problems in FW at runtime.

A new keyword "DefineAttribute" has been added to define attribute type,
size, min value, max value, enum_values. This then allows alsatplg to
validate each topology object attribute.

Topology Classes define the list of attributes that they use and whether
the attribute is mandatory, can be overridden by parent users or is
immutable. This also helps alsatplg emit the appropriate errors for
attribute misuse.

ALSA Conf Parser

All the changes being proposed and discussed here must be 100% compliant
with the ALSA conf parser. i.e. no syntax changes or changes to semantics
for any existing keyword.

It's intended that there will be NO changes to the ALSA conf parser (unless
new keywords require this ?) and all topology building changes will be
in the alsatplg compiler.

alsatplg Compiler

The alsatplg compiler will require changes to construct and validate
objects based on the new keywords. No changes to existing keywords.
Existing topologies will not change.

Opens

  1. There is a lot of replicated boiler plate code that has to be repeated
    in many locations. How do we reduce to simplify for pipeline and
    component designers.

  2. What is the best way to inherit the widget (component, pipeline, dai)
    index when constructed. This could be done as an $ARG, but alsatplg
    compiler can also do this (and probably validate).

  3. We need to define a set of platform constraints and capabilities and
    include them so that alsatplg can apply these platform constraints on
    top of existing class constraints. e.g. valid SSP port number is 3 or 6
    depending on the platform.

Signed-off-by: Liam Girdwood liam.r.girdwood@linux.intel.com

include("buffer.conf")
include("ssp.conf")
include("icl.conf")
include("pipeline-volume.conf")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALSA configuration uses "<file>" to include further configuration files

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, can you fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, can you fix.

@args.INDEX {
type integer
default {
name buffer.index
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, don't understand. The default value of an integer parameter should be an integer, right? What's this trying to say?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy and paste from dmix code. I'm not keen on defining pipeline/component naming/index rules in the conf file so this can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from dmix example - cant be dropped. I don't like setting the naming rules here where they can be easily broken. Lets do this in the compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh... This is a default value for the buffer's index attribute, not related to the INDEX argument. It seems to be in the wrong scope then. And it seems to me, that you're using this default statement to define the buffer name, which isn't defined in the topology file, i.e. it would rely on some hard-coded compiler behaviour.

format [
"s16le"
"s24le"
"s32le"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be clearer if we didn't enclose enum type values in quotes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything here is string based for the parser ? plus I don want some elems being strings and other not , just leads to confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ? preference - it's clear if we have consistent rules that are easy to use.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parser doesn't care, right, so, this isn't too important, I'm not proposing to enforce this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's about the use. The value can be converted to string using snd_config_get_ascii() if required. So even integer 0 may be handled as "0" in the topology library.

]

rates [
"48000"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same with numbers

pipelines [

# Use volume pipeline and connect to SSP0.
"pipeline-volume.0" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an instantiation, right? But you have no special syntax for it. How do you propose to programmatically differentiate it from an ordinary assignment? I'd do this explicitly. Besides, this is actually invalid syntax: you cannot use assignments in an array. So this would be something like

pipelines [
	{
		name "pipeline-volume.0"
		format [...]
		...
	}
]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, instantiation - ok so lets use valid syntax. Instead of name lets use class


# Pipeline ID
DefineAttribute."index" {
type "word"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't integer be enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep.

# format [
# s16le
# s24le
# ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently all configurations only define one pipeline and one DAI formats, not sure how you're proposing to use multiple ones?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will use arrays.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm asking, because the DAI format is used in calculations as a single parameter, e.g. for buffer size calculations.

# bclk_dai_leader "true"
# ssp_quirk1 "true"
# ssp_quirk2 "333"
# format [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you didn't define format here, would you again define it as

DefineAttribute."format" {
		type	"enum"
		values [
			"s16le"
			"s24le"
			"s32le"
		]
		default "s32le"

repeating a previous definition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this stuff is by no way complete, it's just meant to be an example.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it really better to repeat the same pattern instead of using DefineType to define a "format" enum type and then just using type format everywhere?

"buffer.$INDEX.0" {
size "384"
caps "101"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In present configurations this buffer uses the pipeline format to calculate its size.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we can change that

max_db "0"
mute "true"
}
"buffer.$INDEX.1" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this buffer uses the DAI format. How would the compiler know that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endpoint connections will point to the DMA related buffer. This can then be worked out by compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this can work, but

  1. you're proposing arrays of formats
  2. cannot there be cases where a pipeline would decide something different? Do we really want to hard-code that?

# Argument used to construct pipeline ID (widget index)
#

@args [ INDEX ]
Copy link
Collaborator

@lyakh lyakh Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assigned by the compiler? Is this what you propose to use @args for? I have availability automatic in my version for this.

@paulstelian97
Copy link
Collaborator

Is it possible to have DAI-less pipelines? A sort of loopback, where you have two HOST components instead, one Playback and one Capture. This is to be able to use the DSP as merely an accelerator rather than the full blown playback mode.

Some userspace libraries only work like that from what I've been told: Use DSP to decode, get the data back into RAM, then play them through a regular ALSA interface.

Copy link
Member Author

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general flow here is that

  1. all conf files parse into snd_elems or equivalent snd_ structs by ALSA parser.
  2. topology compiler builds elems from 1 into data tree and sorts by type for easy lookup.
  3. topology compiler takes data from machine.conf and uses this data to construct the topology.
  4. topology compiler then validates the data.

@lgirdwood
Copy link
Member Author

Is it possible to have DAI-less pipelines? A sort of loopback, where you have two HOST components instead, one Playback and one Capture. This is to be able to use the DSP as merely an accelerator rather than the full blown playback mode.

Yes - no restrictions to what can be defined..


# connects sink to SSP.0
endpoint."sink.0" {
dai "ssp.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this adding a dai "ssp.0" entry to the endpoint definition from pipeline.conf, thus making it effectively

	DefineEndpoint."sink.$INDEX.0" {
		name		"buffer.$INDEX.1"
		complete	"false"
		dai		"ssp.0"
	}

?

@lgirdwood
Copy link
Member Author

@perexg as been discussed on call.

@perexg
Copy link

perexg commented Aug 21, 2020

@lgirdwood : Thanks.

I basically like the templating and the field validation idea. The @Args use and the whole runtime evaluation (string substitution etc.) can be implemented in a different way in the topology library / topology configuration. An example is the UCM, where only the basic alsa-lib configuration parser is used and the runtime evaluation is different (usage specific).

We should also fix the problem described in #2937 (comment) in the new syntax.

@lgirdwood
Copy link
Member Author

@perexg thanks for your inputs, @lyakh is back next week and has a DMA issue to fix prior to restrating back on topology. We both want to make sure you are 100% happy with the proposed keyword updates before we go ahead and write any C code in alsatplg. M4 is just getting too cumbersome now.

@lgirdwood
Copy link
Member Author

@cujomalainey @dbaluta fyi.

@cujomalainey
Copy link
Contributor

@dgreid @chiang831

#

# mixer control for channels 0,1
control."volume.0" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood the control name should also be an argument I'd think that comes from the machine.conf isnt it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the control class will have a name and the control object instanciation will allow fo rthe kconrtol to be named. e.g. some combination of class name + object name like "(object)Master Playback (class)Volume"

@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch from b2df9d4 to ee610b1 Compare December 9, 2020 07:12
@ranj063
Copy link
Collaborator

ranj063 commented Dec 9, 2020

@lgirdwood @perexg @lyakh This PR is updated now to include building a full topology (sof-cnl-nocodec.conf).
I have also updated the description based on the changes. Please take a look. Thanks!

@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch 2 times, most recently from 44d96ac to c43f98e Compare December 9, 2020 07:21
Copy link
Member Author

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the machine definition at the end - it's really simple to understand.
I think we do need more comments in the base so that everything is easily understood and parsable via doxygen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a mandatory comment description for each class. Doxygen should be able to parse these when ready to help users.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, also in my list of todo's

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also have a doxygen comment for arg too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are probably "attributes" instead of constraints ? Do we need the extra mask[] here too or can we an attributes [] at the top DefineAttribute level ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the mask is easier to understand better when its within the attribute definition instead of clubbing all attributes into the "mask" attribute. Let me experiment a bit more and see if I can make this easier

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to add attributes{} at the top-level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to link to a pcm_caps object ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, exactly!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we do more than one token group or name in this section ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry didnt get that question. Why would we need more than one token group? Are you thinking platform-specific tokens for all platforms?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have some components that have different component types (like ints and strings) and hence more than one tuple group. I assume we can manage them ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeas. we already handle them in this version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need min, max values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs min, max here too (and in other places)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes, I havent added constraints everywhere yet. The all attribute constraints need to be sanitized in all classes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min cant be 0 (or it will never schedule, 333 might be more reasonable as it's 16 frames @ 48kHz).
max should be something like 50ms

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to also have comments following
"timer" #Scheduler is run via a timer interrupt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets have a DefineABI(major, minor, micro) that does this automatically for us.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, its in my list of todo's already

@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch 3 times, most recently from c84a23f to 4c239d6 Compare December 11, 2020 06:13
@ranj063
Copy link
Collaborator

ranj063 commented Dec 11, 2020

@lgirdwood sof-cnl-nocodec.conf is updated for 2 pipelines now and tested on the helios to work.

@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch from 4c239d6 to a12fa26 Compare December 12, 2020 04:08
Copy link
Member Author

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - btw does doxygen or similar work with conf ?

}

DefineAttribute."info" {}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define some enums here to help mapping the control to a control type. I think we use int's today but we can use a enum here and the compiler can map it to the int.

}
}

Class.Base."scale" {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw - do we need to say Class.Base ? can we just use Class instead ?


# Pipeline ID:1 PCM ID: 0
Object.pipeline-volume-playback."1.0" {
pcm_name "Port0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want to give this a more meaningful name than Port0 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be SAI0 / SSP1, etc?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaluta this can literally be anything. On a real device for example, this would be "Headset", "Speaker" etc. But for nocodec, its simply SSP Port 0


DefineAttribute.rate {
constraints {
min 48000
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8000

#
DefineAttribute.channels {
constraints {
min 2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should support mono since it's generic

pcm_name "Port0"
format "s24le"
channels 2
rate 48000
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you overriding any of the class default values ? if so, we should say so.

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising :-)

@@ -0,0 +1,27 @@
Class.Base."connection" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might need to first declare the array as

@args [ type pipeline_id index ]


# Default values for PCM attributes
compress "false"
pcm_caps_name "$$pcm_name.$direction.$pcm_id"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is two dollar-signs in a row? Is it valid in ALSA-conf? Seems to be a typo. But something we'll have to face: currently the ALSA configuration language has no support for "seamless" string concatenation. It can be done by calling a function, but that's rather cumbersome.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just experimenting here. the double $ sign is not really needed.

}

# Pipeline objects
host."$pipeline_id.capture" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do I understand it correctly, that with this you want to teach the ALSA configuration compiler to inherit this from Class.Component."host" ? Shouldn't all these have Object. in front of them?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thats right. The Object. prefix is optional (within a class definition). In the top-level file, it is a requirement.


# Pipeline objects
host."$pipeline_id.capture" {
period_sink_count 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is inherited from Class.Component."host" -> component.conf

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right!


DefineAttribute.index {
# Token reference and type
token_ref "sof_dai_tokens.word"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do I understand it correctly, that you're proposing to make SectionVendorTokens a keyword, that tells the compiler, that it's a composite object with uuid, word and string attributes? Also token_ref is also a keyword, that will tell the compiler to look up the RHS among SectionVendorTokens definitions? Or should these use $ to indicate that they are objects to be replaced by values, not just strings?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, tplg2.0 reuses some of the existing keywords, with SectionVendorTokens being one of them. SectionData is the other one that I have reused for the manifest

# Argument used to construct component: pipeline ID
#
@args."pipeline_id" {
type "integer"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where these arguments, e.g. pipeline_id are set? I'd expect them to be set in the top-most configuration, e.g. in sof-cnl-nocodec.conf, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Args are meant to be set when the object is created. So think of it as arguments to the object's constructor


# Pipeline ID:1 PCM ID: 0
Object.pipeline-volume-playback."1.0" {
pcm_name "Port0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the PCM object instantiated? Currently pipeline "classes" don't instantiate PCMs, which allows connecting different PCMs to different and possibly multiple pipelines. If we want to follow that pattern, then the pcm_name shouldn't be set in a pipeline instance? Instead a PCM object should be instantiated separately and the two should be linked together?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are instantiated in the pipeline class. So 2 pipelines (for different directions) would instatiate the PCM but the alsatplg compiler would only create one PCM instance based on the name and set the capabilities for each direction from the 2 different instantiations in the pipeline class.


@args."pcm_id" {
type "integer"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for unordered comments, I'm learning as I'm reading and this process isn't linear :-) It now seems to me, that when you defing @args you imply, that upon instantiation those arguments will be set from the instance "name?" E.g. if you define Class.myclass with @args.arg1; @args.arg2 and then do Object.myclass.0.1 that means, that an instance of myclass is created with arg1 = 0 and arg2 = 1. Is this an existing ALSA convention? If not - what is the exact proposed syntax? Is it

Class.<group>.<class> {}
Object.<class>[.<arg1>[.<arg2>[...]]]

so class names are unique across all groups? Wouldn't this be a bit too restrictive? Wouldn't it be better to set all that explicitly, e.g.

Object.<group>.<class> {
args.<arg1> <value1>
...
}

e.g.

Object.Pipeline.pipeline-volume-playback {
args.pipeline_id 0
...
}

# Pipeline connections
connection."graph.$pipeline_id.0" {
sink "host.$pipeline_id.capture"
source "buffer.$pipeline_id.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I can understand these: the class connection has two DefineAttribute statements - for sink and source and these set them.


DefineAttribute.core_id {
# Token set reference name and type
token_ref "sof_tkn_comp.word"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I think I know what you mean by this. Speaking in "old terms," you want to have the compiler join "sof_tkn_comp" and "core_id" and generate a SOF_TKN_COMP_CORE_ID token, which will be set from this attribute. Isn't this too restrictive? Elgl DMICs currently have this:

SectionHWConfig."DMIC0" {

	id		"6"

	}
SectionVendorTuples."DAICONFIG.DMIC0_dmic_tuples" {
	tokens "sof_dmic_tokens"
	tuples."word" {
		SOF_TKN_INTEL_DMIC_DRIVER_VERSION	"1"
		SOF_TKN_INTEL_DMIC_CLK_MIN		"500000"
		SOF_TKN_INTEL_DMIC_CLK_MAX		"4800000"
		SOF_TKN_INTEL_DMIC_DUTY_MIN		"40"
		SOF_TKN_INTEL_DMIC_DUTY_MAX		"60"
		SOF_TKN_INTEL_DMIC_SAMPLE_RATE		"48000"
		SOF_TKN_INTEL_DMIC_FIFO_WORD_LENGTH	"32"
		SOF_TKN_INTEL_DMIC_UNMUTE_RAMP_TIME_MS	"400"

						SOF_TKN_INTEL_DMIC_NUM_PDM_ACTIVE	"1"
	}
}
SectionVendorTuples."DAICONFIG.DMIC0_pdm_tuples" {
	tokens "sof_dmic_pdm_tokens"
	tuples."short.pdm0" {
		SOF_TKN_INTEL_DMIC_PDM_CTRL_ID		"0"
		SOF_TKN_INTEL_DMIC_PDM_MIC_A_Enable	"1"
		SOF_TKN_INTEL_DMIC_PDM_MIC_B_Enable	"1"
		SOF_TKN_INTEL_DMIC_PDM_POLARITY_A	"0"
		SOF_TKN_INTEL_DMIC_PDM_POLARITY_B	"0"
		SOF_TKN_INTEL_DMIC_PDM_CLK_EDGE	"0"
		SOF_TKN_INTEL_DMIC_PDM_SKEW		"0"
	}


}

do we really want to define an attribute for each of them?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really want to define an attribute for each of them?

Unfortunately, I cant think of a better way than having a separate attribute for each of them. Im all ears if you have a suggestion

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, attributes must be defined for all parameters so that the compiler will validate each (type, value) as part of the topology build.

@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch from a12fa26 to a1f0640 Compare February 11, 2021 07:17
@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch 7 times, most recently from 06825b9 to 4993e47 Compare February 18, 2021 18:38
@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch 5 times, most recently from d3aa251 to 937a805 Compare February 19, 2021 18:27
data."sof_manifest" {
bytes "0x03,0x12,0x01"
}
} No newline at end of file
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a new line and ABI should be decimal. Btw, will this be private data tuple of teh manifest ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood this is a generated file. Ideally it should go into the build folder. But because alsatplg only allows 1 base include folder, I had to generate it here. Nevertheless, this shouldnt be in the PR let me remove it. And yes, this will be private data in the manifest

<include/pipelines/cavs/pipeline-passthrough-capture.conf>
<include/common/connection.conf>
<include/common/endpoint.conf>
<include/dais/ssp.conf>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best to order this on dependencies, i.e. common at the top

id 0
format "s24le"
sample_bits 32
quirks 64
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be named boolean attributes rather than a value. e.g.

attribute."ssp_do_this" {
    type boolean
    default false
   tuple_id ssp_do_this_quirk
}

it can then be invoked via

ssp_do_this "true"

mandatory [
"widget"
]
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will need some comment in bases classes describing the attributes


DefineAttribute.buffer_size_min {}

DefineAttribute.buffer_size_max {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These and all other attributes should all have default values. This way it's easy to grep for the value and change the default.

format "s24le"
rate 48000
ch 2
hw_config."0" {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the SSP config somewhere ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is only for the SDW topology but the SSP configs are in the sof-tlg-max98373-rt5682.conf file.

@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch from 937a805 to 0a3abc0 Compare March 3, 2021 05:10
@lgirdwood
Copy link
Member Author

@ranj063 - LGTM, I think we should split and merge platform by platform and check for regressions.in the CI.
Once we have stabilised, this should be merged into ALSA to support Hybrid pipelines.

Copy link
Contributor

@cujomalainey cujomalainey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a great improvement from my initial review. I like where it is going and looks much more robust and less fragile than current M4 system. Looking forward to replacing current system in chromebooks with this system.

@ranj063
Copy link
Collaborator

ranj063 commented Mar 9, 2021

this is a great improvement from my initial review. I like where it is going and looks much more robust and less fragile than current M4 system. Looking forward to replacing current system in chromebooks with this system.

Thanks @cujomalainey. I'm glad to hear that this is more intuitive. I plan to complete the work for a few topologies and the alsatplg compiler code cleanup by end of week. Please stay tuned!

@ranj063
Copy link
Collaborator

ranj063 commented Mar 11, 2021

parser support: thesofproject/alsa-lib#2

About
-----

This is a high level keyword extension on top of the existing ALSA conf
topology format designed to:

1) Simplify the ALSA conf topology definitions by providing high level
   "classes" so topology designers need to write less config for common
   object definitions.

2) Allow simple reuse of objects. Define once and reuse (like M4) with
   the ability to alter objects configuration attributes from defaults.

3) Allow data type and value verification. This is not done today and
   frequently crops up in FW bug reports.

Common Topology Classes
-----------------------

Topology today has some common classes that are often reused throughout
with slightly altered configurations. i.e. widgets (components),
pipelines, dais and controls.

This PR introduces the high level concept of reusable "class" like
definitions that can be used to create topology objects e.g.
Class.Component - Class for widgets that can be instantiated by pipeline
classes/objects. See volume.conf, buffer.conf, host.conf, dai.conf in
<include/components>

Class.Pipeline - Class for pipelines that can be instantiated in the
top-level conf files. See pipeline-volume.conf in <include/pipelines>

Class.DAI - Class for DAIs such as SSP/DMIC/HDA etc.
See ssp.conf in <include/dais>

Class.Control- Class for mixers, byte controls, enum controls etc.
See mixer.conf in <include/controls>

Class.Base - Class for generic objects that cannot be inherited and
are not usually instantiated as stand-alone objects but embedded within
classes/objects of the above types.
Ex: hw_config class for SSP hw_config params,
see ssp_hw_config.conf in <include/dais>

Common Topology Attributes
--------------------------

Topology defines a lot of attributes per object with different types and
constraints. Today there is no easy way to validate type or constraints
and this can lead to many hard to find problems in FW at runtime.

A new keyword "DefineAttribute" has been added to define attribute type,
size, min value, max value, enum_values. This then allows alsatplg to
validate each topology object attribute.

Topology Classes define the list of attributes that they use and whether
the attribute is mandatory, can be overridden by parent users or is
immutable. This also helps alsatplg emit the appropriate errors for
attribute misuse.

Common Topology Arguments
-------------------------

Arguments are used to pass essential data needed for instantiating an
object particularly needed for the object name. Ex: host."5.playback".
The first part in the object name stands for the class name “host” and
the remaining “5.playback” stand for the 2 arguments needed to
instantiate the host object i.e.. pipeline_id and direction.

ALSA Conf Parser
----------------

All the changes being proposed and discussed here must be 100% compliant
with the ALSA conf parser. i.e. no syntax changes or changes to semantics
for any existing keyword.

It's intended that there will be NO changes to the ALSA conf parser
(unless new keywords require this ?) and all topology building changes
will be in the alsatplg compiler.

alsatplg Compiler
-----------------

The alsatplg compiler requires changes to construct and validate objects
based on the new keywords. No changes to existing keywords. Existing
topologies will not require any changes. The only two new keywords
proposed at the top-level are “Class” and “Object”. All other
keywords for class arguments and attributes are limited within the
tplg2 compiler changes. Topology2 compiler changes also re-uses most
of the code in the conf parser for parsing the configuration for
widgets, controls, DAI, hw_config, backends etc.

Opens
-----

Add platform constraints and capabilities and include them so that
alsatplg can apply these platform constraints on top of existing class
attribute constraints such as valid SSP port number is 3 or 6 depending
on the platform.

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063 ranj063 force-pushed the lrg/topic/topology2 branch from 461dd26 to b544505 Compare March 11, 2021 02:47
@lgirdwood
Copy link
Member Author

Shall we close here and continue on the other PR ?

@ranj063
Copy link
Collaborator

ranj063 commented Mar 19, 2021

replaced with #3936

@ranj063 ranj063 closed this Mar 19, 2021
@lgirdwood lgirdwood deleted the lrg/topic/topology2 branch April 1, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants