alsactl DUT restoration #1270

LukaszMrugala · 2025-05-12T07:23:08Z

Proof of concept of a change in the way we restore DUTs to their pre-test states.

This solution uses separate alsactl .state files instead of bash scripts to set up SOF DUTs. It still uses the $MODEL variable to decide on the file to use. Currently, LNL, MTL and PTLs that were able to be used in the CI were updated to that new method.

Creating such files for DUTs no longer in CI is a tough nut to crack.

sofci · 2025-05-12T07:27:05Z

Can one of the admins verify this patch?

reply test this please to run this test once

case-lib/lib.sh

marc-hb

Replacing imperative with declarative is usually a good idea but why is this almost 10 times longer!? Is it really equivalent?

For sure the script approach was INCREMENTAL: changing some settings and leaving other in place. Can alsactl do that too or does it save/restore a FULL state every time? In the latter case, this is not just refactoring with a different approach but a big functional change. Which may or may not be better, I don't know that but it should be very clear what it is.

By the way, incremental approaches tend to support (ALSA) upgrades better, will a "FULL restore" approach be compatible across different ALSA versions?

Creating such files for DUTs no longer in CI is a tough nut to crack.

It shouldn't be hard to have some transition period that supports both approaches. That's even more important if the two approaches are not equivalent to each other.

EDIT

cc:

#1275
#1279

LukaszMrugala · 2025-05-28T09:11:43Z

For sure the script approach was INCREMENTAL: changing some settings and leaving other in place. Can alsactl do that too or does it save/restore a FULL state every time? In the latter case, this is not just refactoring with a different approach but a big functional change. Which may or may not be better, I don't know that but it should be very clear what it is.

This approach should only modify the values mentioned in the .state file. So while a typical machine would have a couple dozen values, only the few specified in the .state file will be changed. That should mean this approach is equivalent.

It's most clear in the MTLP_SDW_AIOC.state case. The .state file specifies controls 1, 3, 5, 8, 35 and 36. The same controls were set by the .sh scripts. All the others should be left as-is.

marc-hb · 2025-05-28T19:01:42Z

Replacing imperative with declarative is usually a good idea but why is this almost 10 times longer!?

I spent more than 10s looking at this and realized most of the extra verbosity comes from comment clauses. Most of which do... not look like English comments but like code instead. I'm confused.

LukaszMrugala · 2025-05-29T09:45:37Z

Replacing imperative with declarative is usually a good idea but why is this almost 10 times longer!?

I spent more than 10s looking at this and realized most of the extra verbosity comes from comment clauses. Most of which do... not look like English comments but like code instead. I'm confused.

They inform the user or, more like, someone who wants to modify the values set there, which are possible. Let's take an arbitrary control as an example, firstly without a comment block:

control.1 {
	iface MIXER
	name 'rt711 FU05 Playback Volume'
	value.0 80
	value.1 80
}

Say that my tests come out too low (volume too low). I'd like to max out this volume. What is the maximum value? I cannot know without accessing the machine. Now with the comment block:

control.1 {
	iface MIXER
	name 'rt711 FU05 Playback Volume'
	value.0 80
	value.1 80
	comment {
		access 'read write'
		type INTEGER
		count 2
		range '0 - 87'
		dbmin -6525
		dbmax 0
		dbvalue.0 -525
		dbvalue.1 -525
	}
}

OK, maximum value is 87 and I can change 80 to 87 and be sure that it's max. I do not need to worry what type the values are, how many there are, etc. - it's all in the comment block.

marc-hb · 2025-05-29T16:59:50Z

Still not sure sorry: are you saying the following comment is never parsed by any machine but only for humans? If yes then it should not look like it's written for machines; that's misleading. Right now it really looks like it will be used by some validation code.

Maybe there's some relevant ALSA documentation I'm not aware of, if yes then by all means please share.

	comment {
		access 'read write'
		type INTEGER
		count 2
		range '0 - 87'
		dbmin -6525
		dbmax 0
		dbvalue.0 -525
		dbvalue.1 -525
	}

LukaszMrugala · 2025-05-30T09:46:49Z

