[WIP][RFC] Test-case: Set PGAs to unity gain in script check-alsabat.sh #437

singalsu · 2020-10-14T16:49:53Z

This patch adds find of volume controls for used playback and capture
devices and sets them to unity gain (0 dB). The alsabat test fails
if e.g. capture PGA is set to their maximum value that can be up
to +30 dB. The test sine wave distorts and causes test fail.

Signed-off-by: Seppo Ingalsuo [email protected]

marc-hb

Interestingly https://sof-ci.01.org/softestpr/PR437/build271/devicetest failed to even start this test on all platforms except BSW_CYN_MAX98090 (which tends to have other, unrelated and severe eMMC issues, I digress).

It suspect the test fails to grep some (PGA?) control and then does not start because of set -e, did you expect that? Either way I think you should add more explicit error handling because grep does not print anything when it fails to find anything, in other words:

scale=$(amixer cget name="$cname" | grep "dBscale" || true)
test -n "$scale" || die "dbScale $cname not found"

test-case/check-alsabat.sh

singalsu · 2020-10-15T16:06:58Z

I'm confused why the script fails in every device in CI. This version uses "set -x".

singalsu · 2020-10-15T16:44:18Z

@marc-hb @xiulipan Now I think I know what's the problem. The CI system runs the script with command
TPLG=sof-hda-generic-4ch.tplg ~/sof-test/test-case/check-alsabat.sh -p hw:0,0 -c hw:1,0
It fails because sof-tplgreader.py can't find the file without proper path. Shouldn't the convention to be to have full path to topology file in TPLG environment variable?
/home/ubuntu/sof-test/tools/sof-tplgreader.py sof-hda-generic-4ch.tplg

singalsu · 2020-10-15T16:56:53Z

This would be simple to fix by testing with appended default path if topology file does not exist but I don't feel that is the proper thing to do. The call of this script does not seem to be in this repository.

aiChaoSONG · 2020-10-16T02:19:18Z

@singalsu The bash function func_lib_get_tplg_path in lib.sh helps to get topology full path, whatever a full path or only topology name is specified, because the /lib/firmware/intel/sof-tplg is the default folder for TPLG on every computer that uses SOF. But the sof-tplgreader.py is a general tool, developer may not put the topology in /lib/firmware/intel/sof-tplg, so cannot suppose there is a default folder. I think this makes sense.

Here‘s what you can do：
tplg_full_path=$(func_lib_get_tplg_path $tplg)

marc-hb

tplg_full_path=$(func_lib_get_tplg_path $tplg)

+1

marc-hb · 2020-10-15T16:58:59Z

test-case/check-alsabat.sh

+	return
+    fi
+
+    for pga in $1; do


From a quoting perspective please make this function whitespace- and globbing- (*?) safe with "$@" like this:

for pga in "$@" ...

then you invoke it below without quotes:

CAP_PGAS=$($TPLGREADER "$tplg" -f "snd:$cap_snd" -d pga -v) # this could need a "I know what I'm doing" shellcheck disable= here set_pgas_list_to_unity_gain $CAP_PGAS

This keeps the function clean and concentrates whitespace, globbing and shellcheck issues closer to their source (= the TPLGREADER output)

https://google.github.io/styleguide/shellguide.html#s6.7-arrays

Using a single string for multiple command arguments should be avoided, as it inevitably leads to authors using eval or trying to nest quotes inside the string, which does not give reliable or readable results and leads to needless complexity.

This is confusing, but let's try.

Shellcheck complains about line set_pgas_list_to_unity_gain $CAP_PGAS with SC2086: Double quote to prevent globbing and word splitting. It works OK though.

Yes, that's expected because you should ideally not do that either. You should ideally never use a single string to store multiple, whitespace-separated strings. My suggestion that you applied is only "the lesser of two evils", my suggestion was to do this bad thing only in a single place (here) keeping the set_pgas_list_to_unity_gain() function clean. Thanks for applying it.

