tools: Support multi-output component playback testing on testbench by johnylin76 · Pull Request #3210 · thesofproject/sof

johnylin76 · 2020-07-21T06:47:09Z

Note: this patch is currently followed on top of crossover component PR: #2802

This patch supports testbench playback testing for single-input-multi-output component, e.g. crossover. Test playback m4 file (tools/test/topology/test-playback.m4) is extended to generate up to 4 output pipelines test topologies, where argument "TEST_PIPE_AMOUNT" is provided for identifying the number of pipelines.

While building test topologies, 2way, 3way, and 4way crossover playback topology files will be generated under build_tools/test/topology/ with names as follows:
test-playback-ssp5-mclk-0-I2S-2way-crossover-s16le-s16le-48k-24576k-codec.*
test-playback-ssp5-mclk-0-I2S-3way-crossover-s16le-s16le-48k-24576k-codec.*
test-playback-ssp5-mclk-0-I2S-4way-crossover-s16le-s16le-48k-24576k-codec.*
and so on.

Testbench Run
For example of running 4way-crossover playback, the coefficient file with 4-sink crossover should be generated under tools/tune/crossover/ by

octave example_crossover.m

Then after building, run testbench by the following command:

testbench -i input -o output_1,output_2,output_3,output_4 -t test-playback-ssp5-mclk-0-I2S-4way-crossover-s16le-s16le-48k-24576k-codec.tplg -r 48000 -R 48000 -c 1 -b S16_LE -a crossover=libsof_crossover.so

where argument "-o" now accepts specifying up to 4 output file names delimited by comma (in 4way case 4 files should be given), and "-c" specifies channels. The 4way topology file should be specified.

cujomalainey

We are definitely getting close, i think we can take this out of draft now

cujomalainey · 2020-07-21T15:10:42Z

Also looks like CI is breaking on Seb's crossover changes that need to be removed

johnylin76 · 2020-07-22T07:35:37Z

I removed the dependency of crossover library and PR #2802 for this PR. Then it could be merged on its own. Thanks.

cujomalainey · 2020-07-22T17:44:29Z

@lgirdwood how best should we format this? Also will it need a kernel change?

WARNING: Please update ABI in accordance with http://semver.org
#31: FILE: src/include/kernel/header.h:37:
uint32_t data[0];	/**< Component data - opaque to core */
+} __attribute__((packed, aligned(4)));
WARNING: __packed is preferred over __attribute__((packed))
#31: FILE: src/include/kernel/header.h:37:
+} __attribute__((packed, aligned(4)));
WARNING: __aligned(size) is preferred over __attribute__((aligned(size)))
#31: FILE: src/include/kernel/header.h:37:
+} __attribute__((packed, aligned(4)));
WARNING: Please update ABI in accordance with http://semver.org
#43: FILE: src/include/user/eq.h:154:
int32_t output_gain; /* Q2.14 */
+} __attribute__((packed, aligned(4)));
WARNING: __packed is preferred over __attribute__((packed))
#43: FILE: src/include/user/eq.h:154:
+} __attribute__((packed, aligned(4)));
WARNING: __aligned(size) is preferred over __attribute__((aligned(size)))
#43: FILE: src/include/user/eq.h:154:
+} __attribute__((packed, aligned(4)));

johnylin76 · 2020-07-23T05:58:47Z

Those WARNINGs are brought by one of my commit: fa82026
to clean up GCC errors.

However, I wonder if GCC errors happened on my env only? Otherwise there should be others hitting such errors already.

johnylin76 · 2020-07-23T06:28:48Z

Those WARNINGs are brought by one of my commit: fa82026
to clean up GCC errors.
However, I wonder if GCC errors happened on my env only? Otherwise there should be others hitting such errors already.

And FYI, GCC errors appeared only with Seb's Crossover component commits.
Hmm, then my GCC-error commit actually should not be here... Because this PR is independent of Seb's.

lgirdwood

Can you split the PR into patches per feature with the dependencies first.

Support up to 4 output pipelines for test/topology/test-playback.m4, where argument TEST_PIPE_AMOUNT is added for identifying the number of pipelines while generating test topologies. Multi-output component should be added in ALG_MULTI_MODE_TESTS and ALG_MULTI_SIMPLE_TESTS to generate test topologies with multiple output pipelines in the future. Signed-off-by: Pin-chih Lin <johnylin@google.com>

In order to run multi-output component playback by testbench, this patch supports multiple output file mode on running testbench. Argument "-o" is modified to accept specifying up to 4 filenames delimited by comma, e.g. "-o output1,output2,..." Moreover, debug messages in tplg_parser/tplg_parser.c is refined for demonstrating multiple pipelines better. Signed-off-by: Pin-chih Lin <johnylin@google.com>

Support channel specification by argument "-c" on testbench Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 · 2020-07-24T06:41:48Z

Just split into patches, thanks.

cujomalainey

Looks good, just one optimization that I am not even sure is worth having tbh, I will let others decide if the effort is worth it.

cujomalainey · 2020-07-24T06:55:02Z

