Skip to content
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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5629746
topology: nhlt: Fix dmic configuration blob building
Dec 27, 2023
c589dc3
topology: Expand attribute references inside $[] expressions
Dec 31, 2023
6fe2b9e
topology: Fix one character typo in code comments
Dec 31, 2023
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
47 changes: 27 additions & 20 deletions topology/nhlt/intel/dmic/dmic-process.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,8 +853,12 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
MIC_CONTROL_PDM_EN_A(1);
Copy link

Choose a reason for hiding this comment

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

typo in commit: clock diver -> clock divider

dmic->dmic_blob_pdm[i].mic_control = val;

/* if cfg->mfir_a */
if (di == 0) {
/*
* 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 configured earlier.
*/
if (cfg->mfir_a) {
/* FIR A */
fir_decim = MAX(cfg->mfir_a - 1, 0);
fir_length = MAX(cfg->fir_a_length - 1, 0);
Expand All @@ -863,24 +867,24 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
FIR_CONTROL_A_DCCOMP(dccomp) |
FIR_CONTROL_A_MUTE(fir_mute) |
FIR_CONTROL_A_STEREO(stereo[i]);
dmic->dmic_blob_fir[i][di].fir_control = val;
dmic->dmic_blob_fir[i][0].fir_control = val;

val = FIR_CONFIG_A_FIR_DECIMATION(fir_decim) |
FIR_CONFIG_A_FIR_SHIFT(cfg->fir_a_shift) |
FIR_CONFIG_A_FIR_LENGTH(fir_length);
dmic->dmic_blob_fir[i][di].fir_config = val;
dmic->dmic_blob_fir[i][0].fir_config = val;

val = DC_OFFSET_LEFT_A_DC_OFFS(DCCOMP_TC0);
dmic->dmic_blob_fir[i][di].dc_offset_left = val;
dmic->dmic_blob_fir[i][0].dc_offset_left = val;

val = DC_OFFSET_RIGHT_A_DC_OFFS(DCCOMP_TC0);
dmic->dmic_blob_fir[i][di].dc_offset_right = val;
dmic->dmic_blob_fir[i][0].dc_offset_right = val;

val = OUT_GAIN_LEFT_A_GAIN(0);
dmic->dmic_blob_fir[i][di].out_gain_left = val;
dmic->dmic_blob_fir[i][0].out_gain_left = val;

val = OUT_GAIN_RIGHT_A_GAIN(0);
dmic->dmic_blob_fir[i][di].out_gain_right = val;
dmic->dmic_blob_fir[i][0].out_gain_right = val;

/* Write coef RAM A with scaled coefficient in reverse order */
length = cfg->fir_a_length;
Expand All @@ -889,14 +893,15 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
cfg->fir_a_scale, 31,
DMIC_FIR_SCALE_Q, DMIC_HW_FIR_COEF_Q);
cu = FIR_COEF_A(ci);
/* blob_pdm[i].fir_coeffs[di][j] = cu; */
dmic->dmic_fir_array.fir_coeffs[i][di][j] = cu;
/* blob_pdm[i].fir_coeffs[0][j] = cu; */
dmic->dmic_fir_array.fir_coeffs[i][0][j] = cu;
}
dmic->dmic_fir_array.fir_len[0] = length;
dmic->dmic_fir_array.fir_len[1] = 0;
} else {
dmic->dmic_fir_array.fir_len[0] = 0;
}