So, ideally CAP_PGAS should be converted to an array. In practice we're going to assume that the output of $TPLGREADER is safe and you can tell shellcheck that with this:

CAP_PGAS=$($TPLGREADER "$tplg" -f "snd:$cap_snd" -d pga -v) # Let's assume the output of $TPLGREADER is free of *, other special characters and spurious spaces. # shellcheck disable=SC2086 set_pgas_list_to_unity_gain $CAP_PGAS

test-case/check-alsabat.sh

xiulipan · 2020-10-16T07:45:01Z

@singalsu I think this would be good for nocodec test case. What about the codec cases? There maybe some volume control in codec. How could we handle those values for test?
PS: we also used this scripts for the USB codec, which may not be covered by TPLG.

@keqiaozhang Any comments here for codec test cases? I think in CI framework, we manually tuned each volume gain value to make sure we did not get peak off in record wavs.

Maybe we can set all value to 0db, so ideally we should get almost same wav we played.

singalsu · 2020-10-16T08:30:36Z

@xiulipan The codec based (analog or digital) gains are better to set up per device to be suitable for the (usually hw:1,0) USB sound card and leave them that way. Since the script gets with this patch the PGAs names from topology it fortunately does not alter them at all. In some cases setting codec gain to 0 dB could possibly "overload" the USB sound card A/D converter.

There could be another script to automatically set the device D/A and USB A/D gains if adjustable via SW. The procedure would optimize THD+N performance of playback-capture loop.

marc-hb · 2020-10-16T16:50:26Z

test-case/check-alsabat.sh

+    done
+}
+
+get_snd_base()


get_snd_dev()?

marc-hb · 2020-10-16T16:50:52Z

test-case/check-alsabat.sh

+    echo "/dev/snd/pcmC${ncard}D${ndevice}"
+}
+
+get_play_snd()


get_play_snd_dev()?

marc-hb · 2020-10-16T16:57:54Z

test-case/check-alsabat.sh

+	return
+    fi
+
+    for pga in $1; do


Yes, that's expected because you should ideally not do that either. You should ideally never use a single string to store multiple, whitespace-separated strings. My suggestion that you applied is only "the lesser of two evils", my suggestion was to do this bad thing only in a single place (here) keeping the set_pgas_list_to_unity_gain() function clean. Thanks for applying it.

So, ideally CAP_PGAS should be converted to an array. In practice we're going to assume that the output of $TPLGREADER is safe and you can tell shellcheck that with this:

CAP_PGAS=$($TPLGREADER "$tplg" -f "snd:$cap_snd" -d pga -v) # Let's assume the output of $TPLGREADER is free of *, other special characters and spurious spaces. # shellcheck disable=SC2086 set_pgas_list_to_unity_gain $CAP_PGAS

keqiaozhang · 2020-10-19T02:00:47Z

@keqiaozhang Any comments here for codec test cases? I think in CI framework, we manually tuned each volume gain value to make sure we did not get peak off in record wavs.

Maybe we can set all value to 0db, so ideally we should get almost same wav we played.

I agree to set all PGA value to 0dB. But if we do this, I need to retune the related codec kcontrols(like headset volume, DAC volume etc.) for the old platforms we added before to make sure the waveform does not overflow.

singalsu · 2020-10-19T09:16:16Z

@keqiaozhang Any comments here for codec test cases? I think in CI framework, we manually tuned each volume gain value to make sure we did not get peak off in record wavs.
Maybe we can set all value to 0db, so ideally we should get almost same wav we played.

I agree to set all PGA value to 0dB. But if we do this, I need to retune the related codec kcontrols(like headset volume, DAC volume etc.) for the old platforms we added before to make sure the waveform does not overflow.

Does it mean that the firmware PGAs have been intentionally set to other than 0 dB gain to compensate for some (non-sufficient?) codec gain controls?