+
+ifdef(`TEST_HAS_PIPE4',
+`
+# Playback pipeline 4 on PCM 3 using max 2 channels of s32le.


I wonder if this section could be extracted out to a macro call and the index is just subbed in, thoughts?

cujomalainey · 2020-07-24T06:56:11Z

+
+# playback DAI is SSP TEST_DAI2_PORT using 2 periods
+# Buffers use s24le format, with 48 frame per 1000us on core 0 with priority 0
+DAI_ADD_SCHED(sof/pipe-dai-sched-playback.m4,


same goes for this section

cujomalainey · 2020-08-03T18:26:46Z

@lgirdwood PTAL as this is blocking the rest of the crossover series

lgirdwood · 2020-08-03T20:07:45Z

CI unrelated issues.

lgirdwood · 2020-08-03T20:08:39Z

@singalsu any comments ?

johnylin76 · 2020-08-04T06:28:20Z

@lgirdwood how best should we format this? Also will it need a kernel change?

WARNING: Please update ABI in accordance with http://semver.org
#31: FILE: src/include/kernel/header.h:37:
uint32_t data[0];	/**< Component data - opaque to core */
+} __attribute__((packed, aligned(4)));
WARNING: __packed is preferred over __attribute__((packed))
#31: FILE: src/include/kernel/header.h:37:
+} __attribute__((packed, aligned(4)));
WARNING: __aligned(size) is preferred over __attribute__((aligned(size)))
#31: FILE: src/include/kernel/header.h:37:
+} __attribute__((packed, aligned(4)));
WARNING: Please update ABI in accordance with http://semver.org
#43: FILE: src/include/user/eq.h:154:
int32_t output_gain; /* Q2.14 */
+} __attribute__((packed, aligned(4)));
WARNING: __packed is preferred over __attribute__((packed))
#43: FILE: src/include/user/eq.h:154:
+} __attribute__((packed, aligned(4)));
WARNING: __aligned(size) is preferred over __attribute__((aligned(size)))
#43: FILE: src/include/user/eq.h:154:
+} __attribute__((packed, aligned(4)));

This is actually not related to this PR, and the alternative solution is provided at b041b42
So we may probably remove the ABI tag?

cujomalainey · 2020-08-04T17:48:46Z

Agreed, ABI label does not apply to current solution

lgirdwood · 2020-08-05T08:59:17Z

@johnylin76 the GCC warnings only come about with more recent versions (and this is being updated now in the CI container to catch any problems). We are currently resolving then when accessed via ASSUME_ALIGNED() CC attribute, but I also think the alignment attribute on the structure definition is needed too. @plbossart any issues adding this attribute to the kernel user/ipc headers ?

johnylin76 · 2020-08-05T09:05:06Z

@lgirdwood Thanks for the information I have already done via ASSUME_ALIGNED() in commit b041b42
So this should be no longer an issue now. No need to modify kernel headers.

lgirdwood · 2020-08-05T09:37:59Z

@cujomalainey are you OK with changes to merge ?

cujomalainey

@lgirdwood Yep, only optimizations left that can be done later if needed

lgirdwood · 2020-08-06T11:25:09Z

Unrelated failure on Jenkins as this is testbench.

johnylin76 requested review from cujomalainey and singalsu July 21, 2020 06:47

johnylin76 mentioned this pull request Jul 21, 2020
tools: Support multi-output component playback testing on testbench #3203
Closed

cujomalainey suggested changes Jul 21, 2020
View reviewed changes

Comment threadtools/tplg_parser/include/tplg_parser/topology.h Outdated
Comment threadtools/testbench/topology.c Outdated
Comment threadtools/test/topology/tplg-build.sh Outdated

johnylin76 marked this pull request as ready for review July 22, 2020 01:30

johnylin76 requested review from a team, dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart, ranj063 and tlauda as code owners July 22, 2020 01:30

johnylin76 force-pushed the crossover branch 2 times, most recently from a376786 to 888e6e9Compare July 22, 2020 07:22

cujomalainey added the ABI ABI change involved label Jul 22, 2020

lgirdwood requested changes Jul 23, 2020
View reviewed changes

Comment threadtools/test/topology/test-playback.m4 Outdated

johnylin76 added 3 commits July 24, 2020 13:52

tools: testbench: Support channels specification
9d1581b
Support channel specification by argument "-c" on testbench Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 force-pushed the crossover branch from 888e6e9 to 9d1581bCompare July 24, 2020 06:28

cujomalainey reviewed Jul 24, 2020
View reviewed changes

cujomalainey requested a review from lgirdwood July 29, 2020 01:35

cujomalainey self-requested a review July 29, 2020 01:36

cujomalainey mentioned this pull request Jul 31, 2020
Testbench support for Crossover sebcarlucci/sof#2
Closed

cujomalainey mentioned this pull request Aug 3, 2020
WIP: Testbench Support For Crossover Component #3139
Closed

lgirdwood approved these changes Aug 3, 2020
View reviewed changes

johnylin76 removed the ABI ABI change involved label Aug 4, 2020

cujomalainey approved these changes Aug 5, 2020
View reviewed changes

lgirdwood merged commit b63c3a3 into masterAug 6, 2020

lgirdwood deleted the crossover branch January 27, 2021 09:24

Uh oh!

Conversation

johnylin76 commented Jul 21, 2020

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cujomalainey commented Jul 21, 2020

Uh oh!

johnylin76 commented Jul 22, 2020

Uh oh!

cujomalainey commented Jul 22, 2020

Uh oh!

johnylin76 commented Jul 23, 2020

Uh oh!

johnylin76 commented Jul 23, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnylin76 commented Jul 24, 2020

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

cujomalaineyJul 24, 2020

Choose a reason for hiding this comment

Uh oh!

cujomalaineyJul 24, 2020

Choose a reason for hiding this comment

Uh oh!

cujomalainey commented Aug 3, 2020

Uh oh!

lgirdwood commented Aug 3, 2020

Uh oh!

lgirdwood commented Aug 3, 2020

Uh oh!

johnylin76 commented Aug 4, 2020

Uh oh!

cujomalainey commented Aug 4, 2020

Uh oh!

lgirdwood commented Aug 5, 2020

Uh oh!

johnylin76 commented Aug 5, 2020

Uh oh!

lgirdwood commented Aug 5, 2020

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Aug 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants