[RFC] Demux tokens #1758

juimonen · 2020-01-31T12:52:10Z

In these 2 commits I'm trying to show/demo how you can parse more complex tuple arrays, example being mux/demux.

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

ranj063 · 2020-01-31T17:31:34Z

@juimonen Could you please paste the conf file for an example tplg that we're trying to parse here?I am having a hard time understanding the problem without it

plbossart

this is quite hard to review for someone who hasn't spent hours on the topology. You probably want to explain things a bit more and that feature you are enabling, or what problem you are fixing. I really don't know if you are dealing with a bug or not.

plbossart · 2020-01-31T17:57:53Z

sound/soc/sof/topology.c

why would we want to list out everything here? this doesn't scale. Just test for mux/demux and break in the default case (all others).

...implicit default, since type isn't an enum, no explicit default case is really needed.

ok, removed listing of all existing components and only mux/demux remains.

plbossart · 2020-01-31T17:59:12Z

sound/soc/sof/topology.c

note on comment half a page above "configure iir IPC message" is probably just wrong/obsolete?

yes some oldie, has to be fixed in separate pr was already before this PR...

plbossart · 2020-01-31T18:00:19Z

sound/soc/sof/topology.c

it's really horrible to mix position and error status, sorry.

e.g. here ret could be zero, so the position is NOT updated...

Don't do it, it's just error prone and difficult to maintain.

ok, yes this is really good comment, I need to pass the position as parameter.

fixed now. There's read_bytes argument given to parse_tokens now and return value is just what it was before.

plbossart · 2020-01-31T18:02:23Z

sound/soc/sof/topology.c

I have idea what 'multiple same type arrays' are?

As opposed to single same type array? or multiple different type arrays?

If you have 2 similar tuples arrays in one component's topology, current parser will always parse the last one. So it will parse the 1st one, but continue and parse the 2nd on top of the 1st. Mux is an example of this kind of component, because it may contain multiple stream config arrays.

I'm not sure if we currently any other component with this kind of structure, but we may in the future. Also I'm not aware any other way how this should be handled in alsa topology, is it forbidden to define multiple similar tuples arrays for one component? At least currently the alsa tools are fine with it.

can you give an example of what 'similar tuple arrays in one components's topology' mean?

These are really vague concepts for me, and I am not sure why this is exposed by the mux/demux support?

@plbossart @juimonen If I understand it correctly, I think the problem is similar to what we've done for the PDM controller config in the topology for DMIC config already. We're looking at something like below where we have multiple sets of tokens.

thesofproject/sof#2345 (comment)

plbossart · 2020-01-31T18:03:19Z

sound/soc/sof/topology.c

if there a range check on mc->streams? Or could a bad topology break things here?

AFAIK this is parsed against total data size and tuples array sizes. So we should not go beyond the data area without error...

plbossart · 2020-01-31T18:05:37Z

include/sound/sof/topology.h

I don't think we have u8 in IPC definitions? Are you sure we're not going to have endianness issues or other shenanigangs?

no I'm not. Actually the 8 and 3 in the mask and reserved are also dubious as we have variable size for those in FW depending on platform (for e.g. 8 for apl, 4 for byt). So actually I would like us also to rationalize the mux/demux structs, if they are made public in kernel, so that it would be same for all platforms and have some sane types. I mean pipeline_id is uint32 and channels is uint8? Are we going to have 2 million pipelines? So why don't just set everything to uint16 and fix the max channels to 8?

I think the space save we get from these hacks is minimal. So this is more for mux/demux component developer, but as these structs could be mirrored in kernel, they should take into account other issues as minimal size also...

plbossart · 2020-01-31T18:06:10Z

include/sound/sof/topology.h

what does the mask represent?

so these are copied from FW and should be explained there. As I understand it, it is a channel mask for this stream, so which stream and channel is copied and where? @akloniex any comments?

plbossart · 2020-01-31T18:16:53Z

sound/soc/sof/topology.c

