[RFC] Demux tokens #1800

juimonen · 2020-02-18T12:32:32Z

This is on top of #1766 to show how to parse demux tokens with multiple var arrays. You can do it, but t is quite painful and we should really somehow get rid of var arrays in ipc...
FW counterpart thesofproject/sof#2345

Optimize the parsing so that it will stop after all required tokens have been found as there is no reason to continue after that. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

Widget's parameters consist of several different types of tuple arrays like strings, words and bytes. Lately we've been seeing more complex widget definitions with multiple identical sets of tuple arrays. These include for example dmic pdm configuration and mux/demux channel map definitions. You could compare this to an array of structs of the same type in C. So add a new function sof_parse_token_sets, which can be used to parse multiple token sets. This function defines the number of sets and an offset to copy the tokens to correct positions in the destination ipc struct. Old sof_parse_token will be a special case of calling sof_parse_token_sets to parse 1 set with offset 0. Modify the dmic dai link loading to use sof_parse_array_sets to load multiple pdm configs. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

ranj063 · 2020-02-20T15:16:24Z

sound/soc/sof/topology.c

sizeof(*sm)?

I'm not sure about the customs with sizeof, but sure can change

This can be written easier as

u8 *pos = (u8 *)(sm + 1);

or

u8 *pos = (u8 *)&sm[1];

whatever you prefer

ranj063 · 2020-02-20T15:17:32Z

sound/soc/sof/topology.c

I think we can remove cm_temp_p and just use cm_temp + i * sizeof(*cm_temp) here?

hmm ok. it might get little bit messier, but yeah I'll try it.

that would mean a multiplication on each loop iteration :-) Yes, that's micro-optimisation and we have huge beefy CPUs, but how about something like

for (i = 0, cm_temp_p = cm_temp; i < sm->num_ch_map; i++, cm_temp_p++)

yes, it is just my personal preference to not include multiple "end statement" to for loops, I personally many times forget to see that and wonder who is updating some variable. And maybe even more stylistic preference for myself...

ranj063 · 2020-02-20T15:23:36Z

sound/soc/sof/topology.c

offset doesnt seem to be used past this point, why do we need to update it?

offset is coming as argument so that the calling function would know how much data has been written. the num_coeffs is updated in later loop and doesn't reflect the total amount of coeffs in the end of function.. so fun stuff with var arrays

yes, but its not used even in the caller after sof_process_load_mux() is called

aa ok. Let's say it could be used... :) I'm still wondering what to do if we have data in related control and/or tuples. Are they mutually exclusive? If you come up with idea would be nice... So far I assumed that it is either or. We could also copy the data after tuples, but don't know if it makes too much sense...

@ranj063 so generally we have an issue with this kind of var array of tuples that the "size" coming from asoc topology is much bigger than the actual size we need to send to dsp (tuples arrays take almost twice the space as C ipc structs). So we should probably change the process component creation to handle this (some kind of temp parsing area also,,,huh ugly)

ranj063 · 2020-02-20T16:56:37Z

sound/soc/sof/topology.c

@juimonen this part is confusing me a bit. for each ch_map in num_ch_map, there are a set of coefficients right? So is the expectation that the coefficients are in order for each ch_map ?

well so far there's no "id" or anything for coeffs, as they are as var array. So yes here it is assumed that coeffs are in order, so belonging to the ch_maps (in order). So with my modified parsing, I can parse all at the same time, or I could use inner loop. It doesn't change the problematics of copying them eventually to correct place in ipc struct.

@juimonen this makes me a bit uncomfortable. What we're talking about is a set of ch_map tokens, each a with variable number of coefficients. Do the coefficients have to be a token array or can they be data with matching ch_map id?

So I'm not sure we can mix data and tokens here, I think I tried it sometimes and I mean our parser will go berserk... So I think it is either token arrays or data (or data is in the end and you have to come up with a position to jump there). So as I understand the upper struct has the amount of coeffs, and that variable amount of coeffs is following the ch_map. I assume that first coeffs belong to first ch_map, second to second and so on... Anyway if we are getting rid of var arrays this has to change

