Skip to content

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Feb 16, 2023

This looks like a lot of changes for one patch but in reality the changes are quite monotonous to use arrays in object declarations. This fixes the problem of the topology writer having to come up with obscenely large instance ID's for objects just to make them exclusive when including conf files programatically.

@ranj063
Copy link
Collaborator Author

ranj063 commented Feb 16, 2023

This looks like a lot of changes for one patch but in reality the changes are quite monotonous to use arrays in object declarations. This makes the problem os the topology writer having to come up with obscenely large instance ID's for objects just to make them exclusive when including confi files programatically.

I have verified that the generated binaries with this change are identical to the ones we have today except for these three files and I think these are because of some module names being different due to the change in instance IDs.
sof-tgl-rt711-rt1308-4ch.tplg
sof-tgl-rt711-rt1316-rt714.tplg
sof-tgl-rt715-rt711-rt1308-mono.tplg

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Seems much cleaner without the additional ids (that needed to be unique), thanks!

Where there more than 1 object of the same type defined at the same node
level, use arrays to define the object. For example, we can define 2
routes as follows:
Object.Base.route [
	{
		source	"smart_amp.2.1"
		sink	"copier.SSP.2.1"
	}
	{
		source	"mixin.1.1"
		sink	"mixout.2.1"
	}
]

This allows us to merge 2 arrays from different conf files without
needing to make their instance ID's unique.

Assume we add another route in a separate conf file like below:
Object.Base.route [
        {
                source  "gain.5.1"
                sink    "copier.5.1"
        }
]

The alsatplg compiler will merge them as follows resulting in 3 route
objects.

Object.Base.route [
        {
                source  "smart_amp.2.1"
                sink    "copier.SSP.2.1"
        }
        {
                source  "mixin.1.1"
                sink    "mixout.2.1"
        }
	{
                source  "gain.5.1"
                sink    "copier.5.1"
        }
]

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
These are already in the common includes.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Copy link
Contributor

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

All in all I think I understand the logic behind these changes, and yes the result look much cleaner.

stream_name 'SSP0 Playback'
}
}
passthrough-be [
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to these new pipeline object instantiations with number of pipeline arrays were hardest to follow. The new jumping indexes do not look the most logical choices, but then again I can see that they come from history. In the end the indexes do not matter for anything else but they are needed when referring to these objects from outside, do they?

Copy link
Collaborator Author

@ranj063 ranj063 Feb 16, 2023

Choose a reason for hiding this comment

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

yes, the index has no meaning in most cases except for the widget names. But I havent touched those obects within pipeline class definitions yet. That will be the next target after this one is merged.

@kv2019i
Copy link
Collaborator

kv2019i commented Feb 17, 2023

qemu-boot for icl/cnl fail and the mtl-zephyr-sparse are known issues, other checks pass so proceeding with merge.

@kv2019i kv2019i merged commit b70ae5f into thesofproject:main Feb 17, 2023
@ranj063 ranj063 deleted the fix/use_arrays branch February 17, 2023 07:28
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.

4 participants