topology2: Add build support by ranj063 · Pull Request #5153 · thesofproject/sof

ranj063 · 2022-01-05T21:37:53Z

Move the cavs topologies to the cavs folder and build support for these
topologies.

Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

marc-hb

Maybe move the renames to a separate commit?

marc-hb · 2022-01-05T21:40:04Z

Suggested change

export ALSA_CONFIG_DIR=${SOF_TOP}/tools/topology/topology2

export ALSA_CONFIG_DIR="${SOF_TOP}"/tools/topology/topology2

Use shellcheck. Shellcheck saves lives.

marc-hb · 2022-01-05T21:42:34Z

The last one does not belong to the foreach loop.

marc-hb · 2022-01-05T21:45:43Z

You don't need to omit the entire subdirectory, instead omit only the edge in the dependency graph and don't add this target to the default ALL target. In other words, only this line needs to be conditional:

add_dependencies(topologies2 topology2_cavs)

So everyone is aligned and runs the exact same CMake code at configuration time no matter what their ALSA version is. The difference comes later only at build time.

This is also more flexible for any adventurer who has multiple ALSA versions installed.

but as far as topology2 goes everything in the topology2 folder should be conditionally built based on version right? its not just topology2_cavs

Yes sorry, hasty review and copy/paste. The point is to use a scalpel to conditionally cut only the topology2 dependency in the graph, then all v2 dependency "leaves" will be configured and available but not built by default.

so this look good?

No, instead of return() here use:

add_dependencies(ALL topologies2)

+ remove ALL above

marc-hb · 2022-01-05T21:47:35Z

Should you make the MINOR optional?

no, we want the 1.2.6 version which has the -I option added.

Not supporting version 1.3 will be OK? Only allowed from 1.3.1 and above?

i don;t understand. we check if version is less than 1.2.6, why will 1.3 be not supported?

the string command doesnt check for version number. it only extracts it here

I believe REGEX MATCH "[0-9].[0-9].[0-9] will fail to extract 1.3

you're right. fixed now

ranj063 · 2022-01-06T00:40:22Z

SOFCI TEST

marc-hb · 2022-01-06T00:48:19Z

https://github.com/thesofproject/sof/runs/4721408331?check_suite_focus=true failed the testbench (that depends on test topologies) because of #5155 / #5156. Re-running it now (SOFCI TEST is only for sof-ci/jenkins)

EDIT: passed in https://github.com/thesofproject/sof/runs/4722009946?check_suite_focus=true

marc-hb · 2022-01-06T00:52:23Z

The zephyr build failure in https://github.com/thesofproject/sof/runs/4721407875?check_suite_focus=true is not related to Zephyr and can be reproduced with commit 4b743af40cf4 like this:

./scripts/build-tools.sh -l
...
=> tries to build topology2 for some unknown reason.

Same for ./scripts/build-tools.sh -<ANYTHING SPECIFIC>

marc-hb · 2022-01-06T01:13:20Z

=> tries to build topology2 for some unknown reason.

That was wrong, my bad. I got confused by the message Cannot build topology2. Minimum required version ..., I assumed it was printed at build time but it's printed at configure time.

marc-hb · 2022-01-06T01:26:57Z

Found it! The zephyr image has no alsatplg.

Reproduction:

sudo mv /usr/bin/alsatplg /usr/bin/alsatplg.disabled

./scripts/build-tools.sh -l
-- The C compiler identification is GNU 9.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- No CMAKE_BUILD_TYPE, defaulting to Debug
CMake Error at topology/topology2/CMakeLists.txt:8 (string):
  string sub-command REGEX, mode MATCH needs at least 5 arguments total to
  command.


CMake Error at topology/topology2/CMakeLists.txt:9 (if):
  if given arguments:

    "VERSION_LESS" "1.2.2"

  Unknown arguments specified


-- Configuring incomplete, errors occurred!

marc-hb · 2022-01-06T02:39:44Z

Suggested change

message(WARNING "Topology2 will be skipped. Minimum required version for alsatplg: 1.2.6")

message(WARNING "alsatplg error ${STATUS}, topology2 will be skipped")

marc-hb · 2022-01-06T02:41:16Z

Suggested change

add_custom_target(topologies2 ALL)

add_custom_target(topologies2)

See below

marc-hb · 2022-01-06T02:42:26Z

No, instead of return() here use:

add_dependencies(ALL topologies2)

+ remove ALL above

marc-hb · 2022-01-06T02:44:30Z

Suggested change

