sof-test: read /proc instead of parsing TPLG file for legacy HDA platforms #430

aiChaoSONG · 2020-10-13T02:48:33Z

This PR enhance sof-test for legacy HDA platform. The main idea is to export pipeline parameters from proc file system, just like exporting pipeline parameters from topology.

plbossart · 2020-10-13T17:31:28Z

case-lib/lib.sh

@@ -264,3 +264,12 @@ sudo sync -f || true
 if [ ! "$DMESG_LOG_START_LINE" ]; then
    DMESG_LOG_START_LINE=$(wc -l /var/log/kern.log|awk '{print $1;}')
 fi
+
+is_sof_platform()


this should be is_hda_legacy_platform()

and have additional checks for the PCI ID.

I think we should make this as is_sof_used or is_hda_legacy_used as we can switch to use both on same platform.

now I use /proc/aound/cards to tell if we are using SOF, if yes, dump pipelines from tplg, if not, dump pipelines from proc, it's much more dependable than just check the kernel module.

case-lib/pipeline.sh

marc-hb

that looks absolutely horrible to me.

Agree that generating shell script code from Python just to pass some values is quite ugly[*]. What @plbossart may have not noticed: @aiChaoSONG didn't create this but copied most of it from tools/sof-tplgreader.py and case-lib/pipelline.sh.

@aiChaoSONG one copy of this code is already painful enough, so Duplicating and Diverging a second time it is really not acceptable from a longer term maintenance perspective. Please submit first a pure refactoring where you only move the parts you need to some common files with no logical change. Once it's merged you can submit a second and much smaller PR for legacy HDA that re-uses existing code.

Note you will need to prepare the second PR before submitting the first but don't share the second PR until the first PR is merged so we can leave a few days between merging the two (for "continuous" integration) and to make the review easier.

[*] EDIT: a bit less ugly would be to source a temporary file. I couldn't think of any "cleaner" solution. Off-topic sorry.

case-lib/lib.sh

marc-hb · 2020-10-13T17:45:30Z

tools/sof-dump-status.py

@@ -359,6 +381,7 @@ def dump_dapm(dapm, filter = "all"):
    parser.add_argument('-P', '--fwpath', action='store_true', help='get firmware path according to DMI info')
    parser.add_argument('-S', '--dsp_status', type=int, help='get current dsp power status, should specify sof card number')
    parser.add_argument('-d', '--dapm', choices=['all', 'on', 'off', 'standby'], help='get current dapm status, this option need root permission to access debugfs')
+    parser.add_argument('-e', '--export', action='store_true', help='export pipeline parameters from proc file system')


I guess it would be good to very briefly mention legacy HDA here.

tools/sof-dump-status.py

xiulipan · 2020-10-15T08:02:29Z

case-lib/lib.sh

@@ -264,3 +264,12 @@ sudo sync -f || true
 if [ ! "$DMESG_LOG_START_LINE" ]; then
    DMESG_LOG_START_LINE=$(wc -l /var/log/kern.log|awk '{print $1;}')
 fi
+
+is_sof_platform()


I think we should make this as is_sof_used or is_hda_legacy_used as we can switch to use both on same platform.

aiChaoSONG · 2020-10-20T05:45:50Z

a bit less ugly would be to source a temporary file. I couldn't think of any "cleaner" solution

@marc-hb I think this idea is good to eliminate the python generated bash code. But currently, I am working on enable Zephyr based SOF builds in CI, I think for the code refactoring part, I will file an issue, and return later, or we can let @RuiqingX take part in this.
In fact, the pipeline filtering feature in sof-tplgreader.py is not enabled for pipelines dumped from proc, this can also be done when extract shared code into a new library file.

marc-hb · 2020-10-20T23:20:56Z

I think this idea is good to eliminate the python generated bash code. But currently, I am working on enable Zephyr based SOF builds in CI, I think for the code refactoring part, I will file an issue,

OK let's stop discussing changing this code.

On the other hand, copy/paste is still not acceptable and copy/paste of dubious code is even less acceptable. This creates a lot of difficult work for later because de-duplication requires a lot more work and a lot of testing.

Don't change the existing code but move it to another place (any place) where it can be imported and not duplicated.

aiChaoSONG · 2020-10-21T03:12:27Z

move it to another place (any place) where it can be imported and not duplicated

@marc-hb I move the common code to common.py, and there is no duplicated code, should be less painful.

marc-hb

Much cleaner, thanks. Nice git commit breakdown. Did you really test every commit? :-)

case-lib/lib.sh

