Skip to content

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Mar 19, 2021

This replaces #3211

Copy link
Member

@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.

Looks good, but maybe we can automatically get index from the object name ? Saves duplication. After we merge this we will need to think about CI and incrementally moving machines over to 2.0 for CI to validate.
Btw, IIUC we should be able to define a "DaiGateway" class that contains both a DAI object and buffer object i.e. all the config for DAI, DMA and buffer ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where are these version comparisons used and what for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just copy paste problem. removed now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just realized this... do we really need two get_abi.sh scripts? (the CMakeLists.txt duplication seems harder to avoid)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the way in which the Manifest data is represented is different in topolog2.0, so yes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, so the only duplication is the small bit that parses the C macros. It should ideally be moved to some common (CMakeLists.txt?) file but it's just 3 lines so no big deal.

Copy link
Collaborator

@marc-hb marc-hb Mar 19, 2021

Choose a reason for hiding this comment

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

Please add set -e. DONE

Please move all the code to a main "$@" function, see example in build-tools.sh above.
Pros:

  • can declare other functions in any order
  • can use local variables to avoid shadowing
  • easier to source for interactive debug, just comment out the last line
  • keeps the same indentation level and minimizes diff when moving code around functions

Cons:

  • None

@ranj063 ranj063 requested review from lgirdwood and marc-hb March 19, 2021 18:48
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

cmake and shell scripts LGTM

@ranj063 ranj063 force-pushed the topic/topology2 branch 8 times, most recently from 2788a5e to 557c189 Compare March 22, 2021 06:00
Copy link
Collaborator

Choose a reason for hiding this comment

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

commit message:

  • "howthe" typo
  • are attributes 1-to-1 with tokens? If so, would be good to mention that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the compiler will then combine the token_ref with the attribute name via an underscore to generate the final SOF_TKN_COMP_PERIOD_SINK_COUNT token. Is this documented somewhere? Do we really need to specify the type after the token name? Isn't it enough to define the token, can the same token be used with different types? Probably not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm there no combining here. The name of the token set is sof_tkn_comp and the name of the token in there is period_sink_count. What did you mean by combining?

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 interesting. Looks good! Wondering what other implications or potential uses this can have. Alternatively we could have just primary_mixer_name or something similar and just propagate it. Would probably be easier for the compiler, but less elegant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I started of with passing with a primary_mixer_name/primary_switch_name but there could be cases where there are more than 1 pga widget in a pipeline. THen we'll have to pass 4 names. This way we know exactly what we're setting

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this imply any kind of inheritance from Class.Component."dai"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the component is for the DAI widget and this is config for the DAI itself

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain what exactly "embedding" those 3 objects here mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, a DAI needs a hw_config and 1/2 DAI widgets depeding on whether it is uni-directional or duplex. And these 3 opbjects are for those.

@ranj063 ranj063 requested a review from lyakh March 22, 2021 17:17
@ranj063 ranj063 force-pushed the topic/topology2 branch 4 times, most recently from 9927858 to f43755a Compare March 23, 2021 02:10
@marc-hb marc-hb self-requested a review March 23, 2021 05:17
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 23, 2021

cmake and shell scripts LGTM

I take some of that back: by chance I found that the CMakeLists.txt is missing a dependency on .m4 files, meaning incremental builds don't rebuild anything. Please help review #3945 that fixes this issue in the topology v1 original, then the fix can be copied to this duplicate CMakeLists.txt

Interesting and timely reminder that copy/paste/diverge sucks. Unfortunately I can't think of a simple way to avoid that CMake duplication for now. Anyone?

@ranj063 ranj063 force-pushed the topic/topology2 branch 2 times, most recently from a213471 to d32f12c Compare March 23, 2021 05:45
ranj063 added 12 commits March 22, 2021 22:58
    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
    definition for a AIF_IN/AIF_OUT type object that can be used to create
    topology objects.

    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. A new keyword
    "DefineArgument" has been added to define the arguments. The order in
    which the arguments are defined determines the name for the widget.
    For example, in the case of the host widget, the name would be
    constructed as "host.1.playback" where "1" is the pipeline_id argument
    value and "playback" is the direction argument value.

    Attribute Inheritance:
    ----------------------
    One of the key features of Topology2.0 is howthe attribute values are
    propagated from a parent object to a child object. This is accomplished
    by adding attributes/arguments with the same name for a parent and an
    object. By doing so, when creating a child object, the value for the
    common attribute is populated from the parent. If the value is provided
    in the child object instance, then it overrides the value coming from
    the parent.

    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.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add the class definition for DAI components. A DAI widget
would be constructed as follows:
Object.dai."playback" {
	type			SSP
	index			1
	direction		"playback"
	period_sink_count	"2"
	period_source_count	"0"
	widget_type		"dai_in"
}

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add the definition for the buffer class. A buffer object
can be instantiated as follows:
Object.buffer."N" {
	pipeline_id	1
	index	0
	size	384
	caps	"host"
}