Still not sure sorry: are you saying the following comment is never parsed by any machine but only for humans? If yes then it should not look like it's written for machines; that's misleading. Right now it really looks like it will be used by some validation code.

Maybe there's some relevant ALSA documentation I'm not aware of, if yes then by all means please share.

The comment section should not be parsed by the machine but used by humans. It looks like a machine comment, because ALSA generates the comment section when dumping its state, so the end user doesn't have to write it by themselves.

I wasn't yet able to find a relevant documentation for that feature. Will update if I find it.

golowanow · 2025-05-30T10:50:14Z

case-lib/lib.sh

+    ALSACTL_STATE_FILE_PATH="$1"/alsa_settings/alsactl/"$2".state
+    # .state file does not exist. Creating one from current setup.
+    if [ ! -f "$ALSACTL_STATE_FILE_PATH" ]; then
+        alsactl store --file="$ALSACTL_STATE_FILE_PATH"


alsactl will store all devices present

That shouldn't be a problem, right?

It will look different to what is in the handcrafted *.state files which are for only one card at the moment.
Can you try your PR on a NOCODEC dut ?

It will look different to what is in the handcrafted *.state files which are for only one card at the moment.

It will look different, but that specific function is unused in the automatic code and is left as a tool for new .state file creation.

Can you try your PR on a NOCODEC dut ?

The current code does not touch NOCODEC DUTs. The run No. 53589 contains a NOCODEC platform.

case-lib/lib.sh

golowanow · 2025-05-30T17:27:10Z

alsa_settings/alsactl/README.md

@@ -0,0 +1,7 @@
+# Creating a .state file
+
+1. Gain access to a machine you'd like to create a state file for.