I made this PR because I noticed that the alsabat test failed when the capture pipeline was changed to other type (pipe-volume-switch-capture.m4) that has higher max. gain available. So, I mean setting the device for some defaults and relying it to boot with it correctly is rather fragile if like in this case a topology changes.

singalsu · 2020-10-19T13:03:13Z

SOFCI TEST

singalsu · 2020-10-19T14:26:57Z

@keqiaozhang There's now two timeouts remaining, one for a CML platform and one for a GLK platform. Those seem to be due to excessive signal level (distortion, multiple sine waves detected). I wonder if I should try a lower digital gain for all platforms. Or can the analog DAC or headphone gains be tuned down in those platforms to enable unity digital gains to pass? The latter would be preferred if possible.

keqiaozhang · 2020-10-21T09:18:40Z

@keqiaozhang There's now two timeouts remaining, one for a CML platform and one for a GLK platform. Those seem to be due to excessive signal level (distortion, multiple sine waves detected). I wonder if I should try a lower digital gain for all platforms. Or can the analog DAC or headphone gains be tuned down in those platforms to enable unity digital gains to pass? The latter would be preferred if possible.

amixer setting for GLK:
amixer -c sofglkda7219max cset name='Headphone Gain Ramp Switch' 1
amixer -c sofglkda7219max cset name='Headphone Volume' 35 35
amixer -c sofglkda7219max cset name='Headphone Switch' 1
amixer -c sofglkda7219max cset name='Headphone ZC Gain Switch' 0
amixer -c sofglkda7219max cset name='Headphone Jack Switch' 1
amixer -c sofglkda7219max cset name='Playback Digital Volume' 111 111
amixer -c sofglkda7219max cset name='Playback Digital Switch' 1
amixer -c sofglkda7219max cset name='Playback Digital Gain Ramp Switch' 1
amixer -c sofglkda7219max cset name='Out DACR Mux' 3
amixer -c sofglkda7219max cset name='Out DAIR Mux' 0
amixer -c sofglkda7219max cset name='Mixer Out FilterL DACL Switch' 1
amixer -c sofglkda7219max cset name='Mixer Out FilterR DACR Switch' 1 10
amixer -c sofglkda7219max cset name='PGA1.0 1 Master Playback Volume' 32

For CML, which device has the timeout issue?

This patch adds find of volume controls for used playback and capture devices and sets them to unity gain (0 dB). The alsabat test fails if e.g. capture PGA is set to their maximum value that can be up to +30 dB. The test sine wave distorts and causes test fail. Signed-off-by: Seppo Ingalsuo <[email protected]>

marc-hb · 2021-01-26T19:49:48Z

There is a relatively small number of shellcheck warnings left and most of them seem easy to fix: just missing quotes. While you're testing this script, @singalsu would you mind applying the most obvious shellcheck's suggestions in a separate commit? Don't mind any complex warning or complex quoting issues, any progress is useful.

marc-hb

Just curious: would some UCM make all this easier to solve? I know @plbossart has been complaining about alsa settings since forever.

marc-hb · 2021-01-26T19:58:34Z

test-case/check-alsabat.sh

 # check the PCMs before alsabat test
 dlogi "check the PCMs before alsabat test"
 aplay   -Dplug$pcm_p -d 1 /dev/zero -q || die "Failed to play on PCM: $pcm_p"
 arecord -Dplug$pcm_c -d 1 /dev/null -q || die "Failed to capture on PCM: $pcm_c"

+# Set PGAs for PCMs to 0 dB value
+test -n "$(command -v "$TPLGREADER")" ||
+    die "Command $TPLGREADER is not available."


Note #471 Remove sof-test dependency on topology file, use /proc instead (but I have no idea how it would work here, this is just FYI)

marc-hb · 2021-01-26T20:00:32Z

test-case/check-alsabat.sh

+    # Get multiplied by 100 values by removing decimal dot
+    min_x100="${min_db//.}"
+    step_x100="${step_db//.}"
+    val=$(printf %d "$(((-min_x100) / step_x100))")