if (di == 1) {
if (cfg->mfir_b) {
/* FIR B */
fir_decim = MAX(cfg->mfir_b - 1, 0);
fir_length = MAX(cfg->fir_b_length - 1, 0);
Expand All @@ -905,23 +910,23 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
FIR_CONTROL_B_DCCOMP(dccomp) |
FIR_CONTROL_B_MUTE(fir_mute) |
FIR_CONTROL_B_STEREO(stereo[i]);
dmic->dmic_blob_fir[i][di].fir_control = val;
dmic->dmic_blob_fir[i][1].fir_control = val;

val = FIR_CONFIG_B_FIR_DECIMATION(fir_decim) |
FIR_CONFIG_B_FIR_SHIFT(cfg->fir_b_shift) |
FIR_CONFIG_B_FIR_LENGTH(fir_length);
dmic->dmic_blob_fir[i][di].fir_config = val;
dmic->dmic_blob_fir[i][1].fir_config = val;
val = DC_OFFSET_LEFT_B_DC_OFFS(DCCOMP_TC0);
dmic->dmic_blob_fir[i][di].dc_offset_left = val;
dmic->dmic_blob_fir[i][1].dc_offset_left = val;

val = DC_OFFSET_RIGHT_B_DC_OFFS(DCCOMP_TC0);
dmic->dmic_blob_fir[i][di].dc_offset_right = val;
dmic->dmic_blob_fir[i][1].dc_offset_right = val;

val = OUT_GAIN_LEFT_B_GAIN(0);
dmic->dmic_blob_fir[i][di].out_gain_left = val;
dmic->dmic_blob_fir[i][1].out_gain_left = val;

val = OUT_GAIN_RIGHT_B_GAIN(0);
dmic->dmic_blob_fir[i][di].out_gain_right = val;
dmic->dmic_blob_fir[i][1].out_gain_right = val;

/* Write coef RAM B with scaled coefficient in reverse order */
length = cfg->fir_b_length;
Expand All @@ -930,10 +935,12 @@ static int configure_registers(struct intel_dmic_params *dmic, struct dmic_calc_
cfg->fir_b_scale, 31,
DMIC_FIR_SCALE_Q, DMIC_HW_FIR_COEF_Q);
cu = FIR_COEF_B(ci);
/* blob_pdm[i].fir_coeffs[di][j] = cu; */
dmic->dmic_fir_array.fir_coeffs[i][di][j] = cu;
/* blob_pdm[i].fir_coeffs[1][j] = cu; */
dmic->dmic_fir_array.fir_coeffs[i][1][j] = cu;
}
dmic->dmic_fir_array.fir_len[1] = length;
} else {
dmic->dmic_fir_array.fir_len[1] = 0;
}
}

Expand Down
22 changes: 20 additions & 2 deletions topology/pre-process-object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)]".

int ret;

ret = snd_config_search(tplg_pp->input_cfg, "Define", &conf_defines);
Expand All @@ -1600,14 +1601,31 @@ pre_process_object_variables_expand_fcn(snd_config_t **dst, const char *str, voi
if (ret >= 0)
return ret;

/* No global define found, proceeed to object attribute search */
if (snd_config_get_id(object_cfg, &object_id) < 0)
return -EINVAL;

/* find variable from object attribute values if not found in global definitions */
ret = pre_process_find_variable(dst, str, object_cfg);
if (ret < 0)
if (ret < 0) {
SNDERR("Failed to find definition for attribute %s in '%s' object\n",
str, object_id);
return ret;
}

/* the extracted value may contain a nested $-expression */
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jsarha jsarha Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@jsarha jsarha Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (snd_config_get_string(*dst, &val) >= 0) {
if (val[0] == '$') {
char *var = strdup(val);

snd_config_remove(*dst);
snd_config_delete(*dst);
ret = snd_config_evaluate_string(dst, var,
pre_process_object_variables_expand_fcn,
tplg_pp);
free(var);
}
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

could this be a recursive case like

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

Copy link
Member

@perexg perexg Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jsarha jsarha Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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().

Copy link
Member

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

return ret;
}
Expand All @@ -1623,7 +1641,7 @@ pre_process_object_variables_expand_fcn(snd_config_t **dst, const char *str, voi
* or '\0'.
*
* In '$[<contents>]' case all letters but '[' and ']' are allow in
* any sequence. Nested '[]' is also allowed if the number if '[' and
* any sequence. Nested '[]' is also allowed if the number of '[' and
* ']' match.
*
* The function modifies *stringp, and *prefix - if not NULL - points
Expand Down