case-lib/pipeline.sh

marc-hb · 2020-10-22T00:56:42Z

test-case/check-capture.sh

@@ -75,6 +75,10 @@ do
        type=$(func_pipeline_parse_value $idx type)
        snd=$(func_pipeline_parse_value $idx snd)

+        # Filtering feature is not enabled for pipelines dumped from proc
+        # This workaround is need for this case to run on legacy HDA platform.


Remove the filter on line 64. This will upgrade this "workaround" to a feature, will keep everything the same/consistent/simpler and then you can remove all the puzzling comments.

We cannot remove the filter here, because an empty filter means all pipelines, and filter "-f type:any" means all pipelines except WOV and Echo Reference, we cannot run arecord on these pipelines directly. I will change the filter to "-f type:any", remove the puzzling comments, this also make this a FEATURE.

test-case/check-capture.sh

test-case/check-pause-resume.sh

tools/common.py

marc-hb · 2020-10-22T01:03:43Z

SOFCI TEST

marc-hb · 2020-10-22T01:05:02Z

About the massive https://sof-ci.01.org/softestpr/PR430/build305/devicetest/ failures see #458

aiChaoSONG · 2020-10-22T03:11:16Z

@marc-hb Yes, I have test the commits on one HDA platform and a SOF platform, both work well. All reviews are addressed

marc-hb

I find the code much cleaner now, thanks @aiChaoSONG ! @xiulipan / @plbossart do you mind reviewing again?
This review could also use some topology experts, @ranj063 , @fredoh9 , others?

@marc-hb Yes, I have test the commits on one HDA platform and a SOF platform, both work well.

I meant test each of the 6 commits individually. If you've done that then I'm impressed!

case-lib/pipeline.sh

test-case/test-speaker.sh

aiChaoSONG · 2020-10-22T05:54:14Z

@marc-hb Yes, even the renaming commit.

xiulipan

The biggest problem to me is why to more the filter feature outside? Try to merge the feature in the function and keep minor change is always good idea.

case-lib/pipeline.sh

xiulipan · 2020-10-22T07:40:53Z

test-case/check-capture.sh

@@ -61,7 +61,7 @@ file_prefix=${OPT_VALUE_lst['f']}

 func_lib_setup_kernel_last_line
 func_lib_check_sudo
-func_pipeline_export $tplg "type:capture & ${OPT_VALUE_lst['S']}"
+func_pipeline_export $tplg "type:any & ${OPT_VALUE_lst['S']}"


Why not relay on the func_pipeline_export to do the type filter? I think make check to each end user is not a good idea. I think this would be a degenerate for our function.

The pipeline filtering feature is provided by sof-tplgreader.py, and the filtering feature is not implemented for sof-dump-status.py. If we need a common filtering feature shared between both, it requires a lot of code change, because the filter code in the sof-tplgreader.py is in a class, and requires self, which need some efforts to refactor.

This maybe done later

What's more, we don't need such a complicated filter for proc pipelines, I doubt if it worth the effort.

I checked the output from, I think add a simple filter for playback/capture will be very easy. (a simple grep will make it work)

cat /proc/asound/pcm 00-00: Port5 (*) : : playback 1 : capture 1 00-01: DMIC (*) : : capture 1 00-02: DMIC16kHz (*) : : capture 1 00-04: Media Playback 4 (*) : : playback 1 00-05: HDMI1 (*) : : playback 1 00-06: HDMI2 (*) : : playback 1 00-07: HDMI3 (*) : : playback 1

PS: to me a patch that influence so many files is also a big risk. Changing API is a dangerous thing and need more test. So I would prefer to keep old ABI and add simple implement for the legacy HDA.

Assuming the format of /proc/asound/pcm is stable and guaranteed then a simple grep in func_pipeline_export() seems like a cheap and good idea to me.

PS: to me a patch that influence so many files is also a big risk.

There is 1 new file which is just extracted from an existing one and only 4 other modified files. One of the modified file is just getting a one-line function.

Changing API is a dangerous thing and need more test. So I would prefer to keep old ABI

What specific API and ABI are changing here?

and add simple implement for the legacy HDA.

I don't get this sorry, can you elaborate?

plbossart

this can be improved quite a bit.

tools/sof-dump-status.py

case-lib/pipeline.sh

plbossart · 2020-10-22T20:14:43Z

Another question i have is how would you deal with the TPLG env variable for legacy HDaudio? Is this even relevant?

tools/common.py

No functional change, only move code here and there, and add some comment. Signed-off-by: Amery Song <chao.song@intel.com>

This patch renames func_dump_pipeline to format_pipeline, and renames func_export_pipeline to export_pipeline. Signed-off-by: Amery Song <chao.song@intel.com>

To reuse test cases from sof-test repo, we need to dump pipeline info from proc, just like dump pipeline info from topology on SOF platform. As there is no format/channel/rate information in proc, we hard code some default format/channel/rate values, this should be enough for legacy HDA platform test. Signed-off-by: Amery Song <chao.song@intel.com>

tools/sof-dump-status.py

case-lib/pipeline.sh

marc-hb · 2020-10-23T16:58:36Z

case-lib/pipeline.sh

+# - sof-tplgreader.py (for SOF, pipeline parameters dumped from topology)
+# - sof-dump-status.py (for legacy HDA, pipeline paramters dumped from proc)
+# Args: $1: SOF topology path
+#       $2: Pipeline filter string


So @plbossart was right, you need a topology path even when is_sof_used_ is false? Which one?

I thought this function took different arguments depending on is_sof_used

@marc-hb @plbossart On legacy HDA, if you manually run test case, no TPLG is required. But CI will give a fake topology, as you can see from any report, this is how CI run a test case: TPLG=sof-hda-generic-4ch-kwd.tplg ~/sof-test/test-case/verify-tplg-binary.sh. Some compromise with sof-framework here. Here is how func_pipeline_export called in test case: func_pipeline_export $tplg "type:capture". so the filter string may be $1 (tplg is empty if manual run) or $2 (CI always give tplg), that's the reason for "$@"["$#"]

