topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250

jsarha · 2023-12-27T17:53:00Z

This PR fixes two issues. FIrst it fixes thesofproject/sof#8670 and then it adds to $[] expressions define handling in object attribute, so that we can use DMIC frequency defines in format objects, wihout ibs and obs attributes referring to those attributes breaking it.

jsarha · 2024-01-02T09:45:59Z

@singalsu , @lrgirdwo , and @ranj063 , please take a look, before I mark this ready for wider audiece review.

singalsu

Looks good and all changes work perfectly for me. Thanks for fixing these!

kv2019i

One potential issue in first patch, see inline.

Also, a bit more clarification where the second patch is needed would help. I get this is needed to simply the dmic rules. Maybe a reference to a PR that would be using this PR would help?

kv2019i · 2024-01-02T15:37:14Z

topology/nhlt/intel/dmic/dmic-process.c

@@ -853,8 +853,12 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
 			MIC_CONTROL_PDM_EN_A(1);


typo in commit: clock diver -> clock divider

kv2019i · 2024-01-02T15:37:49Z

topology/nhlt/intel/dmic/dmic-process.c

+		/*
+		 * Here we have to check the both FIRs if they are
+		 * configured as the later configured DAI may have changed
+		 * the configuration of the DAI configurer earlier.


typoe: configurer -> configured

kv2019i · 2024-01-02T15:47:20Z

topology/pre-process-object.c

+			ret = pre_process_find_variable(dst, var, conf_defines);
+			free(var);
+			return ret;
+		}


Shouldn't we "snd_config_delete(*dst)" also in this case won L1623...?

No, I don't think so. The *dst is passed to the caller and it should do what is necessary (however a string value in $[] is probably an error). The reason why I have to delete *dst here is because I reuse *dst to store the result of the defines search and the pointer holding the result of object attribute search may be lost.

Then again a paranoid programmer would check strdup() result and return -ENOMEM in case its NULL. But since this is a commmand line tool, and there is probably some out of memory error produced by libc, I do not think its necessary.

could this be a recursive case like

channels $CHANNELS sample_bits $channel * 4 frame_bits "$[$channels * $sample_bits]"

~~Maybe someone may look, if tplg_evaluate_config_string can be replaced with snd_config_evaluate_string implementation from alsa-lib.~~

EDIT: My point was that tplg_evaluate_config_string may be used here recursively.

Now that I think about it, I guess it could be that the object attribute is an expression of form $ [channels * 4] , plain $channels *4 should not be allowed. But yes, a recursive call to snd_config_evaluate_string() should do it.

About @perexg 's question, there is a functional difference in tplg_evaluate_config_string() and snd_config_evaluate_string(). tplg_evaluate_config_string() expands $ VAR_NAME occurrences in the middle of the string, unlike plain snd_config_evaluate_string(). However, the functionality could probably be moved to snd_config_evaluate_string().

It would be also more elegant to use snd_config_remove + snd_config_delete combo rather than duplicating the already allocated string.

snd_config_t *tmp = *dst; snd_config_remove(*tmp); // detach from the configuration tree snd_config_evaluate_string(dst, val, ....); snd_config_delete(*tmp);

I am not yet too fluent with snd_config_* -functions, but is the remove necessary in this case, where the *dst is already a product of snd_config_copy()?

Now that I have read the code back and forth a bit, I am pretty sure the remove is unnecessary for a node, that has just been copied with snd_config_copy(). However, @perexg if you still think that it is good coding practice to add it there, then certainly I will.

The dmic configuration functions are called for each dmic DAI (or FIFO) separately, and each dmic DAI is configured in their own configuration rounds. However, the later configured dmic FIFO may change the common clock divider and thus affect the FIR configuration of the first configured DAI. However, the first configured FIR blob is not touched after it is configured for the first time. To overcome this problem always check the both FIR configurations, no matter which DAI we are configuring. Suggested-by: Seppo Ingalsuo <[email protected]> Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2024-01-02T19:25:51Z

One potential issue in first patch, see inline.

Also, a bit more clarification where the second patch is needed would help. I get this is needed to simply the dmic rules. Maybe a reference to a PR that would be using this PR would help?

Its needed for example when $ DMIC0_RATE is used in audio_format topology object's in_rate attribute. That is because the attribute is also used in ibs attrbute in $[] expression:

https://github.com/thesofproject/sof/blob/98cc1d3da330052ac43f269fdce11aa8d4562cf0/tools/topology/topology2/include/common/audio_format.conf#L235

Without this change its not possible to put DMIC sample rate completely behind a single define.

jsarha · 2024-01-03T16:32:30Z

Changed since previous version. Make a receive call to snd_config_evaluate_string(), instead of just checking for a global define reference.

jsarha · 2024-01-03T21:29:13Z

Yet one more version that expands also $-expressions found from defines, e.g. this allows:

Define {
	FOO	"$[$BAR * 2]"
	BAR	2
}

Object.Base.foo_bar {
	foo	"$[$FOO * 2]"
	bar	"$[$bar * 2]"
}

Where Object.Base.foo_bar.bar would get value 2 * 2 * 2 * 2 = 16.

However, its worth mentioning that our nhlt code, which what we were working on when this need was recognized, is not able to handle $-expressions found from behind a define. This is fixable thou, and I have hackish code to do it, but I am not sure the added complexity is justified just for the sake of completeness. At least I would need to clean it up first.

ranj063 · 2024-01-04T20:17:48Z

topology/pre-process-object.c

@@ -1589,6 +1589,7 @@ pre_process_object_variables_expand_fcn(snd_config_t **dst, const char *str, voi
 	snd_config_t *object_cfg = tplg_pp->current_obj_cfg;
 	snd_config_t *conf_defines;
 	const char *object_id;
+	const char *val;


@jsarha will this still work if you had the object defined like this instead?

Object.Base.foo { channels $CHANNELS frame_bits "$[$channels * $sample_bits]" sample_bits $BIT_DEPTH }

Yes, that works after this change.

This is the original problem the "topology: Expand attribute references inside $[] expressions"-commit is fixing. E.g. allow Object.Base.input_audio_format.in_rate be defined as $DMIC0_RATE, without breaking input_audio_format.ibs "$[($in_channels * ($[($in_rate + 999)] / 1000)) * ($in_bit_depth / 8)]".

RanderWang · 2024-01-08T07:25:36Z

@jsarha The 2ch DMIC config can't work, on 4ch, do you know why ?

jsarha · 2024-01-08T13:36:32Z

@jsarha The 2ch DMIC config can't work, on 4ch, do you know why ?

Sorry, I do not follow. Some problem in 2ch DMIC nhlt blob generation in a topology that has 4ch DMIC? Can you explain how exactly can trigger this problem?

And if the issue is not directly related to this PR, then it would probably be best to create a SOF issue to track it.

RanderWang · 2024-01-09T01:49:45Z

@jsarha The 2ch DMIC config can't work, on 4ch, do you know why ?

Sorry, I do not follow. Some problem in 2ch DMIC nhlt blob generation in a topology that has 4ch DMIC? Can you explain how exactly can trigger this problem?

And if the issue is not directly related to this PR, then it would probably be best to create a SOF issue to track it.

Sorry, I meant that 2ch DMIC blob can't work. The generated DMIC blob is still 4ch case

RanderWang · 2024-01-09T02:46:26Z

I find the reason, thanks. Need to disable ENABLE_A & ENABLE_B for PDM1

ranj063 · 2024-01-09T16:37:34Z

topology/pre-process-object.c

-	if (ret < 0)
-		SNDERR("Failed to find definition for attribute %s in '%s' object\n",
-		       str, object_id);
+	/* the extracted value may contain a nested $-expression */