How all the *.state files in this PR were obtained ? (context: platform in default state, kernel, version, topology, whatever else what's matter)

sof and linux builds were taken from that day's CI relevant run. Then I've run the PR with alsa-info print added in. This gave me the full .state file I could then adjust.

as I understand the idea behind this PoC proposal, these *.state files are transitional having their content specific to the SOF/Linux components currently deployed on a DUT, thus in most cases they should be explicitly created at some early stage of a test plan execution to keep the appropriate defaults to reset before each test case, where it is important.

these *.state files are transitional having their content specific to the SOF/Linux components currently deployed on a DUT, ...

That looks like a great catch, I feel bad I missed this.

I do not seem to understand. They necessarily cannot be transitional (temporary in their existence), as they are the definition of the DUT state we want to achieve, just as the previously-used .sh files. I would not call the .sh files transitional as I wouldn't call the .state files transitional.

I also do not understand had would we create the .state files during testplan execution, given that those files are our source of truth when it comes to out desired state. It would be akin to creating a MAVEN file in Java for every compilation - the benefits would be slim and the process unnecessarily onerous.

.. given that those files are our source of truth when it comes to out desired state

right, my question is about these *.state files which are included into this PR, and what the 'desired state' they set.

If the basic goal is to reset the state before each test to defaults, then there is no need to keep these files in the sof-test repo as a 'golden source', but call get_alsactl_state_or_create() once at the test plan beginning right after the DUT deployment stage, and then call restore_settings_via_alsactl() before each test it needs. Isn't it enough ? Did you try/consider to call alsactl init for the same purpose ?

When there is a need to do some custom/experimental state change, it can be done e.g. by a CI script deploying a .state file here from some other repo/source with the platform-specific BKC, or a state file placed here by some test case script during its test plan execution.

You wrote in #1270 (comment)

This approach should only modify the values mentioned in the .state file. So while a typical machine would have a couple dozen values, only the few specified in the .state file will be changed. That should mean this approach is equivalent.

But then in #1270 (comment)

sof and linux builds were taken from that day's CI relevant run. Then I've run the PR with alsa-info print added in. This gave me the full .state file I could then adjust.

When you write "adjust", do you mean "trimmed down to an incremental change, strictly equivalent to the .sh files removed"?

If that's what you meant then that looks OK to me.

@marc-hb Yes, that's exactly what I do - full .state files can have over 60 controls specified, while we oftentimes need less than 10, we I trim them to just what the .sh had specified.

@golowanow The full .state file has all the controls and their current values. However, the current .sh files specify only some controls to some values that can be different from what currently exist on the machine. That's why we do not create them during the runs - we'd be restoring the current machine state rather than restoring the desired machine state.

alsactl init is just alsactl restore on a specific file in a predetermined place. Why obfuscate the files to use rather than keep them in the same repo that uses them? Those aren't multi-gigabyte behemoths to be wary of downloading them unnecessarily.

Why we expect that after alsactl init the DUT's state is wrong and we need some handmade *.state file to set correct defaults ?

I think this PR should evolve from its current 'proof-of-concept' to some runnable test case, then it will be easier to see how it solves the problem and ready for merge.

Why we expect that after alsactl init the DUT's state is wrong and we need some handmade *.state file to set correct defaults ?

As we have experienced tests failing in our current CI operation and we have never used alsactl init's files contained by default in /usr/share/alsa/init/ to restore the DUT state to our liking, why should we assume that alsactl init will bring the DUT to a state we desire? We have no information that would indicate that ALSA's default state is congruent with our desired state. Thusly, we are to use custom .state files, as that's the status quo.

I think this PR should evolve from its current 'proof-of-concept' to some runnable test case, then it will be easier to see how it solves the problem and ready for merge.

In my understanding, this repo houses tests for ALSA configurations and tools to that end. Separate tests for the tools inside the sof-test repo seem missing and it would be awkward to task this PR specifically with establishing a base for tests of sof-test itself.

marc-hb

Good idea!

I don't find the _to_diff suffixes useful. Everything that evolves over time can be "diffed". Name suffixes should describe what the file type and that's all. .alsa_state or something?

marc-hb · 2025-06-29T01:23:31Z

case-lib/lib.sh

+
+    # START DEBUG
+    alsactl store --file /tmp/after_set_alsa_settings_to_diff
+    diff /tmp/after_alsactl_init_to_diff /tmp/after_set_alsa_settings_to_diff


Suggested change

diff /tmp/after_alsactl_init_to_diff /tmp/after_set_alsa_settings_to_diff

diff -u tmp/after_alsactl_init_to_diff /tmp/after_set_alsa_settings_to_diff

marc-hb · 2025-07-01T17:13:31Z

case-lib/lib.sh

 {
+    # DEBUG - to avoid the problem where the state is untouched both by init and .state,
+    # we'll first set the state to something entirely different from the state.
+    if get_alsactl_state "$SCRIPT_HOME" "$PNAME"; then


I think this would be a good opportunity to introduce some new SOF_TEST_DEBUG_LEVEL environment variable. If there is not already something like this yet! Please check.

LukaszMrugala · 2025-07-02T12:15:54Z

I've tested the .state files after init in Run No. 55029.
I have trimmed the .state files so that they contain only the changes that are not in the alsactl init.

marc-hb · 2025-07-02T17:37:28Z

Unsurprisingly, the content of the .state files now looks very similar to the .sh files they replace. This also makes things much easier to compare and review.

Speaking of which: it's not 100% identical. So I think this should proceed in several steps:

bug-for-bug replacement of .sh by .state files. Merge, wait for a few days.
Simplify .state files and drop what duplicates alsactl init.

Keep in mind different people test different ALSA versions.

LukaszMrugala · 2025-07-02T21:55:02Z

Unsurprisingly, the content of the .state files now looks very similar to the .sh files they replace. This also makes things much easier to compare and review.
Speaking of which: it's not 100% identical.

It was identical before trimming. You can verify that trimming has not impacted the .state files' usefulness by verifying the internal CI runs under the label fragment lmrugalx_ of the last week.

bug-for-bug replacement of .sh by .state files. Merge, wait for a few days.

That's extremely slow.

Simplify .state files and drop what duplicates alsactl init.

That's exactly what I have just done.

Keep in mind different people test different ALSA versions.

If different ALSA versions have different init files and thus the initial common state is different between them, then the whole idea of using alsactl init as a common initial state is useless. In that case we cannot trim the .state files at all, as we have no guarantee of anything in the init configuration and we need to carry all of the configuration information that ensures the state for the tests on a given machine inside the .state files. Thus, if different ALSA versions have different inits, then we are back to where we were two weeks ago.

LukaszMrugala · 2025-07-03T07:50:42Z

If different ALSA versions have different init files and thus the initial common state is different between them, then the whole idea of using alsactl init as a common initial state is useless.

I've checked in the repo and the last change to the init files has occured nine years ago. I'd say that's stable enough for us.

It doesn't solve #1275 which needs some MODEL_default.state with full and consistent parameters to restore it on that particular kind of DUT.

PR #1279 solves that.

golowanow · 2025-07-03T09:53:34Z

I have trimmed the .state files so that they contain only the changes that are not in the alsactl init.

ok, thank you

Keep in mind different people test different ALSA versions.

That's the point why we need reset to ALSA defaults to have it consistent with the current version.
The 'trimmed' MODEL.state files are just another way of what is done by MODEL.sh files to align with the DUT's harness.

@LukaszMrugala, please also address these comments - https://github.com/thesofproject/sof-test/pull/1270/files#r2166950950

set_alsa_settings function now shall use the alsactl restore command, which allows for ALSA control restoration based on a configuration file, rather than a bash script. New .state files are equivalent to the old .sh files. Incorporates LNL, MTL and PTL families, save for the nocodecs. Previously-used switch-case statement checks whether a relevant .state file exists, based on the $MODEL variable and uses it if it exists. If not, it uses the previous method as a fallback. Before the aforementioned switch-case, alsactl init is called, so that a common starting point, specific for each machine is achieved. Automatically generated comments inside of the .state files removed for readability. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

Addition of alsactl init made it appear before the initial dlogi call, so we add one before it. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

All tests using ALSA now use a common setup function, setup_alsa, which checks locale, resets volume to 0dB, verifies whether $MODEL has been set and runs set_alsa_settings. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

redzynix

Good job Lukasz, changes looks really good!

redzynix · 2025-07-11T07:48:16Z

SOFCI TEST

golowanow · 2025-07-11T07:48:52Z

case-lib/lib.sh

 set_alsa_settings()
 {
+    # This will bring the machine ALSA state to a common known point - a good baseline
+    alsactl init


I think that we even need it this way to reset before applying MODEL.state or MODEL.sh:

alsactl init # restores ALSA defaults from /var/lib/alsa/asound.state sudo alsactl restore

IMO it is essential to execute alsactl restore to reset properly.

If alsactl init does not recognise the device, it will return error code 99 and bring down the whole test. We do not care for unrecognised devices and will ignore it instead. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

redzynix · 2025-07-11T09:50:49Z

test this please

test this please

Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

LukaszMrugala · 2025-07-11T10:50:06Z

test this please

marc-hb · 2025-07-11T17:09:20Z

test this please

@LukaszMrugala you cannot approve yourself, this is a security feature. You really need to request membership to https://github.com/orgs/thesofproject/teams/sof-developers

EDIT: correction, that sof-developers group is for GitHub while this message is for Jenkins. But the idea is the same.

golowanow · 2025-07-23T14:36:55Z

I've put my suggested changes (which were not fully addressed here yet) as #1284 - It is focused only on alsa_setup as a preparational stage to set the default ALSA state.
Thus, this PR with its proposed .state instead of .sh configuration mechanism can apply its additional settings consistently.

marc-hb · 2025-09-04T01:47:37Z

Has this been superseded by #1284 and #1294? If yes, can it be closed?

LukaszMrugala force-pushed the debug_alsa_info branch from 46cbbee to ca7e8f2 Compare May 12, 2025 09:27

marc-hb reviewed May 12, 2025

View reviewed changes

case-lib/lib.sh Outdated Show resolved Hide resolved

LukaszMrugala force-pushed the debug_alsa_info branch 9 times, most recently from c187c3c to 3368d02 Compare May 20, 2025 14:25

LukaszMrugala force-pushed the debug_alsa_info branch 4 times, most recently from 06de720 to 6d2cda4 Compare May 26, 2025 14:14

LukaszMrugala changed the title ~~[DNM] Add alsa-info debug~~ alsactl DUT restoration Proof of Concept. May 27, 2025

LukaszMrugala marked this pull request as ready for review May 27, 2025 14:05

LukaszMrugala requested review from a team, golowanow and lgirdwood as code owners May 27, 2025 14:05

marc-hb reviewed May 27, 2025

View reviewed changes

LukaszMrugala force-pushed the debug_alsa_info branch from 6d2cda4 to 8f6303f Compare May 28, 2025 09:06

golowanow reviewed May 30, 2025

View reviewed changes

marc-hb reviewed Jun 29, 2025

View reviewed changes

LukaszMrugala marked this pull request as draft July 1, 2025 07:27

marc-hb reviewed Jul 1, 2025

View reviewed changes

LukaszMrugala force-pushed the debug_alsa_info branch from ce4fe55 to 5a8cc29 Compare July 2, 2025 12:14

LukaszMrugala marked this pull request as ready for review July 2, 2025 12:16

LukaszMrugala added 3 commits July 7, 2025 21:34

log: Add dlogi at set_alsa_settings start

f9f2401

Addition of alsactl init made it appear before the initial dlogi call, so we add one before it. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

alsa: Add common setup_alsa in ALSA tests

107aeb3

All tests using ALSA now use a common setup function, setup_alsa, which checks locale, resets volume to 0dB, verifies whether $MODEL has been set and runs set_alsa_settings. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

LukaszMrugala force-pushed the debug_alsa_info branch from dd9de8b to 107aeb3 Compare July 7, 2025 21:37

LukaszMrugala requested review from golowanow and marc-hb July 8, 2025 10:12

redzynix approved these changes Jul 11, 2025

View reviewed changes

redzynix added the improvement The issue or pull request improves already existing functionalities label Jul 11, 2025

golowanow requested changes Jul 11, 2025

View reviewed changes

alsa: remove crash on init

0c76c00

If alsactl init does not recognise the device, it will return error code 99 and bring down the whole test. We do not care for unrecognised devices and will ignore it instead. Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

Fix typo

e208838

Signed-off-by: Lukasz Mrugala <lukaszx.mrugala@intel.com>

This was referenced Aug 1, 2025

ALSA reset #1284

Merged

lib.sh: Set ALSA settings from a state file #1294

Merged

majunkier mentioned this pull request Sep 2, 2025

alsa_settings: WCL platform settings #1298

Merged

		@@ -0,0 +1,7 @@
		# Creating a .state file

		1. Gain access to a machine you'd like to create a state file for.

	diff /tmp/after_alsactl_init_to_diff /tmp/after_set_alsa_settings_to_diff
	diff -u tmp/after_alsactl_init_to_diff /tmp/after_set_alsa_settings_to_diff

alsactl DUT restoration #1270

Are you sure you want to change the base?

alsactl DUT restoration #1270

Uh oh!

Conversation

LukaszMrugala commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sofci commented May 12, 2025

Uh oh!

Uh oh!

marc-hb 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!

LukaszMrugala commented May 28, 2025

Uh oh!

marc-hb commented May 28, 2025

Uh oh!

LukaszMrugala commented May 29, 2025

Uh oh!

marc-hb commented May 29, 2025

Uh oh!

LukaszMrugala commented May 30, 2025

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!

Uh oh!

Uh oh!

Uh oh!

golowanow May 30, 2025 • 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!

marc-hb left a comment

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!

LukaszMrugala commented Jul 2, 2025

Uh oh!

marc-hb commented Jul 2, 2025

Uh oh!

LukaszMrugala commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaszMrugala commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

golowanow commented Jul 3, 2025

Uh oh!

redzynix left a comment

Choose a reason for hiding this comment

Uh oh!

LukaszMrugala commented May 12, 2025 •

edited

Loading

marc-hb left a comment •

edited

Loading

golowanow May 30, 2025 •

edited

Loading

LukaszMrugala commented Jul 2, 2025 •

edited

Loading

LukaszMrugala commented Jul 3, 2025 •

edited

Loading

marc-hb commented Jul 11, 2025 •

edited

Loading