ASoC: SOF: topology: change parsing functions to handle multiple items #1766

juimonen · 2020-02-06T11:44:37Z

Widget's parameters can consist of several different types of tuples
arrays like strings, words and bytes. In essence, SOF topology parsing
transforms these sets of tuples arrays into an ipc message sent to dsp.
Lately we've been seeing more complex widget definitions with multiple
identical tuples array sets. These include for example dmic pdm
configuration and mux/demux channel map definitions.

SOF driver gets the widget from ASoC layer and parses the tuples from
widget's data. Currently parsing functions run always through the
widget's entire data area searching for the correct set of tuples
arrays.

If the widget happens to have multiple similar sets, parser doesn't
return after first found "instance", but the next one is always copied
on top of the previous one until end of data. So if you have multiple
identical token sets in topology, the parser will always return the last
one.

To be able to parse multiple tuples array sets this patch:

Changes the parsing so that the parsing will stop after all tokens
for single set have been found as there is no reason to continue parsing
after that.
Adds new function sof_parse_token_sets which can be used to parse
multiple sets. This function uses offset and number of sets to copy the
tokens to correct positions in the destination ipc struct.
sof_parse_token will be a special case of calling sof_parse_token_sets
to parse 1 set with offset 0.
Modifies the dmic dai link loading to show how to use
sof_parse_array_sets to load multiple sets.

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

plbossart

@juimonen you can reduce the whole of changes a great deal, and with precisions on what you are trying to accomplish you'd make the task of reviewers a lot easier :-)
it's not even an objection on the merits of the patch, it's just hard to even get to the technical part of your proposal
Thanks!

sound/soc/sof/topology.c

juimonen · 2020-02-07T09:07:17Z

@plbossart updated.

There's still only 1 commit, but the number or changes should have been reduced. It is quite difficult to separate to commits as the changes don't make too much sense alone.

The multiple entities are now called "sets" of tuples arrays. Also tried to change the commit message to be more clear...

juimonen · 2020-02-07T09:16:38Z

and changed to [RFC] as this needs to be tested really well as I'm poking with topology parsing...

plbossart · 2020-02-07T15:53:38Z

3. Modifies the dmic dai link loading to show how to use
sof_parse_array_sets to load multiple sets.

I am still a bit in the dark on the dmic changes. Are you
a) trying to change the topology definitions for DMICs?
b) trying to fix something that's broken with DMICs
c) trying to apply a new way of parsing to the existing DMICs

Overall I am still not clear on what we are really trying to fix

ranj063 · 2020-02-07T17:05:26Z

Modifies the dmic dai link loading to show how to use
sof_parse_array_sets to load multiple sets.

I am still a bit in the dark on the dmic changes. Are you
a) trying to change the topology definitions for DMICs?
b) trying to fix something that's broken with DMICs
c) trying to apply a new way of parsing to the existing DMICs

Overall I am still not clear on what we are really trying to fix

@plbossart let me try and explain. Our topology definitions today contain multiple sets of the same tuples only for the case of the DMIC PDM config tokens (see below). So the topology parser handles parsing these tokens in a very specific way. But with the addition of multiple set of tuples for the MUX/DEMUX stream data, we need to make the logic generic and this is what @juimonen and I discussed we should do. I will review the patch and add more comments. Does that make sense?

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


}

ranj063 · 2020-02-07T17:08:22Z

and changed to [RFC] as this needs to be tested really well as I'm poking with topology parsing...

@juimonen I had a slightly different approach in mind. Let me send you a patch today.

ranj063 · 2020-02-10T15:30:52Z

@juimonen lets stick with your PR but we should split this into 3 commits to make the intention clear:

Optimize token parsing to stop when all required tokens are parsed.
Make DMIC PDM token parsing logic generic to be applicable for all token types
Make all token parsing generic for multiple tuple arrays.

juimonen · 2020-02-11T13:55:17Z

@ranj063 now chopped to 3 commits. However I'm not sure the dmic parsing will work properly after 2nd commit, that's why I made originally it with 1 patch. Anyway, hope it is more clear.

sound/soc/sof/topology.c

juimonen · 2020-02-11T16:50:21Z

@ranj063 I'm working at the same time on the demux with new interface (channel map) and unify it with dmic handling in this PR. So this PR would then also make more sense...

sound/soc/sof/topology.c

juimonen · 2020-02-12T12:43:04Z

