topology2: Mod cfg ibs obs cpc addition #7348

jsarha · 2023-03-29T21:16:54Z

I am feeling quite clueless here, so even if this does not work, I push this out in hope someone can spot the problem.

For some reason the widget tuple array only contains the first instance of mod_cfg objects in the topology2 conf file. I am trying to read the array out with this code: https://github.com/jsarha/linux-sof/tree/peter-cpc-test

lgirdwood

@ranj063 pls review

lgirdwood · 2023-03-30T10:17:43Z

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

 			}
 			Object.Widget.gain.1 {
 				Object.Control.mixer.1 {
 					name '2 Main Playback Volume'
 				}
+				Object.Base.mod_cfg.1 {


Do we need to use the .1 here, is it possible to use ."48k2cS16LE" instead to describe the contents is a more human readable way ?

lgirdwood · 2023-03-30T10:18:12Z

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

+					obs 3145
+					cpc 3178
+				}
+				num_mod_cfgs 3


Do we need this, the CC should count this.

Ok. I was actually wondering why some widgets have explicitly set num_audio_formats and some doesn't.

some widgets support only 1 format, so they're set in teh class definition. Others support variable number of format based on tplg, so they're set in the top-level file

jsarha · 2023-03-30T13:02:30Z

@ranj063 can help me out here?

ranj063 · 2023-03-30T18:08:58Z

I am feeling quite clueless here, so even if this does not work, I push this out in hope someone can spot the problem.

For some reason the widget tuple array only contains the first instance of mod_cfg objects in the topology2 conf file. I am trying to read the array out with this code: https://github.com/jsarha/linux-sof/tree/peter-cpc-test

@jsarha the tplg is fine. but what you are missing is the parsing bits in the kernel. Take a look at how we parset multiple audio formats here: https://github.com/thesofproject/linux/blob/bef9852fe3228ce83717dada3ebcb7ad07c6099b/sound/soc/sof/topology.c#L1238

You need to allocate memory for "n" number of token sets first. And then parse the number of sets you're expecting like https://github.com/thesofproject/linux/blob/bef9852fe3228ce83717dada3ebcb7ad07c6099b/sound/soc/sof/ipc4-topology.c#L221 and then finally parse the tokens like https://github.com/thesofproject/linux/blob/bef9852fe3228ce83717dada3ebcb7ad07c6099b/sound/soc/sof/ipc4-topology.c#L257. Hope this helps

jsarha · 2023-03-31T12:53:00Z

This should now be good. This introduces the mod_cfg class for all relevant widgets. However, this pr does not set any mod_cfg:s as such.

lgirdwood

@jsarha can you give an example of how this would work per module instance for @ujfalusi to check. Thanks !

ujfalusi · 2023-04-04T13:30:46Z

@lgirdwood, I have my own variant of this (looks similar and I think @jsarha based this on my wip).
fwiw, this is how I set it for one module instance for testing:

diff --git a/tools/topology/topology2/include/pipelines/cavs/mixout-gain-dai-copier-playback.conf b/tools/topology/topology2/include/pipelines/cavs/mixout-gain-dai-copier-playback.conf
index 48a9d62938c2..ec2e31a72d76 100644
--- a/tools/topology/topology2/include/pipelines/cavs/mixout-gain-dai-copier-playback.conf
+++ b/tools/topology/topology2/include/pipelines/cavs/mixout-gain-dai-copier-playback.conf
@@ -17,6 +17,7 @@
 #
 
 <include/common/audio_format.conf>
+<include/common/mod_cfg.conf>
 <include/components/copier.conf>
 <include/components/gain.conf>
 <include/components/mixout.conf>
@@ -76,6 +77,7 @@ Class.Pipeline."mixout-gain-dai-copier-playback" {
 			num_audio_formats 2
 			num_input_audio_formats 2
 			num_output_audio_formats 2
+			num_mod_cfgs 3
 
 			# 32-bit 48KHz 2ch
 			Object.Base.audio_format.1 {
@@ -91,6 +93,24 @@ Class.Pipeline."mixout-gain-dai-copier-playback" {
 				out_bit_depth		32
 				out_valid_bit_depth	32
 			}
+
+			Object.Base.mod_cfg.1 {
+				ibs 192
+				obs 256
+				cpc 1456
+			}
+
+			Object.Base.mod_cfg.2 {
+				ibs 384
+				obs 384
+				cpc 4242
+			}
+
+			Object.Base.mod_cfg.3 {
+				ibs 432
+				obs 256
+				cpc 2566
+			}
 		}
 
 		pipeline."1" {

lgirdwood · 2023-04-05T12:13:52Z

@jsarha @ujfalusi @ranj063 we need to have a per HW target per module file for each module i.e.

cavs/tgl/copier.conf
ace/mtl/copier.conf

and so on that we would include in teh high level generic pipeline files.

jsarha · 2023-04-05T16:31:23Z

@jsarha @ujfalusi @ranj063 we need to have a per HW target per module file for each module i.e.
cavs/tgl/copier.conf
ace/mtl/copier.conf
and so on that we would include in teh high level generic pipeline files.

I still have some trouble in understanding the best ways to implement things in topology2, but I would like much more to have one place were we can define ibs/obs/cpc configurations for different platforms, than duplicating the module definitions for it. Could we have something like this:

Class.Widget."copier" {
#
# Pipeline ID for the copier object
#
DefineAttribute."index" {}

    IncludeByKey.PLATFORM {
     "mtl"	"platform/intel/mtl-copier-mod-cfgs.conf"
     "tgl"	"platform/intel/tgl-copier-mod-cfgs.conf"
    }

I have no idea if this would work, but it would be nice if it did 😅. @ranj063 can say if this approach could be made to work?

sys-pt1s · 2023-04-06T10:16:47Z

Can one of the admins verify this patch?

kv2019i · 2023-04-24T09:26:12Z

@jsarha Status? Please mark as draft if this is not intended for merging...

jsarha · 2023-04-24T10:09:31Z

@jsarha Status? Please mark as draft if this is not intended for merging...

This PR is good as such, but it does not contain the structures requested here: #7348 (comment)

They could be implemented as separate PR. But maybe I'll mark this as draft as you requested and try to put all the pieces together. Then again I have no idea of any mod_cfg settings for any platform, so in the end there will not be any settings in the later PR either, just places for different platforms to put them.

kv2019i · 2023-04-24T11:42:47Z

@jsarha So ack, so maybe we need to push this forward then. thesofproject/linux#4315 is close to merging on driver side, so adding @ujfalusi and @ranj063 to review.

jsarha · 2023-04-24T22:09:39Z

Actually, I am not so sure about the second commit in this series. Maybe its better to include the mod_cfg.conf already in the component files (e.g. in include/component/*.conf), and not in the pipeline definitions. I already have an alternative commit: ab8538c
@ranj063 , this is better, isn't it?

ranj063 · 2023-04-25T14:59:04Z

Actually, I am not so sure about the second commit in this series. Maybe its better to include the mod_cfg.conf already in the component files (e.g. in include/component/*.conf), and not in the pipeline definitions. I already have an alternative commit: ab8538c @ranj063 , this is better, isn't it?

@jsarha yes adding it to the widget class definitions where they will be instantaited and used is probably better

jsarha · 2023-04-25T19:58:53Z

New version pushed. This version should have what was requested in #7348 (comment) . Please check that I have added the placeholder and included it in actual component widgets. I already removed pipeline from them, but for many of them I have no idea what those entities are.

Add mod_cfg class for ibs, obs, and cpc configuration triplets, and num_mod_cfg to indicate the number of configurations. The num_mod_cfg attribute is added to common widget attributes. Signed-off-by: Jyri Sarha <[email protected]>

Add widget-common-inc.conf as a common place to add include files for all components. Unlike widget-common.conf, this is included before the class definition starts. The initial content for the widget-common-inc.conf is <include/common/mod_cfg.conf> for adding mod_cfg class definition before to be used as a member in all component files or in widget-common.conf. The commit also adds <include/components/widget-common-inc.conf> in the beginning of the all component includes. Signed-off-by: Jyri Sarha <[email protected]>

Adds empty include files for all modules and platforms to place their mod_cfg objects in and includes them in the relevant widget class files. Only thing found in the files is a comment telling what these files are for. Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2023-04-26T12:57:49Z

New version pushed. This version should have what was requested in #7348 (comment) . Please check that I have added the placeholder and included it in actual component widgets. I already removed pipeline from them, but for many of them I have no idea what those entities are.

Hmm. The last commit does not work. I do not know if there is a way to instantiate mod_cfg objects already in widget class definition.

jsarha · 2023-04-26T14:11:07Z

New version pushed. This version should have what was requested in #7348 (comment) . Please check that I have added the placeholder and included it in actual component widgets. I already removed pipeline from them, but for many of them I have no idea what those entities are.

Hmm. The last commit does not work. I do not know if there is a way to instantiate mod_cfg objects already in widget class definition.

Nope, this indeed does work. But only for targets that have PLATFROM variable set at compiler line. Generic targets, like sof-hda-generic.tplg, do not have any mod_cfg settings, but how could they, if the platform is not know at compile time. So, AFAICT this PR should be good.

ranj063 · 2023-04-26T15:55:01Z

@jsarha conflicts?

jsarha · 2023-04-26T16:00:36Z

@jsarha conflicts?

Solved now.

ranj063 · 2023-04-26T16:14:29Z

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

@@ -29,6 +29,12 @@ Class.Widget."asrc" {
 	#include common component definition
 	<include/components/widget-common.conf>

+	#include platform specific module configs for the asrc module


@jsarha is there value in this commit just yet? Can we not add the files as and when needed?

Sure we can. Just drop the last commit. It is all about interpreting this request #7348 (comment) .

Whether "Can we have?" is interpreted as if we can in theory or if the files should actually be added in practice. If it means in theory, then consider the last commit as a proof of concept.

Then again, adding them here and now would make it more likely that platform and component specific mod_cfgs files will be added in uniform way, following the same naming scheme for all the components.

@lgirdwood Can you ack as this was an ask from you? I.e. to have the placeholders or not.

I agree add on demand.

if want to add in this patch, I would suggest there is a folder to cover all component instead of directly under platform/intel/mod_cfg/

kv2019i

Not sure what is more likely, need to override values per platform, or per specific product, but in any case, this PR provides the necessary building blocks for both needs, so seems good.

kv2019i · 2023-04-27T09:08:03Z

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

@@ -29,6 +29,12 @@ Class.Widget."asrc" {
 	#include common component definition
 	<include/components/widget-common.conf>

+	#include platform specific module configs for the asrc module


@lgirdwood Can you ack as this was an ask from you? I.e. to have the placeholders or not.

jsarha · 2023-05-03T16:39:49Z

Pushed forwards, and changes requested in mail, so turning to draft again.

Girdwood, Liam R:

Jyri, a topology override must copy the manifest CPC schema exactly.
Even the unused fields must be copied and populated with 0 today.

lgirdwood · 2023-07-04T15:03:15Z

@jsarha now that the manifest and kernel CPC parts are working we should add the overide option for v2.7.
@ujfalusi @plbossart fyi.

plbossart · 2023-07-04T15:25:41Z

@jsarha now that the manifest and kernel CPC parts are working we should add the overide option for v2.7. @ujfalusi @plbossart fyi.

We need to agree on the definition of 'working'. I still routinely see messages telling me that CPC is not set ?

lgirdwood · 2023-07-05T08:27:25Z

@jsarha now that the manifest and kernel CPC parts are working we should add the overide option for v2.7. @ujfalusi @plbossart fyi.

We need to agree on the definition of 'working'. I still routinely see messages telling me that CPC is not set ?

There are still a lot of holes in the manifest CPC data for each module and (format, rate, channels) configuration. @aiChaoSONG will be working on populating this prior to v2.7. This should fix the kernel warning messages for all tested configs.

The toplogy part will use the same CPC schema and kernel logic/IPC, but will allow

Adding a missing CPC data element if one is missing in manifest for a new configuration.
Overide an existing manifest CPC data element if needed.

plbossart · 2023-07-05T08:30:49Z

I'd like to reach the point where all CPC data is valid in the manifest and used for something meaningful, before we plan work on overriding stuff.

lgirdwood · 2023-07-05T12:31:05Z

I'd like to reach the point where all CPC data is valid in the manifest and used for something meaningful, before we plan work on overriding stuff.

Need to do both in parallel, to be ready for mistakes in manifest CPC.

plbossart · 2023-07-05T12:58:36Z

I'd like to reach the point where all CPC data is valid in the manifest and used for something meaningful, before we plan work on overriding stuff.

Need to do both in parallel, to be ready for mistakes in manifest CPC.

we can't afford to hedge our bets for every single feature. The firmware team is responsible for providing correct CPC values, if they don't it's a bug and they fix it.
We have lots of other things to do really with SoundWire-related enablement.

lgirdwood · 2023-07-05T16:08:04Z

I'd like to reach the point where all CPC data is valid in the manifest and used for something meaningful, before we plan work on overriding stuff.

Need to do both in parallel, to be ready for mistakes in manifest CPC.

we can't afford to hedge our bets for every single feature. The firmware team is responsible for providing correct CPC values, if they don't it's a bug and they fix it. We have lots of other things to do really with SoundWire-related enablement.

CPC architecture is based on a single module unit test with no upstream/downstream module I$/D$ impact taken into consideration for module CPC performance delta when different pipeline topologies are used. This is not a big item and will be painful to modify/fix in manifest.

kv2019i · 2023-11-21T19:25:38Z

In draft status, pushing to v2.9.

btian1 · 2023-11-22T01:32:43Z

tools/topology/topology2/include/common/mod_cfg.conf

+# Object.Base.mod_cfg."0" {
+#	ibs	384
+#	obs	384
+#	cpc	4600


can we add comments here:
for ibs/obs, it is initialized as: 48 * 2 * 4 = 384, it is 48k LL with 2 channel 32 bit format.
for cpc, I am not sure how CPC figured out as instantiated value?

btian1 · 2023-11-22T01:34:58Z

tools/topology/topology2/include/common/mod_cfg.conf

+    DefineAttribute."instance" {
+    }
+
+    # input buffer size


let's give comments like:
ibs = (sample rate / (1000000/period)) * channels * valid_sample_width

btian1 · 2023-11-22T01:40:22Z

tools/topology/topology2/include/components/widget-common-inc.conf

+# This file is for include files needed for widget-common.conf
+#
+<include/common/mod_cfg.conf>
+


why not directly include mod_cfg.conf?

btian1 · 2023-11-22T01:50:38Z

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

@@ -29,6 +29,12 @@ Class.Widget."asrc" {
 	#include common component definition
 	<include/components/widget-common.conf>

+	#include platform specific module configs for the asrc module


I agree add on demand.

if want to add in this patch, I would suggest there is a folder to cover all component instead of directly under platform/intel/mod_cfg/

kv2019i · 2024-03-04T13:06:40Z

Stable-v2.9 branched, this didn't make the cut, bumping forward. @jsarha is this still valid (given bumps over multiple releases)?

kv2019i · 2024-04-24T10:54:28Z

No activity in 2.7...2.9, clearing the milestone.

lgirdwood reviewed Mar 30, 2023

View reviewed changes

jsarha force-pushed the mod-conf-ibs-obs-cpc branch from e8ca6ee to 8c7b123 Compare March 31, 2023 12:49

jsarha changed the title ~~topology2: Mod cfg ibs obs cpc addition and a test commit to test it DOES NOT WORK~~ topology2: Mod cfg ibs obs cpc addition Mar 31, 2023

jsarha force-pushed the mod-conf-ibs-obs-cpc branch from 8c7b123 to 5696d10 Compare March 31, 2023 12:51

jsarha marked this pull request as ready for review March 31, 2023 12:53

ujfalusi mentioned this pull request Apr 3, 2023

ASoC: SOF: ipc4: Fetch CPC from tplg mod_cfg or from manifest thesofproject/linux#4262

Closed

lgirdwood reviewed Apr 4, 2023

View reviewed changes

jsarha closed this Apr 5, 2023

jsarha reopened this Apr 5, 2023

kv2019i requested review from ranj063, ujfalusi and abonislawski April 24, 2023 11:41

jsarha force-pushed the mod-conf-ibs-obs-cpc branch from 5696d10 to 5b6ef5b Compare April 25, 2023 19:55

Jyri Sarha added 3 commits April 26, 2023 12:16

jsarha force-pushed the mod-conf-ibs-obs-cpc branch from 5b6ef5b to c3bd46e Compare April 26, 2023 16:00

ranj063 reviewed Apr 26, 2023

View reviewed changes

kv2019i approved these changes Apr 27, 2023

View reviewed changes

jsarha marked this pull request as draft May 3, 2023 16:37

lgirdwood added this to the v2.7 milestone Jul 4, 2023

alex-cri modified the milestones: v2.7, v2.8 Aug 22, 2023

kv2019i modified the milestones: v2.8, v2.9 Nov 21, 2023

btian1 reviewed Nov 22, 2023

View reviewed changes

kv2019i modified the milestones: v2.9, v2.10 Mar 4, 2024

kv2019i removed this from the v2.10 milestone Apr 24, 2024

topology2: Mod cfg ibs obs cpc addition #7348

Are you sure you want to change the base?

topology2: Mod cfg ibs obs cpc addition #7348

Conversation

jsarha commented Mar 29, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Mar 30, 2023

ranj063 commented Mar 30, 2023

jsarha commented Mar 31, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

ujfalusi commented Apr 4, 2023

lgirdwood commented Apr 5, 2023

jsarha commented Apr 5, 2023 • edited Loading

sys-pt1s commented Apr 6, 2023

kv2019i commented Apr 24, 2023

jsarha commented Apr 24, 2023

kv2019i commented Apr 24, 2023

jsarha commented Apr 24, 2023

ranj063 commented Apr 25, 2023

jsarha commented Apr 25, 2023

jsarha commented Apr 26, 2023

jsarha commented Apr 26, 2023

ranj063 commented Apr 26, 2023

jsarha commented Apr 26, 2023

Choose a reason for hiding this comment

jsarha Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented May 3, 2023

lgirdwood commented Jul 4, 2023

plbossart commented Jul 4, 2023

lgirdwood commented Jul 5, 2023

plbossart commented Jul 5, 2023

lgirdwood commented Jul 5, 2023

plbossart commented Jul 5, 2023

lgirdwood commented Jul 5, 2023

kv2019i commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Mar 4, 2024

kv2019i commented Apr 24, 2024

jsarha commented Apr 5, 2023 •

edited

Loading

jsarha Apr 26, 2023 •

edited

Loading