@juimonen token arrays are fine but we need a way to match the coefficients with the ch_map_id. I dont think the assumption that the topology will always be perfect is valid. So we parse the ch_map_tokens first and then the ch_map_coeff_tokens for each ch_map id. This will complicate the parsing function a bit but guess what the ch_map_id field in each coeff will give us the offset to where to copy them. makes sense?

hmm I'm not sure how to do that..? doesn't this also assume that the id of the coeff is somewhere close to it (just before it) in arrays? So not sure this will make it more robust than now... or am I missing something?

what I meant was you'll need to extend the coeff tokens to include also the ch_map_id apart from the actual coeff itself

I dont think the assumption that the topology will always be perfect is valid.

@ranj063 could you explain a bit, please? Is this because those tokens should be manually created and are particularly error prone? There can be many errors in topology files, I think. I would actually even (almost) bet, that we already have some - no software is bug free. So we can only build in checks to make sure we don't crash the kernel and otherwise we have no choice but rely on the data, that we're fed?

lyakh

this doesn't seem to be "on top of #1766" but it includes #1766 and an older version of that at that. I'm actually not sure if it's possible to make a PR on top of another PR, that hasn't been merged yet, without including it. If all these patches are related, just make it one PR (and close the other abandoned one), but please use the newest version!

lyakh · 2020-02-20T18:28:38Z

sound/soc/sof/topology.c

This can be written easier as

u8 *pos = (u8 *)(sm + 1);

or

u8 *pos = (u8 *)&sm[1];

whatever you prefer

lyakh · 2020-02-20T18:39:30Z

sound/soc/sof/topology.c

oh, why are you forcing the software (and presumably much slower) version of hweight? Don't our CPUs have the popcntl instruction? I think the best is to use long_hweight() - it'll figure out the optimal one and the compiler will optimise all the auxiliary logic out.

ok, this is totally new function for me, was trying to search for example with slim results,
definitely will change to your suggestion.

lyakh · 2020-02-20T18:54:50Z

sound/soc/sof/topology.c

I dont think the assumption that the topology will always be perfect is valid.

@ranj063 could you explain a bit, please? Is this because those tokens should be manually created and are particularly error prone? There can be many errors in topology files, I think. I would actually even (almost) bet, that we already have some - no software is bug free. So we can only build in checks to make sure we don't crash the kernel and otherwise we have no choice but rely on the data, that we're fed?

lyakh · 2020-02-20T18:58:19Z

sound/soc/sof/topology.c

lyakh · 2020-02-20T19:01:44Z

sound/soc/sof/topology.c

that would mean a multiplication on each loop iteration :-) Yes, that's micro-optimisation and we have huge beefy CPUs, but how about something like

for (i = 0, cm_temp_p = cm_temp; i < sm->num_ch_map; i++, cm_temp_p++)

plbossart · 2020-02-20T20:48:21Z

@juimonen Can we pretty please get an example of what those tokens are and why we need something different? Do you have a topology file example that we can look at? It's really difficult to review all this....

Take a very simple example of a tee, with duplication of data on two outputs.

juimonen · 2020-02-21T08:06:33Z

@plbossart I already once included the related topology in the previous closed PR...so I really try to do as you request here. thesofproject/sof#2345 has the topology change related to this PR (as mentioned in the title), but I'll attach the conf file here to see.

This PR is just to show how to parse the var arrays, and as you can see it is really pita. So it is not doing anything differently just for fun, we have not ever parsed topology with var arrays (which themselves include var arrays). So this is a stream_map including var amount of ch_maps, which include var amount of coeffs.

If we are changing the ch_map and others to not include var arrays, this PR can be used just as example, so it will have to change anyway.

sof-apl-demux-pcm512x.txt

juimonen · 2020-02-21T08:13:10Z