doesnt' look like any of the routines below actually use the position for anything, they just test the status. Why is this needed? or what I am missing?

the mux/demux parsing uses it. nobody else doesn't currently.

plbossart · 2020-01-31T18:19:24Z

sound/soc/sof/topology.c

comment on commit message: I have absolutely no idea if you describe a data corruption issue ("copied on top"), a bad list parsing (returns last instead of first).

What are 'multiple similar tuple arrays'? why is this useful?
What are you trying to fix or enable really?

Yeah, sorry for being too sparse. I tried to explain this above also... so I don't think currently the parser handles multiple similar tuples arrays following each other in topology. Every array is parsed but copied on top of the previous one. And you can't do that easily as parser doesn't tell that it has found something or the position of the found array. So our parser will always run to the end of given data section despite it found 1,2,3,4 instances of the array and the last one of those is what you get. AFAIK currently there is just no way of parsing the array number 1,2,3 as the parser will run to the end of data.

As said we've not had this kind of situation, because our components don't currently have this kind of multiple array structure. Mux/demux is the first one with this kind of more complex structure.

juimonen · 2020-02-01T07:15:57Z

@ranj063 check the demux part in following conf file...
sof-apl-demux-pcm512x.txt

sound/soc/sof/topology.c

lyakh · 2020-02-03T07:52:58Z

sound/soc/sof/topology.c

...implicit default, since type isn't an enum, no explicit default case is really needed.

sound/soc/sof/topology.c

juimonen · 2020-02-03T09:37:51Z

@plbossart @ranj063 @lyakh updated. Actually I want to achieve the following things:

sof_parse_tokens should return when it has found all tokens (found == count)
sof_parse_tokens should return the current parse position
I'm using the above to parse multiple similar tuples arrays in mux/demux

final observation: I don't get why we are checking (ret != 0) everywhere with sof_parse_tokens? I don't see it could return positive values..

ranj063 · 2020-02-03T16:28:32Z

@juimonen I am still trying to understand what the actual problem is. So please bear with my stupid questions here.

Looking at the conf file you sent, you have the below. Is your problem that you have 2 stream data tuples that have similar sets of tokens? ex: mux_stream_data_1 and mux_stream_data_2?

If so, this is a problem we have already solved in the case of DMIC. We have 2 sets of PDM config tokens and we parse both sets depending on the number of PDM's in the DMIC config. Could you please take a look?

SectionData."mask_priv_1" {
	       bytes "0x01,0x04,0x08,0x10,0x18,0x20,0x40,0x80"
}
SectionVendorTuples."mux_stream_data_1_tuples_w" {
	tokens "sof_mux_tokens"
	tuples."word" {
		SOF_TKN_MUX_STREAM_PIPELINE_ID 	"1"
		SOF_TKN_MUX_STREAM_CHANNELS		"2"
	}
}
SectionData."mux_stream_data_1" {
	tuples "mux_stream_data_1_tuples_w"
}

SectionData."mask_priv_2" {
	       bytes "0x01,0x04,0x08,0x10,0x18,0x20,0x40,0x80"
}
SectionVendorTuples."mux_stream_data_2_tuples_w" {
	tokens "sof_mux_tokens"
	tuples."word" {
		SOF_TKN_MUX_STREAM_PIPELINE_ID 	"5"
		SOF_TKN_MUX_STREAM_CHANNELS		"2"
	}
}
SectionData."mux_stream_data_2" {
	tuples "mux_stream_data_2_tuples_w"
}

juimonen · 2020-02-04T11:01:13Z

@ranj063 yes I saw the dmic stuff in sof_parse_tokens. I think my solution is more generic, but that's just my personal opinion... actually I would like to convert the dmic part similar to this solution.

sound/soc/sof/topology.c

lyakh · 2020-02-04T11:02:15Z

sound/soc/sof/topology.c

you could just add this to your loop condition as while (priv_size > 0 && found != count)

@lyakh I have problem with this function, so should we return error value if not all tokens are found? I think we shouldn't have situation where some tokens are defined but some are not? then I could return here 0 and something like -EINVAL at the end...