this can happen only in the case of a variable that points to a object attribute right? So can we move this block inside the previous if block after like 1612?

@ranj063 , is there any reason for us to forbid $[]-expressions in the global definitions? I can imagine that could be handy sometimes. If we allow that then we need to check for $[]-strings also in the global definitions.

@jarsha its not that we want to forbid but it's not really useful isn't it? The global definitions are all known prior to building all objects. So, having references to other globals is the only option and that seems futile isn't it?

@ranj063 , I don't think the preprocessor works that way. The top level definitions are never resolved for $-expression on their own, but only when references to them are found in object attribute values. And when they are resolved there, they can in turn refer to other object attributes found in the same object. E.g. I can define

Define { MY_OBS_DEF "$[($out_channels * ($[($out_rate + 999)] / 1000)) * ($out_bit_depth / 8)]" }

And then in

Class.Base."audio_format" { ... obs "$MY_OBS_DEF" }

I tested that this work.

I can imagine that there could be some use for some common expressions defined at top-level, relying on generic object attributes, and used in several objects.

but does that even make sense. Have a global definition that uses a local attribute definition? The other way around makes sense.

I can see some sense in that, but if you don't, that is good enough. We can very well live without this "feature". I'll disable it.

Properly expand referred object attributes inside $[] expression. This allows for example this simplified case: Define { CHANNELS 2 BIT_DEPTH 16 } Object.Base.foo { channels $CHANNELS sample_bits $BIT_DEPTH frame_bits "$[$channels * $sample_bits]" } Reported-by: Seppo Ingalsuo <[email protected]> Signed-off-by: Jyri Sarha <[email protected]>