@ranj063 updated, tried to address your comments.

ranj063

LGTM now @juimonen. Thanks!

ranj063 · 2020-02-12T16:01:29Z

@lyakh @kv2019i @plbossart this PR is needed to move forward and add support for other widgets with complex token arrays like the DMIC PDM tokens.

lyakh

almost LGTM, just a couple of minor comments from me

sound/soc/sof/topology.c

lyakh · 2020-04-03T06:27:17Z

sound/soc/sof/topology.c

Didn't you want to change this to >=? If I understood you correctly, you said, that currently the ALSA topology compiler cannot generate arrays with elements of different sizes, right? So you should be safe here. But if someone wants to crash the kernel they will make a modified version of the compiler to create invalid topology binaries. So I don't think we can rely on that.

ok, changed now to >=

kv2019i

@juimonen I think this starts to be ready. One thing that bugs a bit me when rereading the reviews: the commit message doesn't actually explain why you are doing this. What is the benefit of this change? I think you should address this in the commit message. This is a fairly nontrivial patch and there'd need to be some motivation in the commit message why the change is needed. To me, reading code, the biggest benefit in current codebase seems to get rid of the weird usage of sof->private in dmic pdm parsing. Is there others?

kv2019i · 2020-04-06T09:50:31Z

sound/soc/sof/topology.c

+			offset += object_size;
+			total += found;
+			found = 0;
+		}


@juimonen At least not for this PR. Let's fix one issue at a time.

juimonen · 2020-04-06T10:03:01Z

@kv2019i yes that's the benefit. Other is the optimization, that we are not parsing until the end of data area if we found the tokens. Before it was always going to the end of area even if token we're found. So it is a optimization, as @ranj063 wanted to point out in coommit message.

Now for the "sets" parsing to work, I need to found items also (to be able to move to next set). So the commits are interdependent, I initially made only 1 commit, but as requested, I splitted it up.

so summa summarum:

do not parse more if we have found all tokens (optimization)
make parsing of "sets" possible in general, not just for dmic (feature)
make dmic to use the new "sets" method

kv2019i · 2020-04-06T14:53:10Z

@juimonen wrote:

@kv2019i yes that's the benefit. Other is the optimization, that we are not parsing until the end of data area if we found the tokens. Before it was always going to the end of area even if token we're found. So it is a optimization, as @ranj063 wanted to point out in coommit message.

Ack, this is clear, first commit msg is good.

so summa summarum:
2. make parsing of "sets" possible in general, not just for dmic (feature)
3. make dmic to use the new "sets" method

So I'd like the commit msg of second to mention this briefly, to describe why these changes are done. Now it just tells you add ability to parse sets, but this is a bit odd as to why dmic worked at all if sets were not parsed before. I think the commit message should explain this better.

juimonen · 2020-04-06T16:27:08Z

@kv2019i hmm, I now modified the 2nd commit message a bit. But as I see it, I'm exactly talking in the commit message first about the multiset generalization and then mentioning in the end that I'm modifying the dmic config.

I think this is one of the most logical commit messages I've ever written :) (which might not tell good thing about me)... so next you'll have to suggest something and guide me through it...

kv2019i

Thanks @juimonen , now looks good. To me, "which is currently handled as a special case in token
parsing. This is not scalable for other components with multiple sets." is the magic addition.

kv2019i · 2020-04-09T08:27:11Z

@juimonen needs rebase
@plbossart ping, you still have requested changes open

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 are set in topology and they usually consist of several different types of tuple arrays like strings, words and bytes. Here this kind of combination is called a "set". Lately we've seen more complex widget definitions with multiple identical sets of tuple arrays. One example is the dmic pdm configuration, which is currently handled as a special case in token parsing. This is not scalable for other components with multiple sets. So add a new function sof_parse_token_sets, which can be used to parse multiple 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 function will be a special case of calling sof_parse_token_sets to parse 1 set with offset 0. Finally modify the dmic dai link loading to use the new sof_parse_array_sets to load multiple pdm configs. Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>

plbossart · 2020-04-09T11:27:47Z

sound/soc/sof/control.c

this looks like a different patch?

hmm, yes dealing with several PR's at the same time... sorry. should be updated now.

plbossart

I still haven't understood why we need this but the code looks fine, thanks @juimonen

juimonen requested review from bardliao, ranj063 and singalsu as code owners February 6, 2020 11:44