func_pipeline_export $tplg "type:capture". so the filter string may be $1 (tplg is empty if manual run) or $2 (CI always give tplg

This is really missing from the function header!

That's yet another demonstration that almost every parameter expansion must be quoted. Maybe it would be a too big change but with func_pipeline_export "$tplg" "type:capture" then $1 would be "" when $tplg is missing, the function would get a constant number of arguments and the filter would always be in $2.

that's the reason for "$@"["$#"]

... which doesn't seem to do what I understand you want. How was this tested!?

I have run the test case on a legacy HDA platform, and an SOF platform with TPLG, both passed!

Maybe it would be a too big change but with func_pipeline_export "$tplg" "type:capture" then $1 would be "" when $tplg is missing, the function would get a constant number of arguments and the filter would always be in $2.

I just check some test case, no quote for $tplg. And after adding quote, the filter is $2. My bad, let's forget this "$@"["$#"] story. I will check every test case, and add missing quote for $tplg.
@marc-hb And also, I will try to add more comment to my code, really appreciate it, and also thanks Pierre and Ranjani

Maybe a broken filter_str does not matter?

@marc-hb I am going to drop the "$@"["$#"] thing, and add quote in every test case to $tplg, thus make sure the filter_str is always $2. So we don't need to think about how to get the last function argument in shell.

UPDATE: submitted in #475

add quote in every test case to $tplg

Sounds good. You probably want to merge that first and separately to avoid a "Big Bang Integration" (the opposite of Continuous Integration)

@marc-hb @plbossart On legacy HDA, if you manually run test case, no TPLG is required. But CI will give a fake topology, as you can see from any report, this is how CI run a test case: TPLG=sof-hda-generic-4ch-kwd.tplg ~/sof-test/test-case/verify-tplg-binary.sh. Some compromise with sof-framework here. Here is how func_pipeline_export called in test case: func_pipeline_export $tplg "type:capture". so the filter string may be $1 (tplg is empty if manual run) or $2 (CI always give tplg), that's the reason for "$@"["$#"]

we really need to stop thinking about 'fake topologies'. Don't do it and use /proc. it'll be simpler in the end.

I find that "fake topologies" sound bad too. On the other hand, an empty, dummy HDA topology file would be OK if needed to keep sof-test APIs consistent or for some transition period. Such a dummy HDA topology should make any test FAIL FAST if the test tries using it by mistake.

@plbossart @marc-hb Agreed. I think this requires some change in the CI framework, I will check.

ranj063 · 2020-10-23T21:15:48Z

@plbossart @aiChaoSONG @marc-hb this PR is in the right direction. I think we should decouple sof-test from the topology file even when testing with the SOF driver and ... moved to #471

tools/common.py

marc-hb

I think we should decouple sof-test from the topology file even when testing with the SOF driver and use the procfs

Interesting but please file a new github issue (done in #471) to discuss such a major sof-test design change otherwise this PR will become totally unreadable (it already has a very large number of comments)

that's not a good-enough reason to screw-up the card name parsing.

See also similar Auxiliary devices in SOF break SOFCARD #466 (thx @ranj063 for the link)

case-lib/pipeline.sh

plbossart · 2020-10-24T03:15:31Z

@plbossart @aiChaoSONG @marc-hb this PR is in the right direction. I think we should decouple sof-test from the topology file even when testing with the SOF driver and ... transferred to #471

Agree that if we can use the /proc info then we have all the information needed for tests with PCM devices.
For probes it's actually the only solution since ... transferred to #471

marc-hb · 2020-10-24T04:40:17Z

Agree that if we can use the /proc info then we have all the information needed for tests with PCM devices.

This PR implies the exact opposite.

Discussion transferred to new issue #471 Remove sof-test dependency on topology file

marc-hb

This looks OK except it now depends on (part of) #475 to be merged first if I understood correctly

case-lib/pipeline.sh

tools/sof-dump-status.py

We don't need a complex filter for proc pipeline as topology pipeline does. So a simple type filter for proc pipeline is implemented in this patch. This makes it possible for some test cases to run under legacy HDA platform without any script change. Signed-off-by: Amery Song <chao.song@intel.com>

This patch adds is_sof_used function to check we are running test on SOF platform or legacy HDA platform. Signed-off-by: Amery Song <chao.song@intel.com>

aiChaoSONG · 2020-10-30T05:45:37Z

SOFCI TEST

aiChaoSONG · 2020-11-03T02:49:10Z

@xiulipan Ping!

marc-hb

One minor change to keep pipeline.sh shellcheck clean otherwise the code style LGTM

case-lib/pipeline.sh

Signed-off-by: Amery Song <chao.song@intel.com>

code style LGTM, need others to approve the logic.

marc-hb · 2020-11-04T16:47:30Z

The aplay: pcm_write:2061: write error: Input/output error in https://sof-ci.01.org/softestpr/PR430/build355/devicetest/ is in only one test (check-playback-3rounds) on only one system (sh-bdw-wsb-rt286-01) so I can't see how it could be related to this.

xiulipan

LGTM, CI error is known issue on legacy platforms

aiChaoSONG requested a review from a team as a code owner October 13, 2020 02:48

plbossart requested changes Oct 13, 2020

View reviewed changes

marc-hb requested changes Oct 13, 2020

View reviewed changes

marc-hb mentioned this pull request Oct 13, 2020

[BYT][NOCODEC] alsa-bat check-alsabat-headset-playback.sh FAIL #379

Closed

xiulipan suggested changes Oct 15, 2020

View reviewed changes

aiChaoSONG force-pushed the proc_pipeline branch from 27ca77d to 37bc48b Compare October 20, 2020 05:21

aiChaoSONG force-pushed the proc_pipeline branch from 37bc48b to 3cb61c3 Compare October 20, 2020 06:15

aiChaoSONG force-pushed the proc_pipeline branch 2 times, most recently from d827257 to 421347d Compare October 21, 2020 03:10

marc-hb requested changes Oct 22, 2020

View reviewed changes

aiChaoSONG force-pushed the proc_pipeline branch from 421347d to 5468c72 Compare October 22, 2020 03:08

marc-hb requested review from marc-hb, fredoh9 and ranj063 October 22, 2020 05:45

marc-hb reviewed Oct 22, 2020

View reviewed changes

case-lib/pipeline.sh Show resolved Hide resolved

test-case/test-speaker.sh Outdated Show resolved Hide resolved

marc-hb requested review from marc-hb and lyakh October 22, 2020 05:47

aiChaoSONG force-pushed the proc_pipeline branch 2 times, most recently from 47241e0 to f16499e Compare October 22, 2020 06:28

xiulipan suggested changes Oct 22, 2020

View reviewed changes

aiChaoSONG force-pushed the proc_pipeline branch from f16499e to dd2ac80 Compare October 22, 2020 10:32

plbossart requested changes Oct 22, 2020

View reviewed changes

tools/sof-dump-status.py Outdated Show resolved Hide resolved

tools/sof-dump-status.py Show resolved Hide resolved

tools/sof-dump-status.py Show resolved Hide resolved

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

case-lib/pipeline.sh Show resolved Hide resolved

ranj063 reviewed Oct 23, 2020

View reviewed changes

tools/common.py Show resolved Hide resolved

Amery Song added 3 commits October 23, 2020 13:11

tools: move shared python functions to common.py

0952b96

No functional change, only move code here and there, and add some comment. Signed-off-by: Amery Song <chao.song@intel.com>

tools: rename func_dump_pipeline and func_export_pipeline

796de48

This patch renames func_dump_pipeline to format_pipeline, and renames func_export_pipeline to export_pipeline. Signed-off-by: Amery Song <chao.song@intel.com>

aiChaoSONG force-pushed the proc_pipeline branch from dd2ac80 to 426140a Compare October 23, 2020 06:30

marc-hb requested changes Oct 23, 2020

View reviewed changes

ranj063 reviewed Oct 23, 2020

View reviewed changes

tools/common.py Outdated Show resolved Hide resolved

marc-hb requested changes Oct 23, 2020

View reviewed changes

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

marc-hb mentioned this pull request Oct 24, 2020

Remove sof-test dependency on topology file, use /proc instead #471

Closed

aiChaoSONG force-pushed the proc_pipeline branch from 426140a to fe508fc Compare October 27, 2020 07:47

marc-hb reviewed Oct 27, 2020

View reviewed changes

case-lib/pipeline.sh Show resolved Hide resolved

tools/sof-dump-status.py Outdated Show resolved Hide resolved

marc-hb self-requested a review October 27, 2020 23:38

Amery Song added 2 commits October 28, 2020 10:19

lib.sh: add is_sof_used function

7af5c7c

This patch adds is_sof_used function to check we are running test on SOF platform or legacy HDA platform. Signed-off-by: Amery Song <chao.song@intel.com>

aiChaoSONG force-pushed the proc_pipeline branch from fe508fc to b6235e8 Compare October 28, 2020 02:29

aiChaoSONG mentioned this pull request Oct 29, 2020

test-case: add quotes to function arguments for func_export_pipeline #475

Merged

marc-hb previously requested changes Nov 3, 2020

View reviewed changes

case-lib/pipeline.sh Show resolved Hide resolved

pipeline.sh: export proc pipeline info to test case

f0314fa

Signed-off-by: Amery Song <chao.song@intel.com>

aiChaoSONG force-pushed the proc_pipeline branch from b6235e8 to f0314fa Compare November 4, 2020 00:53

aiChaoSONG requested a review from xiulipan November 4, 2020 00:54

xiulipan approved these changes Nov 5, 2020

View reviewed changes

xiulipan requested a review from plbossart November 5, 2020 06:32

xiulipan merged commit 756941d into thesofproject:master Nov 5, 2020

sof-test: read /proc instead of parsing TPLG file for legacy HDA platforms #430

sof-test: read /proc instead of parsing TPLG file for legacy HDA platforms #430

Conversation

aiChaoSONG commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment • 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

aiChaoSONG commented Oct 20, 2020

marc-hb commented Oct 20, 2020 • edited Loading

aiChaoSONG commented Oct 21, 2020

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

marc-hb commented Oct 22, 2020

marc-hb commented Oct 22, 2020

aiChaoSONG commented Oct 22, 2020

marc-hb left a comment

Choose a reason for hiding this comment

aiChaoSONG commented Oct 22, 2020

xiulipan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aiChaoSONG Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

xiulipan Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

plbossart commented Oct 22, 2020

Choose a reason for hiding this comment

aiChaoSONG Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aiChaoSONG Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aiChaoSONG Oct 27, 2020 • 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

Choose a reason for hiding this comment

ranj063 commented Oct 23, 2020 • edited by marc-hb Loading

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

plbossart commented Oct 24, 2020 • edited by marc-hb Loading

marc-hb commented Oct 24, 2020

marc-hb left a comment

Choose a reason for hiding this comment

aiChaoSONG commented Oct 30, 2020

aiChaoSONG commented Nov 3, 2020

marc-hb left a comment

Choose a reason for hiding this comment

marc-hb commented Nov 4, 2020

xiulipan left a comment

Choose a reason for hiding this comment

marc-hb left a comment •

edited

Loading

marc-hb commented Oct 20, 2020 •

edited

Loading

aiChaoSONG Oct 22, 2020 •

edited

Loading

xiulipan Oct 22, 2020 •

edited

Loading

aiChaoSONG Oct 26, 2020 •

edited

Loading

aiChaoSONG Oct 26, 2020 •

edited

Loading

aiChaoSONG Oct 27, 2020 •

edited by marc-hb

Loading

ranj063 commented Oct 23, 2020 •

edited by marc-hb

Loading

marc-hb left a comment •

edited

Loading

plbossart commented Oct 24, 2020 •

edited by marc-hb

Loading