Signed-off-by: Jyri Sarha <[email protected]>

jsarha · 2024-01-11T10:42:44Z

In the latest version I:

disabled the $-expression resolving if found from global defines
- related comments: topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250 (comment)
added snd_config_remove(*dst) here https://github.com/jsarha/alsa-utils/blob/6fe2b9ef7fad343bcf3cbe9975060d74b0610f92/topology/pre-process-object.c#L1621
- related comments: topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250 (comment)
- adding remove before delete should never do any harm (but should it be called from snd_config_delete() always?)

singalsu · 2024-01-23T08:55:48Z

@kv2019i @ranj063 @RanderWang Please re-review this. We can't merge patches to SOF without having these issues fixed.

ranj063 · 2024-01-23T16:10:59Z

i'm good with this one. Thanks @jsarha

jsarha · 2024-01-24T12:52:08Z

We now have the relevant approvals from Intel people. I think this could be ready for merge. @perexg , what do you think?

Properly expand referred object attributes inside $[] expression. This allows for example this simplified case: Define { CHANNELS 2 BIT_DEPTH 16 } Object.Base.foo { channels $CHANNELS sample_bits $BIT_DEPTH frame_bits "$[$channels * $sample_bits]" } Closes: #250 Reported-by: Seppo Ingalsuo <[email protected]> Signed-off-by: Jyri Sarha <[email protected]> Signed-off-by: Jaroslav Kysela <[email protected]>

Closes: #250 Signed-off-by: Jyri Sarha <[email protected]> Signed-off-by: Jaroslav Kysela <[email protected]>

jsarha mentioned this pull request Dec 27, 2023

[BUG] Alsatplg produces incorrect NHLT DMIC blob when second DAI rate is higher than first thesofproject/sof#8670

Closed

singalsu approved these changes Jan 2, 2024

View reviewed changes

jsarha marked this pull request as ready for review January 2, 2024 15:05

jsarha force-pushed the dmic-nhlt-fix branch from db1779d to 004d689 Compare January 2, 2024 15:09

kv2019i suggested changes Jan 2, 2024

View reviewed changes

jsarha force-pushed the dmic-nhlt-fix branch from 004d689 to 61972d9 Compare January 2, 2024 19:16

jsarha changed the title ~~topology: nhlt: Fix dmic configuration blob building~~ topology: Fix dmic nhlt blob building, and fu Jan 2, 2024

jsarha changed the title ~~topology: Fix dmic nhlt blob building, and fu~~ topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving Jan 2, 2024

singalsu mentioned this pull request Jan 3, 2024

Tools: Topology2: Add DMIC1 and other rates support thesofproject/sof#8688

Closed

