[RFC] topology: set mux/demux parameters with tuples #2345

juimonen · 2020-01-31T12:50:55Z

Currently all process (effect) component parameters are set with binary
blobs. This makes sense with eq's and such where parameters are highly
varying in size and readability. So make sof mux/demux component to use
tuples as an example of a component having complex parameter structure.
This makes it easier to setup new mux parameters from topology.

Signed-off-by: Jaska Uimonen jaska.uimonen@linux.intel.com

juimonen · 2020-01-31T12:56:09Z

driver counterpart: thesofproject/linux#1758

lgirdwood

Good find, btw how do we deal with backwards compatibility ?

juimonen · 2020-02-01T06:23:37Z

@lgirdwood If I understand my own code correctly (which is not the case always), this PR and and driver counterpart should be "transparent" to old binary blob and new tuples way at least with demux. So kernel is parsing the tuples but sending it as binary blob to FW. So currently there isn't a particular ipc struct for mux, it is still sent with process (effect) ipc struct.

lgirdwood · 2020-02-03T10:03:13Z

@juimonen ok. CI also looks good.
@ranj063 @jajanusz any inputs ?

juimonen · 2020-02-03T11:01:42Z

@lgirdwood let's keep this still as RFC for a while...I'd like yours and others comments about the current mux/demux structs, which would be replicated in kernel side... There are now arrays, of which the size is varying depending on platform. Also there are uint8_t for channels and uint32_t for pipeline_id's. Are we going to have like 2 million pipelines?. In kernel/driver side we have ready parsing mostly for uint16 and uint32. So could we just fix the array sizes with max channel count and streamline the variable sizes to uint16 and uin32?

lgirdwood · 2020-02-03T16:00:09Z

@juimonen pipe and comp id needs the headroom afforded by 32bits, since we will be using the large size to partition the DSP into logical zones, say for different VM guests, safety critical, multi clients, etc

ranj063 · 2020-02-03T16:36:08Z

@juimonen sorry it took me a while to understand the problem here. The general direction is correct here but there's a more elegant way to add the mux_stream_data_1/2. Please take a look at the PDM_CONFIG() macro used for DMIC_CONFIG. If you look at the conf file, what you'll see is that you have 2 sets of PDM tuples as below. Can you please follow that example?

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"
        }

        tuples."short.pdm1" {
                SOF_TKN_INTEL_DMIC_PDM_CTRL_ID          "1"
                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"
        }


}

juimonen · 2020-02-04T07:53:38Z

@ranj063 yes if you look at muxdemux.m4 in this PR, at least to me it looks exactly like the method used in dmic, so using list to add variable amount of tuples arrays. Did you meant something else?

@lgirdwood yes the 32 bit pipeline_id is fine, I meant streamlining the struct so that other members are also uin16 or uint32 so that parsing is easier and we avoid most packing issues. I don't think we help the ipc by defining all types as small as possible? Also I don't like the variable array length based on platform, will look really ugly if exported to kernel side...

juimonen · 2020-02-04T10:51:54Z

@lgirdwood ok, seems that the demux interface is about to change with other current sof PR's, so my rant about the types in demux structs can be ignored. I can modify this PR when the new interface is clear so we would have good topology support.

ranj063 · 2020-02-04T16:43:51Z

@ranj063 yes if you look at muxdemux.m4 in this PR, at least to me it looks exactly like the method used in dmic, so using list to add variable amount of tuples arrays. Did you meant something else?

@juimonen yes your PR does something similar to the DMIC but in the case of DMIC, you can clearly tell that there are 2 sets of PDM tuple arrays because they are right next to each other with pdm0 and pdm1. Whereas in your case, its hard to tell if there one or more.

juimonen · 2020-02-04T19:06:51Z

@ranj063 yes as I understand how the mux/demux works, you can have 1-8 stream configs so variable amount. It should be clear from sof-demux-pcm521x that we are using 2 arrays for this case. Compared to dmic, the arrays are just defined 1 level up, as it can be something between 1-8 (or actually varying from 1-4 or 1-8 depending on platform) depending how you are connecting the mux.

Currently all process (effect) component parameters are set with binary blobs. This makes sense with eq's and such where parameters are highly varying in size and readability. So make sof mux/demux component to use tuples as an example of a component having complex parameter structure. This makes it easier to setup new mux parameters from topology. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

juimonen · 2020-02-18T12:34:07Z

updated to use channel map, driver counterpart thesofproject/linux#1800
some channel map values are bogus, but you can see how to use tokens for this...and from driver side PR you can see how painful it is to parse these in kernel.

lgirdwood · 2020-02-19T10:49:15Z

@ranj063 @bkokoszx any comments on this ?

juimonen · 2020-02-19T13:07:08Z

@lgirdwood I think @plbossart has an effort to change all ipc structs with var arrays to non-var arrays... So not sure what will happen to the channel_map struct used here. I wanted to made this PR to show how to do the multiple arrays with topology as this is even more complicated as dmic pdm.

Anyway if @ranj063 checks this out we could polish the topology parts. Also the bit mask value in channel_map and the Q-format coeffs make the topology still quite "mysterious" as there's no very nice way of describing them.

