Topology clean ups - Part 2 #6627

ranj063 · 2022-11-17T18:56:29Z

This is rebased on top of #6592 and should be merged only after that one is merged

plbossart

thanks for starting this @ranj063. These days all I do is review your PRs :-)

Couple of suggestions below

plbossart · 2022-11-17T20:00:18Z

tools/topology/topology2/cavs-sdw.conf

i find it hard to review this.

dai_index = 2 is seen on both line 123 and 140. is this a bug or a feature?

its a feature. This one is FE DAI index 2 and the other one is BE DAI index 2

plbossart · 2022-11-17T20:00:33Z

tools/topology/topology2/cavs-sdw.conf

dai_index always before stream name?

plbossart · 2022-11-17T20:02:09Z

tools/topology/topology2/include/components/copier.conf

them why not use the same value to FE DAI index and PCM ID with a macro or something that enforces this rule?

yes, I think I should do that for my own sanity and others' as well

plbossart · 2022-11-17T20:03:40Z

tools/topology/topology2/cavs-mixin-mixout-hda.conf

do we ever have a case where dai_type and copier_type are different

copier_type is applicable to all copiers (host, module, DAI copiers) whereas dai_type is only applicable for the DAI copiers. When set, the dai_type and copier_type must match. But we need 2 attributes because the dai_type is added as a token whereas copier_type is purely for the use of naming the copier

plbossart · 2022-11-17T20:07:39Z

tools/topology/topology2/cavs-sdw-src-gain-mixin.conf

For ALH I am not sure if the dai_index has any meaning. It's my understanding that the same copier or gateway will manage streaming over multiple links.

@bardliao need your help here.

plbossart · 2022-11-17T20:12:14Z

tools/topology/topology2/cavs-nocodec.conf

do we need to add the pipeline ID? It's used for topology1 and helps visualize the pipeline boundaries.

The position in the pipeline could also be useful for controls.

Maybe we need

<pipeline> <position in pipeline>

for all modules, and

<dai_index>

in addition as a suffix for copiers only? (with the caveat on dai_id for ALH above)

It is not used at all. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

tools/topology/topology2/include/dais/hda.conf

tools/topology/topology2/platform/intel/sdw-amp-generic.conf

kv2019i

Changes look good, but the motivation/background would be nice to have (maybe link to some kernel/FW PR and/or discussion).

kv2019i · 2023-01-20T08:22:18Z

tools/topology/topology2/cavs-mixin-mixout-hda.conf

 Object.Dai {
 	HDA.0 {
 		name $HDA_ANALOG_DAI_NAME
+		dai_index 0


The commit leaves me a bit hanging on why these changes are made. The "This is in preparation" is information, but at least for me, this still opens questions. What it is the "instance" id, why we are adding one, how should I set it, etc.

+1, changes "in preparation for something in the future" are always difficult, if possible, such changes are easier to understand when submitted together with the actual use-case

@kv2019i @lyakh hopefully the commit messages give you more context now.

BTW I want to make this a rule in tplg2 to have an instance for every object. will be adding that in the compiler soon.

Thanks @ranj063 the expanded description is very helpful.

lgirdwood

@jsarha pls review

Add an instance attribute that will be used to instantiate DAI objects and make dai_index a mandatory attribute for all DAI objects types. This is in preparation to make the instance attribute default for all objects with topology2. This is required because the topology2 compiler allows for expansion of string values with variable definitions. So, if we wanted to expand the value of dai_index from a variable definition(shown as below), it is only possible if it were a normal attribute instead of the node ID as it is currently. Define { SSP_DAI_INDEX 3 } Object.Dai.SSP.1 { dai_index $SSP_DAI_INDEX } Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add an instance attribute and make the name attribute mandatory for the hw_config objects. This is required because the topology2 compiler allows for expansion of string values with variable definitions. So, if we wanted to expand the name from a variable definition(as shown below), it is only possible if it were a normal attribute instead of the node ID as it is currently. Define { SSP0_HW_CONFIG_NAME "SSP0" } Object.DAI.SSP.1 { dai_index 0 Object.Base.hw_config.1 { name $SSP0_HW_CONFIG_NAME } } Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lyakh · 2023-01-23T07:20:13Z

tools/topology/topology2/cavs-mixin-mixout-hda.conf


 Object.Dai {
 	HDA.0 {
 		name $HDA_ANALOG_DAI_NAME


"This is required because the topology2 compiler allows for expansion of string values with variable definitions." - did you mean "doesn't allow?" Same in the next commit. But how does it help to solve the issue with included topology2 files? You still need to use unique module / component names in them, don't you? And since you don't know where it's included from and what numbers are already taken you end up trying to guess some "really large number that nobody hopefully has taken yet"

@lyakh no I meant "allow". This PR is not meant to solve the issue with topology2 files but rather only amined at being able to allow substituting the dai_index for DAI objects and name for hw_config objects

ranj063 · 2023-01-23T17:32:10Z

SOFCI TEST

This was referenced Nov 17, 2022

[FEATURE] Clean-up topology2/cavs to make it user-friendly and maintainable #6576

Closed

topology2: cavs-sdw: add Speaker and Microphone pcm #6549

Merged

plbossart reviewed Nov 17, 2022

View reviewed changes

topology2: remove all pipeline stream_name setting

37865a6

It is not used at all. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/cavs_tplgs_part2 branch from 4fd7a25 to d5ce924 Compare January 19, 2023 18:38

ranj063 changed the title ~~[DO NOT MERGE] Topology clean ups - Part 2~~ Topology clean ups - Part 2 Jan 19, 2023

ranj063 marked this pull request as ready for review January 19, 2023 18:38

ranj063 requested review from dbaluta, lbetlej, lgirdwood and mmaka1 as code owners January 19, 2023 18:38

ranj063 requested review from juimonen, kv2019i and plbossart January 19, 2023 18:38

juimonen reviewed Jan 19, 2023

View reviewed changes

tools/topology/topology2/include/dais/hda.conf Outdated Show resolved Hide resolved

tools/topology/topology2/platform/intel/sdw-amp-generic.conf Outdated Show resolved Hide resolved

ranj063 force-pushed the fix/cavs_tplgs_part2 branch from d5ce924 to b56a22e Compare January 19, 2023 19:51

kv2019i approved these changes Jan 20, 2023

View reviewed changes

lgirdwood approved these changes Jan 20, 2023

View reviewed changes

ranj063 added 2 commits January 20, 2023 07:48

ranj063 force-pushed the fix/cavs_tplgs_part2 branch from 98f2137 to 476eabd Compare January 20, 2023 15:53

juimonen approved these changes Jan 20, 2023

View reviewed changes

lyakh reviewed Jan 23, 2023

View reviewed changes

kv2019i mentioned this pull request Jan 23, 2023

topology2: intel: add generic Bluetooth offload pipelines #6971

Merged

kv2019i merged commit 97374b6 into thesofproject:main Jan 23, 2023

ranj063 deleted the fix/cavs_tplgs_part2 branch January 23, 2023 19:47

Topology clean ups - Part 2 #6627

Topology clean ups - Part 2 #6627

Uh oh!

Conversation

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

Uh oh!

Uh oh!

plbossart 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!

Uh oh!

Uh oh!

kv2019i 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!

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!

ranj063 commented Jan 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ranj063 commented Nov 17, 2022 •

edited

Loading