add_custom_target(topology2_cavs ALL

add_custom_target(topology2_cavs

marc-hb · 2022-01-06T02:47:00Z

Is this the "silent" v1 -> v2 upgrade? If yes add a short comment.

Assuming there will be other subdirectories later besides cavs/, can this be done only once for all in the ../CMakeLists.txt?

marc-hb · 2022-01-06T02:58:38Z

In general ${CMAKE_CURRENT_BINARY_DIR} is not required. You haven't used it below. Quick enough to try.

Major comments addressed

marc-hb · 2022-01-06T04:49:27Z

Would GREATER be more future-proof?

Suggested change

if (${last_index} EQUAL 2)

if (${length} GREATER 2)

math(EXPR last_index "${length}-1")

In any case I think testing ${length} would be slightly more readable.

marc-hb · 2022-01-06T04:52:26Z

Is this dependency real? Or is it just to make sure the v2 copy_directory "wins"? Either way a comment would probably not hurt.

marc-hb · 2022-01-06T04:55:43Z

Why is topology2_cavs optional (should probably be called: topologiesV2_cavs) but topologies2 is not optional? ~~In other words, shouldn't this test be located one level up in ../CMakeLists.txt?~~

marc-hb

Some nits left but good enough.

ranj063 · 2022-01-06T18:02:39Z

SOFCI TEST

Move all cavs tooplogies to the cavs folder. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add build support for topology2. Topology2 will be built only if the alsatplg version if 1.2.6 or greater. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

All .tplg output files have been compared and are strictly identical after the change. The deprecation warnings were added more than one year ago in https://github.com/alsa-project/alsa-lib/commits//706192341d1d0bbb906 Now that we just upgraded our Docker image to ALSA 1.2.6 (https://hub.docker.com/r/thesofproject/sof/tags) so thesofproject#5153 can enable topology v2, the volume of warnings has became unbearable. For instance good luck trying to find the actual error messages for the build failures of thesofproject#5155 - they're totally drowned in these deprecation warnings. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

All .tplg output files have been compared and are strictly identical after the change. The deprecation warnings were added more than one year ago in https://github.com/alsa-project/alsa-lib/commits//706192341d1d0bbb906 Now that we just upgraded our Docker image to ALSA 1.2.6 (https://hub.docker.com/r/thesofproject/sof/tags) so #5153 can enable topology v2, the volume of warnings has became unbearable. For instance good luck trying to find the actual error messages for the build failures of #5155 - they're totally drowned in these deprecation warnings. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

aiChaoSONG · 2022-01-21T08:07:28Z

If we add a new topology, which doesn't have any defines, that means the defines passed to -D option of alsatplg is empty. alsatplg takes next string as the optarg of -D, which coincidentally is -p. This is wrong, and we will have build failure. @ranj063 FYI

marc-hb · 2022-01-21T08:36:07Z

Does alsatplg -D "" -p ... work? The -D argument is a list, so an empty list should work too. If it does not then alsatplg should be fixed.

In the mean time how about the -D ignored-=placeholder workaround?

marc-hb · 2022-01-24T21:46:17Z

Fix in #5249

ranj063 requested review from dbaluta, lbetlej, lgirdwood, marc-hb, mmaka1 and plbossart as code owners January 5, 2022 21:37

marc-hb reviewed Jan 5, 2022

View reviewed changes

ranj063 force-pushed the fix/topology2_build branch from 0e63f95 to 4b743af Compare January 5, 2022 23:26

ranj063 force-pushed the fix/topology2_build branch 3 times, most recently from 557245e to f813869 Compare January 6, 2022 02:08

marc-hb previously requested changes Jan 6, 2022

View reviewed changes

ranj063 force-pushed the fix/topology2_build branch from f813869 to fa307d8 Compare January 6, 2022 04:10

ranj063 requested a review from marc-hb January 6, 2022 04:11

ranj063 force-pushed the fix/topology2_build branch from fa307d8 to 41572eb Compare January 6, 2022 04:34

marc-hb reviewed Jan 6, 2022

View reviewed changes

ranj063 force-pushed the fix/topology2_build branch 2 times, most recently from 5f431cd to 81a9cee Compare January 6, 2022 05:52

ranj063 requested a review from marc-hb January 6, 2022 17:43

marc-hb approved these changes Jan 6, 2022

View reviewed changes

ranj063 added 2 commits January 6, 2022 10:03

topology2: move all cavs topologies

9a1409d

Move all cavs tooplogies to the cavs folder. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

topology2: Add build support

bd659e1

Add build support for topology2. Topology2 will be built only if the alsatplg version if 1.2.6 or greater. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/topology2_build branch from 81a9cee to bd659e1 Compare January 6, 2022 18:05

marc-hb mentioned this pull request Jan 6, 2022

Simplifications for codec_adapter_set_params() #5160

Merged

lgirdwood approved these changes Jan 6, 2022

View reviewed changes

lgirdwood merged commit 308a24a into thesofproject:main Jan 6, 2022

marc-hb mentioned this pull request Jan 6, 2022

topologies: switch all .m4 files to codec_provider and codec_consumer #5162

Merged

marc-hb mentioned this pull request Apr 16, 2022

topology1: SoundWire/HDAudio: add headphone deep buffer support #4611

Merged

	export ALSA_CONFIG_DIR=${SOF_TOP}/tools/topology/topology2
	export ALSA_CONFIG_DIR="${SOF_TOP}"/tools/topology/topology2

	message(WARNING "Topology2 will be skipped. Minimum required version for alsatplg: 1.2.6")
	message(WARNING "alsatplg error ${STATUS}, topology2 will be skipped")

	add_custom_target(topologies2 ALL)
	add_custom_target(topologies2)

	add_custom_target(topology2_cavs ALL
	add_custom_target(topology2_cavs

	if (${last_index} EQUAL 2)
	if (${length} GREATER 2)
	math(EXPR last_index "${length}-1")

Conversation

ranj063 commented Jan 5, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jan 6, 2022

Uh oh!

marc-hb commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jan 6, 2022

Uh oh!

marc-hb commented Jan 6, 2022

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!

Uh oh!

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 Jan 6, 2022 • 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!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jan 6, 2022

Uh oh!

aiChaoSONG commented Jan 21, 2022

Uh oh!

marc-hb commented Jan 21, 2022

Uh oh!

marc-hb commented Jan 24, 2022

Uh oh!

Reviewers

marc-hb commented Jan 6, 2022 •

edited

Loading

marc-hb commented Jan 6, 2022 •

edited

Loading

marc-hb Jan 6, 2022 •

edited

Loading