lgirdwood · 2020-02-19T14:51:42Z

@juimonen lets collate all these IPC problems and add to the feature request for IPC2.0. This will allow us to track all the opens we have and align on how to fix.

ranj063 · 2020-02-20T15:30:40Z

tools/topology/m4/muxdemux.m4

 define(`N_MUXDEMUX', `MUXDEMUX'PIPELINE_ID`.'$1)

-dnl W_MUXDEMUX(name, mux/demux, format, periods_sink, periods_source, kcontrol_list)
+dnl CH_MAP_COEFF(coeff)


ranj063 · 2020-02-20T15:31:19Z

tools/topology/m4/muxdemux.m4

+
+dnl CH_MAP(name, id, ext_id, mask)
+define(`CH_MAP',
+`	tuples."word.$1" {'


did you mean $1 here? or $2?

so as I understand the $1 is like "name". So it should be the same "name" meaning in both CH_MAP and CHMAP_COEFF as we have actual "id" token member in the other... goes quite confusing otherwise. I'm not totally sure what is actually the meaning of this "word.xxx" syntax, can we actually parse/see that in driver/kernel side? So I copied this from your dmic pdm...

ranj063 · 2020-02-20T17:07:45Z

tools/topology/sof/tokens.m4

+	SOF_TKN_CHANNEL_MAP_INDEX		"1401"
+	SOF_TKN_CHANNEL_MAP_EXT_ID		"1402"
+	SOF_TKN_CHANNEL_MAP_MASK		"1403"
+	SOF_TKN_CHANNEL_MAP_COEFF		"1404"


I am confused by this too. This suggests that sof_ch_map_tokens contains exactly one channel_map_coeff but there could be multiple ones no?

well it is just the token id for coeff. Yes there can be var array of coeffs in var array of ch_maps (which are in stream map struct). I made an example in sof side with 2 ch_maps with 2 coeffs each...

I will also split the coeff into a separate set of tokens just like you've done in your linux PR

sorry I didn't get what you mean here..?

Can you comment the tokens too - we should really be doing this for all tokens.

ranj063 · 2020-02-20T17:08:49Z

tools/topology/m4/muxdemux.m4

+
+dnl CH_MAP(name, id, ext_id, mask)
+define(`CH_MAP',
+`	tuples."word.$1" {'


missing tokens "sof_chmap_tokens"?

it is included in _mux_data_tuples_w which has the tokens "sof_chmap_tokens"... should I do it somehow differently?

ranj063 · 2020-02-20T17:20:17Z

tools/topology/sof/tokens.m4

 }
+
+SectionVendorTokens."sof_chmap_tokens" {
+	SOF_TKN_STREAM_MAP_NUM_CH_MAPS		"1400"


this has to be split into a separate token maybe called sof_mux_demux_token

I mean sure... So what is the reasoning for this? It could be that the channel maps etc are used elsewhere also than demux only. I mean maybe it would be somehow more clear, but basically we are defining the "parse unit" in the kernel side tuples array definitions, so that could easily combine tuple id's all over and no one would notice...

paulstelian97 · 2021-03-29T09:05:26Z

Please resubmit with "main" as PR base branch.

juimonen requested review from dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners January 31, 2020 12:50

juimonen mentioned this pull request Jan 31, 2020

[RFC] Demux tokens thesofproject/linux#1758

Closed

lgirdwood reviewed Jan 31, 2020

View reviewed changes

lgirdwood approved these changes Feb 3, 2020

View reviewed changes

juimonen mentioned this pull request Feb 4, 2020

mux: using channel map as mux configuration #2212

Closed

juimonen force-pushed the demux_tokens branch from 46125a8 to bb3a66e Compare February 18, 2020 12:28

juimonen mentioned this pull request Feb 18, 2020

[RFC] Demux tokens thesofproject/linux#1800

Closed

ranj063 reviewed Feb 20, 2020

View reviewed changes

lgirdwood closed this Mar 28, 2021

lgirdwood deleted the branch thesofproject:master March 28, 2021 13:44

[RFC] topology: set mux/demux parameters with tuples #2345

[RFC] topology: set mux/demux parameters with tuples #2345

Uh oh!

Conversation

juimonen commented Jan 31, 2020

Uh oh!

juimonen commented Jan 31, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

juimonen commented Feb 1, 2020

Uh oh!

lgirdwood commented Feb 3, 2020

Uh oh!

juimonen commented Feb 3, 2020

Uh oh!

lgirdwood commented Feb 3, 2020

Uh oh!

ranj063 commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juimonen commented Feb 4, 2020

Uh oh!

juimonen commented Feb 4, 2020

Uh oh!

ranj063 commented Feb 4, 2020

Uh oh!

juimonen commented Feb 4, 2020

Uh oh!

juimonen commented Feb 18, 2020

Uh oh!

lgirdwood commented Feb 19, 2020

Uh oh!

juimonen commented Feb 19, 2020

Uh oh!

lgirdwood commented Feb 19, 2020

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!

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!

paulstelian97 commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ranj063 commented Feb 3, 2020 •

edited

Loading