Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kv2019i merged 2 commits into from
Apr 14, 2020
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b7a2f0
ASoC: SOF: topology: stop parsing when all tokens have been found
Feb 11, 2020
5059ec4
ASoC: SOF: topology: handle multiple sets of tuple arrays
Feb 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 98 additions & 71 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,13 +753,15 @@ static const struct sof_topology_token led_tokens[] = {
get_token_u32, offsetof(struct snd_sof_led_control, direction), 0},
};

static void sof_parse_uuid_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array)
static int sof_parse_uuid_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array,
size_t offset)
{
struct snd_soc_tplg_vendor_uuid_elem *elem;
int found = 0;
int i, j;

/* parse element by element */
Expand All @@ -777,19 +779,26 @@ static void sof_parse_uuid_tokens(struct snd_soc_component *scomp,
continue;

/* matched - now load token */
tokens[j].get_token(elem, object, tokens[j].offset,
tokens[j].get_token(elem, object,
offset + tokens[j].offset,
tokens[j].size);

found++;
}
}

return found;
}

static void sof_parse_string_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array)
static int sof_parse_string_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array,
size_t offset)
{
struct snd_soc_tplg_vendor_string_elem *elem;
int found = 0;
int i, j;

/* parse element by element */
Expand All @@ -807,24 +816,27 @@ static void sof_parse_string_tokens(struct snd_soc_component *scomp,
continue;

/* matched - now load token */
tokens[j].get_token(elem, object, tokens[j].offset,
tokens[j].get_token(elem, object,
offset + tokens[j].offset,
tokens[j].size);

found++;
}
}

return found;
}

static void sof_parse_word_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array)
static int sof_parse_word_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array,
size_t offset)
{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_tplg_vendor_value_elem *elem;
size_t size = sizeof(struct sof_ipc_dai_dmic_pdm_ctrl);
int found = 0;
int i, j;
u32 offset;
u32 *index = NULL;

/* parse element by element */
for (i = 0; i < le32_to_cpu(array->num_elems); i++) {
Expand All @@ -843,58 +855,45 @@ static void sof_parse_word_tokens(struct snd_soc_component *scomp,
if (tokens[j].token != le32_to_cpu(elem->token))
continue;

/* pdm config array index */
if (sdev->private)
index = sdev->private;

/* matched - determine offset */
switch (tokens[j].token) {
case SOF_TKN_INTEL_DMIC_PDM_CTRL_ID:

/* inc number of pdm array index */
if (index)
(*index)++;
/* fallthrough */
case SOF_TKN_INTEL_DMIC_PDM_MIC_A_Enable:
case SOF_TKN_INTEL_DMIC_PDM_MIC_B_Enable:
case SOF_TKN_INTEL_DMIC_PDM_POLARITY_A:
case SOF_TKN_INTEL_DMIC_PDM_POLARITY_B:
case SOF_TKN_INTEL_DMIC_PDM_CLK_EDGE:
case SOF_TKN_INTEL_DMIC_PDM_SKEW:

/* check if array index is valid */
if (!index || *index == 0) {
dev_err(scomp->dev,
"error: invalid array offset\n");
continue;
} else {
/* offset within the pdm config array */
offset = size * (*index - 1);
}
break;
default:
offset = 0;
break;
}

/* load token */
tokens[j].get_token(elem, object,
offset + tokens[j].offset,
tokens[j].size);

found++;
}
}

return found;
}