sound/soc/sof/topology.c

Currently topology parsing functions run through the whole data area searching for correct tuples array. If you happen to have multiple similar tuples arrays the last one is always copied on top of the previous. So change the parsing functions to return after first found array instance. To be able to parse several same type arrays the parsing needs to know where to start searching for the next instance. So make the parsing functions return the position after found array so that same type arrays can be parsed in a loop. This changes the semantics of the parsing return values so that positive value is not anymore considered error. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

ranj063 · 2020-02-04T18:29:31Z

sound/soc/sof/topology.c

@juimonen whats the purpose of read_bytes here? How is it used?

so the read_bytes will give you the number of how many bytes from the beginning of the data area the last array was found and parsed. So you can parse next array from that point and not start from beginning again and again... You can see the example in the demux parsing in this pr.

ranj063 · 2020-02-04T18:46:21Z

sound/soc/sof/topology.c

duplicate ;

oh, ok fixed now.

ranj063 · 2020-02-04T18:51:38Z

sound/soc/sof/topology.c

@juimonen If I am not wrong, you will end up overwriting msd with every stream_data array tokens that are parsed here isnt it? Did you miss incrementing msd addr inside the loop?

oops sorry didnt realize it was in the for (). please ignore

sound/soc/sof/topology.c

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

juimonen · 2020-02-06T07:13:11Z

@ranjani was checking again the dmic topology after talking to you.... So it is done little bit different than my implementation:

In dmic there's only 1 array with multiple tuplet sets inside it, I have here several arrays with 1 set of tuples inside.
dmic tuplet parse implementation assumes that the ipc struct is similar in contents of the struct coming from topology (so you can just increment 32bit slots and assume the value is inserted to correct place). As for example the demux "ipc" structs have all kinds of paddings etc. so you can't immediately assume that just incrementing the dst pointer we get correct placement.

So, we have quite a lot of assumptions in the parsing, not sure what would be the correct thing to do...

juimonen · 2020-02-06T11:49:03Z

@ranj063 @plbossart @lyakh thank you for you reviews. After talking with Ranjani I went for different approach: #1766

It is changing the existing dmic "multi-item" parsing and doesn't touch the demux at all, as I also learned the demux interface is in flux. In the new PR I try to generalize the "multi-item" parsing so the demux (or others) should be fairly easy to add.

juimonen · 2020-02-06T11:49:18Z

so closing...

juimonen requested review from bardliao, keyonjie, lyakh, ranj063 and singalsu as code owners January 31, 2020 12:52

juimonen changed the title ~~Demux tokens~~ [RFC] Demux tokens Jan 31, 2020

juimonen mentioned this pull request Jan 31, 2020

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

Closed

plbossart requested changes Jan 31, 2020

View reviewed changes

lyakh requested changes Feb 3, 2020

View reviewed changes

juimonen force-pushed the demux_tokens branch from 8cc4d6e to 9400b1c Compare February 3, 2020 09:27

lyakh reviewed Feb 4, 2020

View reviewed changes

ranj063 reviewed Feb 4, 2020

View reviewed changes

sound/soc/sof/topology.c Outdated Show resolved Hide resolved

juimonen force-pushed the demux_tokens branch from 9400b1c to b889d28 Compare February 4, 2020 19:16

juimonen closed this Feb 6, 2020

[RFC] Demux tokens #1758

[RFC] Demux tokens #1758

Uh oh!

Conversation

juimonen commented Jan 31, 2020

Uh oh!

juimonen commented Jan 31, 2020

Uh oh!

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

Uh oh!

Uh oh!

plbossart left a comment • 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!

plbossart Jan 31, 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!

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!

juimonen commented Feb 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

ranj063 commented Jan 31, 2020 •

edited

Loading

plbossart left a comment •

edited

Loading

plbossart Jan 31, 2020 •

edited

Loading

ranj063 commented Feb 3, 2020 •

edited

Loading