Nit, this would be slightly easier to read with some spaces:

val=$(printf %d "$(( (-min_x100) / step_x100))" )

Why do you need the printf at all?

marc-hb · 2021-01-26T20:01:28Z

test-case/check-alsabat.sh

+    # Get volume min and step to compute value for cset
+    # for 0 dB gain. The amixer line looks like
+    # "| dBscale-min=-50.00dB,step=1.00dB,mute=1"
+    scale=$(amixer cget name="$cname" | grep "dBscale" || true)


|| true means an empty $scale is OK. Is it really?

marc-hb · 2021-01-26T20:02:20Z

test-case/check-alsabat.sh

@@ -72,11 +79,83 @@ function __upload_wav_file
    done
 }

+set_pga_to_unity_gain()
+{
+    tmp=$(amixer controls | grep "$1" | grep Volume)


As you use very generic variable names, this first line will avoid any collision:

local tmp search cname scale ...

xiulipan · 2021-01-27T06:14:14Z

Just curious: would some UCM make all this easier to solve? I know @plbossart has been complaining about alsa settings since forever.

@singalsu @keqiaozhang @libinyang I remember you said the volume value can not be reset by UCM right? Please confirm here

marc-hb · 2021-06-22T04:41:34Z

@singalsu did you forget about this? :-)

plbossart · 2021-06-24T23:12:59Z

@singalsu is this still relevant or should we close this one?

marc-hb · 2021-11-01T21:19:45Z

@singalsu please re-open when you get a chance.

marc-hb · 2024-04-22T21:34:13Z

test-case/check-alsabat.sh

+tplg_full_path=$(func_lib_get_tplg_path "$tplg")
+dlogi "Getting playback PGA information"
+play_snd=$(get_play_snd "$pcm_p")
+PLAY_PGAS=$($TPLGREADER "$tplg_full_path" -f "snd:$play_snd" -d pga -v)


-d pga seems to be compatible with topology v1 only, same problem as in #1068. @ranj063 confirm?

are we talking about the string "pga" in the kcontrol name here? Then ~~yes it is compatible with the tplg2 as well~~. Take a look at the gain kcontrols in the nocodec tplg https://sof-ci.01.org/linuxpr/PR4937/build2360/devicetest/index.html?model=TGLU_RVP_NOCODEC-ipc4&testcase=verify-tplg-binary

When run on topology v1 files, tools/sof-tplgreader.py shows a field named pga. Example:

./tools/sof-tplgreader.py /lib/firmware/intel/sof-tplg/sof-apl-nocodec.tplg -d pga pga=PGA9.0 PGA5.0; pga=PGA6.0; pga=PGA11.0 PGA5.0;

Note the blank lines stand for DMIC and other PCMs without a pga= field.

What would be the equivalent for topology v2? Is there a v2 compatibility issue with sof-tplgreader.py?

Take a look at the gain kcontrols in the nocodec tplg https://sof-ci.01.org/linuxpr/PR4937/build2360/devicetest/index.html?model=TGLU_RVP_NOCODEC-ipc4&testcase=verify-tplg-binary

This device has at least 15 amixer controls with "Volu.." in the name (name= which may or may not be truncated). That's the list of kcontrols volume tests want, correct?

That log shows a list of pipelines, 3 of them which have cap_name=Gain Capture.

I don't see the connection.

@marc-hb i see your problem. Sorry I misundertood the PGA in the picture. Looks like that's the tplgtool printing that the PGA prefix for the kcontrols.

So, yes there's no "pga" string in tplg2 kcontrol names. Infact we consciously removed the widget name from the kcontrol name as well so you have "gain" either. So the question is how to figure out which kcontrols to test?

Can we do this during tplg parsing ie when as we parse the gain widgets, save the kcontrol names for the kcontrols associated with the widget and test only those?