juimonen mentioned this pull request Feb 6, 2020

[RFC] Demux tokens #1758

Closed

juimonen force-pushed the multi_item branch from 7164b90 to 33a0272 Compare February 6, 2020 11:52

plbossart requested changes Feb 6, 2020

View reviewed changes

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

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

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

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

juimonen force-pushed the multi_item branch from 33a0272 to 7e5ea6c Compare February 7, 2020 08:43

juimonen changed the title ~~ASoC: SOF: topology: change parsing functions to handle multiple items~~ [RFC] ASoC: SOF: topology: change parsing functions to handle multiple items Feb 7, 2020

juimonen force-pushed the multi_item branch from 7e5ea6c to a7b8f3c Compare February 7, 2020 11:01

ranj063 mentioned this pull request Feb 7, 2020

Update DMIC parsing logic #1773

Closed

juimonen force-pushed the multi_item branch from a7b8f3c to af8de74 Compare February 11, 2020 13:53

ranj063 reviewed Feb 11, 2020

View reviewed changes

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

ranj063 reviewed Feb 11, 2020

View reviewed changes

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

ranj063 reviewed Feb 11, 2020

View reviewed changes

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

ranj063 reviewed Feb 11, 2020

View reviewed changes

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

juimonen force-pushed the multi_item branch from af8de74 to a9e9702 Compare February 12, 2020 12:42

ranj063 previously approved these changes Feb 12, 2020

View reviewed changes

ranj063 requested review from lyakh and plbossart February 12, 2020 16:00

lyakh requested changes Feb 14, 2020

View reviewed changes

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

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

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

juimonen requested review from lyakh and plbossart April 2, 2020 08:07

lyakh requested changes Apr 3, 2020

View reviewed changes

juimonen force-pushed the multi_item branch from 942f32f to 08d6d69 Compare April 3, 2020 06:38

lyakh previously approved these changes Apr 6, 2020

View reviewed changes

kv2019i reviewed Apr 6, 2020

View reviewed changes

juimonen dismissed lyakh’s stale review via 2c8bd7e April 6, 2020 16:24

juimonen force-pushed the multi_item branch from 08d6d69 to 2c8bd7e Compare April 6, 2020 16:24

kv2019i previously approved these changes Apr 7, 2020

View reviewed changes

kv2019i requested review from lyakh and ranj063 April 7, 2020 09:10

lyakh previously approved these changes Apr 8, 2020

View reviewed changes

kv2019i requested review from plbossart and removed request for plbossart April 8, 2020 07:12

juimonen dismissed stale reviews from lyakh and kv2019i via dfca80f April 9, 2020 08:37

juimonen force-pushed the multi_item branch from 2c8bd7e to dfca80f Compare April 9, 2020 08:37

Jaska Uimonen added 2 commits April 9, 2020 14:25

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

6b7a2f0

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>

plbossart requested changes Apr 9, 2020

View reviewed changes

juimonen force-pushed the multi_item branch from dfca80f to 5059ec4 Compare April 9, 2020 11:41

plbossart approved these changes Apr 9, 2020

View reviewed changes

ranj063 approved these changes Apr 9, 2020

View reviewed changes

lyakh approved these changes Apr 14, 2020

View reviewed changes

kv2019i approved these changes Apr 14, 2020

View reviewed changes

kv2019i merged commit 7318743 into thesofproject:topic/sof-dev Apr 14, 2020

ASoC: SOF: topology: change parsing functions to handle multiple items #1766

ASoC: SOF: topology: change parsing functions to handle multiple items #1766

Uh oh!

Conversation

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

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juimonen commented Feb 7, 2020

Uh oh!

juimonen commented Feb 7, 2020

Uh oh!

plbossart commented Feb 7, 2020

Uh oh!

ranj063 commented Feb 7, 2020

Uh oh!

ranj063 commented Feb 7, 2020

Uh oh!

ranj063 commented Feb 10, 2020

Uh oh!

juimonen commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juimonen commented Feb 11, 2020

Uh oh!

Uh oh!

juimonen commented Feb 12, 2020

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Feb 12, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

juimonen Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

juimonen commented Apr 6, 2020

Uh oh!

kv2019i commented Apr 6, 2020

Uh oh!

juimonen commented Apr 6, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Apr 9, 2020

Uh oh!

plbossart Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

juimonen Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

juimonen commented Feb 6, 2020 •

edited

Loading