static int sof_parse_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array,
int priv_size)
{
/**
* sof_parse_token_sets - Parse multiple sets of tokens
* @scomp: pointer to soc component
* @object: target ipc struct for parsed values
* @tokens: token definition array describing what tokens to parse
* @count: number of tokens in definition array
* @array: source pointer to consecutive vendor arrays to be parsed
* @priv_size: total size of the consecutive source arrays
* @sets: number of similar token sets to be parsed, 1 set has count elements
* @object_size: offset to next target ipc struct with multiple sets
*
* This function parses multiple sets of tokens in vendor arrays into
* consecutive ipc structs.
*/
static int sof_parse_token_sets(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array,
int priv_size, int sets, size_t object_size)
{
size_t offset = 0;
int found = 0;
int total = 0;
int asize;

while (priv_size > 0) {
while (priv_size > 0 && total < count * sets) {
asize = le32_to_cpu(array->size);

/* validate asize */
Expand All @@ -915,19 +914,19 @@ static int sof_parse_tokens(struct snd_soc_component *scomp,
/* call correct parser depending on type */
switch (le32_to_cpu(array->type)) {
case SND_SOC_TPLG_TUPLE_TYPE_UUID:
sof_parse_uuid_tokens(scomp, object, tokens, count,
array);
found += sof_parse_uuid_tokens(scomp, object, tokens,
count, array, offset);
break;
case SND_SOC_TPLG_TUPLE_TYPE_STRING:
sof_parse_string_tokens(scomp, object, tokens, count,
array);
found += sof_parse_string_tokens(scomp, object, tokens,
count, array, offset);
break;
case SND_SOC_TPLG_TUPLE_TYPE_BOOL:
case SND_SOC_TPLG_TUPLE_TYPE_BYTE:
case SND_SOC_TPLG_TUPLE_TYPE_WORD:
case SND_SOC_TPLG_TUPLE_TYPE_SHORT:
sof_parse_word_tokens(scomp, object, tokens, count,
array);
found += sof_parse_word_tokens(scomp, object, tokens,
count, array, offset);
break;
default:
dev_err(scomp->dev, "error: unknown token type %d\n",
Expand All @@ -938,10 +937,35 @@ static int sof_parse_tokens(struct snd_soc_component *scomp,
/* next array */
array = (struct snd_soc_tplg_vendor_array *)((u8 *)array
+ asize);

/* move to next target struct */
if (found >= count) {
offset += object_size;
total += found;
found = 0;
}
Copy link
Member

@plbossart plbossart Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if found != count?

Copy link
Author

@juimonen juimonen Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should run until end of area and return -EINVAL

Copy link
Collaborator

@lyakh lyakh Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juimonen isn't there a possibility, that if found on one of iterations jumps from < count to > count, then the loop will continue until it runs out of space, but then it will return 0 - success?

Copy link
Author

@juimonen juimonen Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh the count is given to subfunctions and subfunctions ++ the found max count times... so found should never be more than count.(?)

Copy link
Collaborator

@lyakh lyakh Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juimonen is it found++ or found += N? It looks like found can be incremented by more than 1 on a single iteration. This is what I'm thinking about: say, iteration 0 count = 4, found = 0. You go to one of parsers and it returns 3. That's possible, right? Now 3 != 4, so you don't reset found. You go to the next iteration and again call the parser with count = 4, so, it can return anything <= 4, say, it returns 3 again. Now found = 6...

Copy link
Author

@juimonen juimonen Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh no, they might find more or less or exact amount as different token types might make up a "set". so set might be like 2 uuids and 1 string. And array here is not same as "set". array is the alsa topology concept that packages a token. All tokens are in arrays, despite they belong to "artificial" composition called "set". So sets are like the "_tokens" structs on top of this file i.e. composition of multiple tokens we try to set for ipc.

So someone can put less tokens to topology or multiple overlapping tokens (so this doesn't make any sense, it is a error case for us). Before this PR this function was just blindly copying on top of previous found item. If you had less tokens than count, you would run until the end (and I think return success still, or actually this was "void" so you would not even know that).

Copy link
Author

@juimonen juimonen Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart @ranj063 as chatting today with @lyakh, I was really testing what happens
with different "token cases", so FYI (and sorry for the long report):

I was running basic sof-apl-pcm512x.tplg in up2, findings:

We search for following tokens, but don't care if they don't exist
(because they don't exist in this topology and it is successfully loaded):

pcm: dmac_tokens
volume_control: led_tokens
dai: stream_tokens
link: hda_tokens (which btw token array is empty so count = 0?)

So I can see that we have tokens which are optional, so we should
be able to search for those but not bail out.

In this sense sof_parse_token_sets is returning error only
in case:

  • source token array size coming from user space is > 0
  • dest data priv_size is enough to hold the token
  • uuid type in source token array is supported

So currently sof_parse_token_(sets) doesn't care if the token is found or not,
just that parameters given to it are sane.

I tried the following error cases:

  1. duplicates, more values than suppose to be there

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"
SOF_TKN_INTEL_DMIC_PDM_POLARITY_A "2"
SOF_TKN_INTEL_DMIC_PDM_POLARITY_B "2"
SOF_TKN_INTEL_DMIC_PDM_CLK_EDGE "2"
SOF_TKN_INTEL_DMIC_PDM_SKEW "2"
}

In this case alsa_tplg is parsing the duplicates out and
the last set values remain, when converting from conf to tplg binary.
I could try to hack the binary but probably doesn't make sense...

  1. too few values

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

No error, we are just not setting the values in ipc struct,
so they are the default what was in ipc struct creation.

  1. 2 sets with 1 too few values

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

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

because found is in first iteration 5 then 7
it is newer equal to count, so offset is not updated
and all values (even duplicates) are copied on top of
first set. 2nd set will remain in default values from
ipc struct creation.

So questions:

  1. are 2 and 3 errors as all ipc struct values in the set aren't found?
  2. should we return error if not all are found and change all optional token
    parsing in upper level to handle that, mandatory ones should bail out with error?

Copy link
Collaborator

@kv2019i kv2019i Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @juimonen. That's a bit surprising. Reading parse_tokens documentation, it would seem that all the tokens are mandatory, but based on your analysis, with our existing topologies, we in practise have optional elements, so if we start reporting errors, we can expect many failures from existing topologies, right? I'd vote to make the parsing as strict as possible, given current topologies, but no more.

Copy link
Author

@juimonen juimonen Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i yes, as this is not mostly my original code, I really can't trace the original author's idea about the parsing... Anyway as said, I can make the parsing more strict and handle the optional cases I found. But that would be probably new commit or perhaps a PR and not strictly related to this PR. And would need even more extensive testing in all possible platforms.

So should I try to fix that? I can see try to see what happens, this PR is not in panic to get merged...

Copy link
Collaborator

@kv2019i kv2019i Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return 0;
}

static int sof_parse_tokens(struct snd_soc_component *scomp,
void *object,
const struct sof_topology_token *tokens,
int count,
struct snd_soc_tplg_vendor_array *array,
int priv_size)
{
/*
* sof_parse_tokens is used when topology contains only a single set of
* identical tuples arrays. So additional parameters to
* sof_parse_token_sets are sets = 1 (only 1 set) and
* object_size = 0 (irrelevant).
*/
return sof_parse_token_sets(scomp, object, tokens, count, array,
priv_size, 1, 0);
}

static void sof_dbg_comp_config(struct snd_soc_component *scomp,
struct sof_ipc_comp_config *config)
{
Expand Down Expand Up @@ -2919,9 +2943,12 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
return -ENOMEM;

/* get DMIC PDM tokens */
ret = sof_parse_tokens(scomp, &config->dmic.pdm[0], dmic_pdm_tokens,
ret = sof_parse_token_sets(scomp, &config->dmic.pdm[0], dmic_pdm_tokens,
ARRAY_SIZE(dmic_pdm_tokens), private->array,
le32_to_cpu(private->size));
le32_to_cpu(private->size),
config->dmic.num_pdm_active,
sizeof(struct sof_ipc_dai_dmic_pdm_ctrl));

if (ret != 0) {
dev_err(scomp->dev, "error: parse dmic pdm tokens failed %d\n",
le32_to_cpu(private->size));
Expand Down