@lyakh yes sorry, I was refining the PR which is below this (as I think it is more important) and didn't rebase this. will update..

juimonen · 2020-02-21T08:16:37Z

@lyakh generally I don't know what is the preferred way if you have several PR's which depend on each other...? I usually include the previous to the current as it is easier for someone to test and the following PR might not even compile.

There's no really easy way for me to show (for example to @plbossart) why I need some PR, unless I have follow up PR on top of it to show that, well this is the reason..

Currently sof process (dapm effect type) components have only binary blob parameter parsing. Change sof mux/demux component to use tuple arrays so that the parameters are easier to set in topology. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

plbossart · 2020-02-22T00:57:48Z

@plbossart I already once included the related topology in the previous closed PR...so I really try to do as you request here. thesofproject/sof#2345 has the topology change related to this PR (as mentioned in the title), but I'll attach the conf file here to see.

I am at a loss as to what this means:

SectionVendorTuples."MUXDEMUX1.0_mux_data_tuples_w" {
	tokens "sof_chmap_tokens"
	tuples."word" {
		SOF_TKN_STREAM_MAP_NUM_CH_MAPS 		"2"
	}
		tuples."word.map_1" {
		SOF_TKN_CHANNEL_MAP_INDEX		"0"
		SOF_TKN_CHANNEL_MAP_EXT_ID		"1"
		SOF_TKN_CHANNEL_MAP_MASK		"5"
	}
		tuples."word.1" {
	SOF_TKN_CHANNEL_MAP_COEFF		"34954"
	}
   	tuples."word.2" {
	SOF_TKN_CHANNEL_MAP_COEFF		"87343"
	}
   

			tuples."word.map_2" {
		SOF_TKN_CHANNEL_MAP_INDEX		"1"
		SOF_TKN_CHANNEL_MAP_EXT_ID		"5"
		SOF_TKN_CHANNEL_MAP_MASK		"5"
	}
		tuples."word.3" {
	SOF_TKN_CHANNEL_MAP_COEFF		"12345"
	}
   	tuples."word.4" {
	SOF_TKN_CHANNEL_MAP_COEFF		"54321"
	}		
}

you have two maps and 4 coefficients, but I can't figure out how you might relate the maps and coefficients together...

plbossart · 2020-04-15T22:53:09Z

@juimonen is this PR still relevant? we've merge another PR for token parsing.

juimonen · 2020-04-16T05:41:25Z

@plbossart the way to go is still in haze (blobs vs. tokens) as the process/effect handling generally is in flux. So let's close this, it's easy to come back to this if needed...

ASoC: SOF: topology: stop parsing when all tokens have been found

f0439ec

Optimize the parsing so that it will stop after all required tokens have been found as there is no reason to continue after that. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

juimonen requested review from bardliao, keyonjie, lyakh, ranj063 and singalsu as code owners February 18, 2020 12:32

juimonen mentioned this pull request Feb 18, 2020

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

Closed

ranj063 reviewed Feb 20, 2020

View reviewed changes

lyakh requested changes Feb 20, 2020

View reviewed changes

juimonen force-pushed the demux_tokens branch from 518bdcb to de78ae4 Compare February 21, 2020 08:58

juimonen force-pushed the demux_tokens branch from de78ae4 to 8bbd29a Compare February 21, 2020 11:58

juimonen closed this Apr 16, 2020

[RFC] Demux tokens #1800

[RFC] Demux tokens #1800

Uh oh!

Conversation

juimonen commented Feb 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

plbossart commented Feb 20, 2020

Uh oh!

juimonen commented Feb 21, 2020

Uh oh!

juimonen commented Feb 21, 2020

Uh oh!

juimonen commented Feb 21, 2020

Uh oh!

plbossart commented Feb 22, 2020

Uh oh!

plbossart commented Apr 15, 2020

Uh oh!

juimonen commented Apr 16, 2020

ranj063 Feb 20, 2020 •

edited

Loading