where 'N' is a unique instance number for the buffer object within
the same alsaconf node.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add the class definition for PGA widget. It can be
instantiated as follows:
        Object.pga."N" {
                pipeline_id     1
                index           0
                period_source_count     "2"
                period_sink_count       "2"
        }

Where N is the unique instance number for pga widget in the same
alsaconf node.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add the class definition for a mixer control.
A mixer control can be instantiated as follows:
	Object.mixer."0" {
		#Channel register and shift for Front Left/Right
		Object.channel."0" {
			name	"fl"
			shift	0
		}
		Object.channel."1" {
			name	"fr"
		}

		Object.tlv."0" {
			name	"vtlv_m64s2"
			Object.scale."0" {
				name	"m64s2"
				mute	1
			}
		}

		Object.ops."0" {
			name	"ctl"
			info	"volsw"
			thesofproject#256 binds the mixer control to volume get/put handlers
			get	256
			put	256
		}
	}

Also add the other commonly used class definitions that
will be used to instantiate the mixer object such as,
channel, TLV, ops, scale etc.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add the volume and switch mixer control objects for the
PGA class. The mixers will be added to the PGA widget
if the pga widget is instantiated with the volume_mixer_name
and switch_mixer_name attributes set.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add a connection class that will be used for creating DAPM routes
between widgets. A route can be between 2 widgets or between a
widget and pipeline endpoint as follows:
	Object.connection."N" {
		source	"Object.SSP.1.dai.capture"
		sink	"Object.volume-playback.0.endpoint.0"
	}

	Object.connection."N" {
		source	"Object.buffer.2.1"
		sink	"Object.pga.2.1"
	}

Also, add a helper class for creating pipeline endpoints.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add 2 pipeline classes for Volume playback and Volume Capture.
Each of these classes includes the widget objects, their connections
and define attributes for the pipeline.

A volume playback pipeline can be instanted as follows:
Object.pipeline-volume-playback."2" {
	pipeline_id	2
	pcm_id		1
	channels	2
	channels_min	2
	channels_max	2
	rate		48000
	rate_min	48000
	rate_max	48000
	pcm_name		"Headset"
	format			"s32le"
	volume_mixer_name	"2 Master Playback Volume"
}

Every pipeline class is associated with a pipeline widget, so
add the class definition for the pipeline widget as well.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add the definitions SSP DAI and the hw_config classes.

These are used for instantiating the DAI objects in the
top-level topology file as follows:
Object.SSP."0" {
	index			0
	direction		"duplex"
	stream_name		"NoCodec-0"
	id 			0
	default_hw_config	0
	format			"s24le"
	quirks			"lbm_mode"
	sample_bits		24
	Object.hw_config."0" {
		id		0
		mclk_freq	24000000
		bclk_freq	4800000
		tdm_slot_width	25
	}

	#Add DAI widgets
	Object.dai."playback" {
		direction		"playback"
		period_source_count	2
		period_sink_count	0
	}

	Object.dai."capture" {
		direction		"capture"
		period_source_count	0
		period_sink_count	2
	}
}

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add class definitions for PCM and PCM Capabilities. These
can be instantiated as:
	Object.pcm."N" {
		pcm_name	"Headset"
		direction	"playback"
		pcm_id		2
	}

	Object.pcm_caps."N" {
		pcm_name	"Headset"
		direction	"playback"
		pcm_id		2
		formats 		"S32_LE,S24_LE,S16_LE"
		rate_min 		48000
		rate_max 		48000
		channels_min 		2
		channels_max 		2
		periods_min 		2
	}

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
These will be used to add the ABI to the topology
manifest section.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Add a simple machine topology showing 2 pipelines,
an SSP DAI and the connections between the DAI
endpoints and the pipeline endpoints.

Add the manifest and data objects for adding ABI to the
generated tplg file. Also add support for building
the topology2 conf files.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@lgirdwood lgirdwood closed this Mar 28, 2021
@lgirdwood lgirdwood deleted the branch thesofproject:master March 28, 2021 13:44
@paulstelian97
Copy link
Collaborator

Please resubmit with "main" as PR base branch.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 30, 2021

BTW https://github.com/github/renaming

Renaming a branch will:

  • Re-target any open pull requests
  • etc.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 30, 2021

I take some of that back: by chance I found that the CMakeLists.txt is missing a dependency on .m4 files, meaning incremental builds don't rebuild anything. Please help review #3945 that fixes this issue in the topology v1 original, then the fix can be copied to this duplicate CMakeLists.txt

@ranj063 #3945 is merged, please copy/paste.

@cujomalainey
Copy link
Contributor

Lol well that sucks, oh well, we can call it also a PR cleanup for any abandoned stale PRs

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.

6 participants