jsarha force-pushed the dmic-nhlt-fix branch from 61972d9 to fb2839e Compare January 3, 2024 16:29

jsarha force-pushed the dmic-nhlt-fix branch 2 times, most recently from 80cb9aa to ef6a5b0 Compare January 3, 2024 21:17

ranj063 reviewed Jan 4, 2024

View reviewed changes

ranj063 reviewed Jan 9, 2024

View reviewed changes

Jyri Sarha added 2 commits January 11, 2024 12:32

topology: Fix one character typo in code comments

6fe2b9e

Signed-off-by: Jyri Sarha <[email protected]>

jsarha force-pushed the dmic-nhlt-fix branch from ef6a5b0 to 6fe2b9e Compare January 11, 2024 10:33

singalsu mentioned this pull request Jan 22, 2024

Tools: Topology2: Add other rates support for DMIC and HDA thesofproject/sof#8728

Merged

ranj063 approved these changes Jan 23, 2024

View reviewed changes

RanderWang approved these changes Jan 24, 2024

View reviewed changes

perexg closed this in 6eb1eb5 Jan 24, 2024

perexg pushed a commit that referenced this pull request Jan 24, 2024

topology: Fix one character typo in code comments

6da3737

Closes: #250 Signed-off-by: Jyri Sarha <[email protected]> Signed-off-by: Jaroslav Kysela <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250

topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250

jsarha commented Dec 27, 2023 •

edited

Loading

jsarha commented Jan 2, 2024

singalsu left a comment

kv2019i left a comment

kv2019i Jan 2, 2024

kv2019i Jan 2, 2024

kv2019i Jan 2, 2024

jsarha Jan 2, 2024

jsarha Jan 2, 2024

RanderWang Jan 3, 2024

perexg Jan 3, 2024 •

edited

Loading

jsarha Jan 3, 2024 •

edited

Loading

perexg Jan 3, 2024

jsarha Jan 3, 2024

jsarha Jan 5, 2024

jsarha commented Jan 2, 2024 •

edited

Loading

jsarha commented Jan 3, 2024

jsarha commented Jan 3, 2024 •

edited

Loading

ranj063 Jan 4, 2024

jsarha Jan 5, 2024

RanderWang commented Jan 8, 2024

jsarha commented Jan 8, 2024

RanderWang commented Jan 9, 2024

RanderWang commented Jan 9, 2024 •

edited

Loading

ranj063 Jan 9, 2024

jsarha Jan 9, 2024 •

edited

Loading

ranj063 Jan 9, 2024

jsarha Jan 9, 2024 •

edited

Loading

ranj063 Jan 10, 2024

jsarha Jan 11, 2024

jsarha commented Jan 11, 2024

singalsu commented Jan 23, 2024

ranj063 commented Jan 23, 2024

jsarha commented Jan 24, 2024

		@@ -853,8 +853,12 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
		MIC_CONTROL_PDM_EN_A(1);

topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250

topology: Fix dmic nhlt blob building, and fix $[] attribute value resolving #250

Conversation

jsarha commented Dec 27, 2023 • edited Loading

jsarha commented Jan 2, 2024

singalsu left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perexg Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

jsarha Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Jan 2, 2024 • edited Loading

jsarha commented Jan 3, 2024

jsarha commented Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Jan 8, 2024

jsarha commented Jan 8, 2024

RanderWang commented Jan 9, 2024

RanderWang commented Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

jsarha Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha commented Jan 11, 2024

singalsu commented Jan 23, 2024

ranj063 commented Jan 23, 2024

jsarha commented Jan 24, 2024

jsarha commented Dec 27, 2023 •

edited

Loading

perexg Jan 3, 2024 •

edited

Loading

jsarha Jan 3, 2024 •

edited

Loading

jsarha commented Jan 2, 2024 •

edited

Loading

jsarha commented Jan 3, 2024 •

edited

Loading

RanderWang commented Jan 9, 2024 •

edited

Loading

jsarha Jan 9, 2024 •

edited

Loading

jsarha Jan 9, 2024 •

edited

Loading