as we parse the gain widgets, save the kcontrol names for the kcontrols associated with the widget and test only those?

Sounds like a plan. Let me take a look.

EDIT: done in

volume-basic-test.sh: switch to .tplg parsing to test topologies v2 #1193

singalsu requested review from keqiaozhang, marc-hb and aiChaoSONG October 14, 2020 16:49

marc-hb reviewed Oct 14, 2020

View reviewed changes

test-case/check-alsabat.sh Outdated Show resolved Hide resolved

test-case/check-alsabat.sh Outdated Show resolved Hide resolved

test-case/check-alsabat.sh Show resolved Hide resolved

singalsu mentioned this pull request Oct 15, 2020

Topology: Use pipe-volume-switch-capture in nocodec topologies thesofproject/sof#3509

Closed

singalsu force-pushed the add_to_alsabat_pga_set branch 4 times, most recently from 45fe2f0 to 68e731d Compare October 15, 2020 16:05

marc-hb requested changes Oct 16, 2020

View reviewed changes

singalsu force-pushed the add_to_alsabat_pga_set branch 2 times, most recently from f41328f to 9279531 Compare October 16, 2020 13:01

marc-hb reviewed Oct 16, 2020

View reviewed changes

singalsu force-pushed the add_to_alsabat_pga_set branch from 9279531 to 1b5c3c0 Compare January 26, 2021 13:53

singalsu mentioned this pull request Jan 26, 2021

Script check-alsabat.sh fails with capture pipeline with volume scale max above 0 dB #436

Open

singalsu force-pushed the add_to_alsabat_pga_set branch from 1b5c3c0 to 6e14cdf Compare January 26, 2021 15:35

marc-hb reviewed Jan 26, 2021

View reviewed changes

fredoh9 self-requested a review February 22, 2021 16:37

marc-hb added the type:test coverage gap This requires a new test case, not just fixing one label Jul 3, 2021

marc-hb closed this Nov 1, 2021

marc-hb reopened this Jun 22, 2023

marc-hb mentioned this pull request Jan 2, 2024

Simple dependency-free ALSA test rig for PCM capture analysis. thesofproject/sof#8681

Closed

marc-hb reviewed Apr 22, 2024

View reviewed changes

marc-hb mentioned this pull request May 9, 2024

volume-basic-test.sh: don't skip "playback volume" controls #1068

Closed

[WIP][RFC] Test-case: Set PGAs to unity gain in script check-alsabat.sh #437

Are you sure you want to change the base?

[WIP][RFC] Test-case: Set PGAs to unity gain in script check-alsabat.sh #437

Conversation

singalsu commented Oct 14, 2020

marc-hb left a comment

Choose a reason for hiding this comment

singalsu commented Oct 15, 2020

singalsu commented Oct 15, 2020 • edited Loading

singalsu commented Oct 15, 2020

aiChaoSONG commented Oct 16, 2020 • edited Loading

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

xiulipan commented Oct 16, 2020

singalsu commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keqiaozhang commented Oct 19, 2020 • edited Loading

singalsu commented Oct 19, 2020 • edited Loading

singalsu commented Oct 19, 2020

singalsu commented Oct 19, 2020

keqiaozhang commented Oct 21, 2020

marc-hb commented Jan 26, 2021

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

xiulipan commented Jan 27, 2021

marc-hb commented Jun 22, 2021

plbossart commented Jun 24, 2021

marc-hb commented Nov 1, 2021

Choose a reason for hiding this comment

ranj063 Apr 22, 2024 • edited by marc-hb 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

marc-hb Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

singalsu commented Oct 15, 2020 •

edited

Loading

aiChaoSONG commented Oct 16, 2020 •

edited

Loading

keqiaozhang commented Oct 19, 2020 •

edited

Loading

singalsu commented Oct 19, 2020 •

edited

Loading

ranj063 Apr 22, 2024 •

edited by marc-hb

Loading

marc-hb Apr 26, 2024 •

edited

Loading