topology2: Merge avs-tplg and sof-ace-tplg under production directory #8712
Conversation
ujfalusi
requested review from
plbossart,
mengdonglin,
lgirdwood,
kv2019i and
aiChaoSONG
|
This is going to fail in CI as all IPC4 firmware files are under sof-ipc4-tplg. The generic HDA topologies does not contain NHLT binary! |
@ujfalusi What is your proposed solution to fix the issue in CI ? |
If CI needs to (stop!) renam(ing) some directories or create some symbolic link(s) then just specify very clearly what it must do or stop doing. Then we'll merge all changes at about the same time. The only purpose of tests and CI is to find problems before customers do and every CI hack and CI "secret" has been for too long a failure to do that (admittedly not helped by a lack of concern and documentation before @ujfalusi stood up) Let's worry about |
|
@marc-hb, @lgirdwood, @kv2019i, with this PR CI (and sof-bin) should do this for all IPC4 platforms: sof-bin note: remove the CAVS2.5 and ACE2 topologies before release (sof-tgl/adl/etc and sof-cavs25-*) for now? After this DUT should just work - no tplg path override needed!, but... We have machines in CI (UPX I think) which does not have NHLT blob in BIOS and we were using the topology embedded one. on ACE1 and ACE2 platforms: The reason is that the blob format differs between architectures, so the sof-hda-generic* is converted to be generic, it works on systems where the NHLT is in BIOS, however for developers (and unfortunate users) we have the topologies with NHLT. |
|
@ujfalusi lets base solution on what the upstream driver loads today for MTL, we can adapt kernel topology preference from LNL onwards. I would ideally like to leave MTL (and everything before) with the naming convention it uses today and land any new schema from LNL onwards. |
@lgirdwood, I disagree. We should transition to a documented and standard layout, we can provide the fallback for existing kernels without complicated rules and logistics. |
This means managing different kernel version and distros through any transition and will be cumbersome. Whilst the current upstream schema may not be perfect for all needs, its often a lot more effort than its worth to back port the improvements. I think its better to leave it as it is (since its working for MTL ?) and make the changes for LNL+ |
One of the main disagreements seems to be on the definition of "too late". @ujfalusi please spell out your precise "too late" definition. I guess this should include some kernel version(s)? I personally don't see any difference between "One platform (MTL) uses the
These are development boards, not "regular" customers but developers so a small hack for these seems acceptable to me if absolutely needed. Also: CAVS25+TGL is NOT officially supported.
I don't see what's wrong with this either. I admittedly don't understand all the details and nuances. |
@lgirdwood, no backporting is needed to support released distro kernels for MTL. We have only MTL released currently, we have the opportunity now to change for a layout which is going to work in the future. We need to define where the topology files are going to be before we can release new platforms. The firmware file location is now settled and it is scaling up to any new IPC4 platform but the topology location is not and if we don't settle on a layout which is going to scale then we are going to have the burden to deal with an increasing mess overtime. |
|
@lgirdwood wrote:
@ujfalusi already replied, but let me add a note that I also had this concern, but this is not a problem in the PR as we will always have the "sof-ace-tplg" symlink in sof-bin (this we cannot change anymore). The idea is just to put all Intel IPC4 tplg files in one directory and build our CI/sof-bin automation around that idea. Whether it's called "sof-ipc4-tplg" or "sof-ace-tplg" is less important. |
|
One scaled down alternative B is to just strip the tplg NHLT blobs from the sof-ace-tplg/sof-hda-generic*tplg files and not converge the folder. This will make "sof-ace-tplg" more future proof (solve #8683), but not trying to clean up naming or help releasing the cavs2.5 IPC4 tplg files. Or alternative C: no action now. Fork "sof-ace-tplg" to "sof-bar-tplg" if/when a platform comes that makes the sof-hda-generic*tplg incompatible and use sof-ace-tplg name until then. |
|
I know that it is intuitive for most of the people A simple rule of |
there's no reason to treat MTL and LNL differently at this point. let's keep the two grouped. |
We have changed the CAVS2.5 paths, we could do that because CAVS2.5 is not released with IPC4. To build the firmware, you need different platform target: What is wrong with saying that IPC4 topology files are under |
no we don't. see build-tools.sh |
https://github.com/thesofproject/sof/blob/main/tools/topology/topology2/sof-ace-tplg/tplg-targets.cmake#L19 VS |
all topologies will be built. |
ujfalusi
force-pushed
the
peter/pr/merge_ipc4_tplgs_01
branch
from
289e77b
to
9b51cc6
Compare
ujfalusi
changed the title
|
Changes since v1:
|
|
The changes look ok, but I am not Cmake expert, and I don't quite get how CI is going to deal with this change across all devices. |
|
Changes since v2:
|
It almost looks good on the storage disk in web view but for some reason the symlink is not transferred there, the target is OK with all the tplg files in place: @marc-hb, do you have idea why the symlink is missing? |
|
@marc-hb, @aiChaoSONG, @mengdonglin, can you try to plan a solution for CI to use the new location of the built tplg files and to prepare the DUTs? |
ujfalusi
force-pushed
the
peter/pr/merge_ipc4_tplgs_01
branch
from
9f489dc
to
d3f690e
Compare
|
Changes since v3:
|
|
Hi @ujfalusi @lgirdwood @plbossart @marc-hb, @keqiaozhang is going to see how to update CI to support this change. |
|
@ujfalusi Do we still need a kernel PR to change the default topology path for cavs2.5 platforms? |
CAVS2.5 is already changed to use This is to avoid temporary regression and to not need to sync PRs. |
|
@ujfalusi , CI only need to change the tplg path in software configuration. If both kernel and sof PRs are ready, I think to avoid potential risks, we can ask help from Bard to merge these PRs tomorrow morning (PRC time), I will do the configuration changes accordingly. |
@keqiaozhang, you also need to override the topology filename for devices where the NHLT is coming from the topology file (like upx-i11 boards afaik). With CAVS2.5 NHLT: With ACE1/2 NHLT: Wherever CI sets the |
Sure, I will also change the tplg file name in hardware configuration for HDA platforms. |
|
The kernel PR to change the topology path: thesofproject/linux#4779 |
scripts/build-tools.sh
Outdated
| make_tool $util | ||
| fi | ||
| done | ||
| if eval $DO_BUILD_topologies; then |
There was a problem hiding this comment.
This is suspicious, does this really work? I would expect this new code to perform like this:
eval $DO_BUILD_topologies;
eval _topologies; # EDIT: this is wrong sorry.
=> `_topologies` command not found, return `false` always.
I would have expected the original quotes to be preserved:
| if eval $DO_BUILD_topologies; then | |
| if eval '$DO_BUILD_'topologies; then |
While eval may stretch shellcheck a bit, it's always, always worth asking shellcheck what it thinks.
There was a problem hiding this comment.
Sorry, my bad. I shouldn't review after a certain hour.
The eval is not required anymore, just drop it. It's confusing and confused me.
| if eval $DO_BUILD_topologies; then | |
| if ${DO_BUILD_topologies}; then |
Also run shellcheck -x - always.
scripts/build-tools.sh
Outdated
| make_tool $util | ||
| fi | ||
| done | ||
| if eval $DO_BUILD_topologies; then |
There was a problem hiding this comment.
Sorry, my bad. I shouldn't review after a certain hour.
The eval is not required anymore, just drop it. It's confusing and confused me.
| if eval $DO_BUILD_topologies; then | |
| if ${DO_BUILD_topologies}; then |
Also run shellcheck -x - always.
scripts/build-tools.sh
Outdated
| ln -rsf $TPLG2_RELEASE_PATH/sof-ipc4-tplg $TPLG2_RELEASE_PATH/sof-ace-tplg | ||
| fi | ||
|
|
||
| if eval $DO_BUILD_tests; then |
There was a problem hiding this comment.
| if eval $DO_BUILD_tests; then | |
| if ${DO_BUILD_tests}; then |
scripts/build-tools.sh
Outdated
| @@ -146,12 +146,20 @@ main() | |||
| exit 0 | |||
| fi | |||
|
|
|||
| # Keep 'topologies' first because it's the noisiest. | |||
There was a problem hiding this comment.
# Keep 'topologies' first because it's the noisiest.
Why drop that comment?
There was a problem hiding this comment.
I guess it was an error, keeping it
scripts/build-tools.sh
Outdated
| if eval $DO_BUILD_topologies; then | ||
| make_tool topologies | ||
|
|
||
| TPLG2_RELEASE_PATH="$BUILD_TOOLS_DIR/topology/topology2/production/target/" |
There was a problem hiding this comment.
| TPLG2_RELEASE_PATH="$BUILD_TOOLS_DIR/topology/topology2/production/target/" | |
| local TPLG2_RELEASE_PATH="$BUILD_TOOLS_DIR/topology/topology2/production/target/" |
|
|
||
| add_custom_target(topology2_cavs) | ||
| add_custom_target(topology2) |
There was a problem hiding this comment.
| add_custom_target(topology2) | |
| add_custom_target(topology2_prod) |
Be more explicit, these renames and links are confusing enough.
| add_custom_target(topology2_avs_${output} DEPENDS ${output}.tplg) | ||
| add_dependencies(topology2_cavs topology2_avs_${output}) | ||
| add_custom_target(topology2_${output} DEPENDS ${output}.tplg) | ||
| add_dependencies(topology2 topology2_${output}) |
There was a problem hiding this comment.
| add_dependencies(topology2 topology2_${output}) | |
| add_dependencies(topology2_prod topology2_${output}) |
scripts/build-tools.sh
Outdated
| TPLG2_RELEASE_PATH="$BUILD_TOOLS_DIR/topology/topology2/production/target/" | ||
| echo ""; echo "Target directory for topology2: $TPLG2_RELEASE_PATH" | ||
| mkdir -p "$TPLG2_RELEASE_PATH/sof-ipc4-tplg" | ||
| mv $BUILD_TOOLS_DIR/topology/topology2/production/*.tplg $TPLG2_RELEASE_PATH/sof-ipc4-tplg/ |
There was a problem hiding this comment.
Finally noticing this, sorry for the belated review.
| mv $BUILD_TOOLS_DIR/topology/topology2/production/*.tplg $TPLG2_RELEASE_PATH/sof-ipc4-tplg/ | |
| mv $BUILD_TOOLS_DIR/topology/topology2/production/*.tplg $TPLG2_RELEASE_PATH/sof-ipc4-tplg/ |
Nope.
Never move, delete or rename the files that CMake/ninja just built. They belong to CMake/ninja; $BUILD_TOOLS_DIR is EXCLUSIVE property, don't trespass with some outsider script unknown to CMake. This will cause at least these problems:
- spurious rebuilds
- invoking ninja directly behaves differently from invoking this script. Debugging layers of indirections is painful enough, in no circumstances should this behave differently.
- other, unknown and unexpected CMake issues because no one ever pulls the rug under CMake/ninja like this.
These files can be at most copied but the best is really to structure the source directories to match what you want the build directories to be. In other words: move and rename the sources to match.
I hate git renames more than the average but this is one case where they must be performed.
If you really, really, really can't move sources then look at CMake's "install" feature. Spoiler alert: you don't want that.
EDIT: there's also cmake -E copy_directory, example in commit 78f4fd7
There was a problem hiding this comment.
I hate git renames more than the average but this one case where they must be performed.
That does not mean moving everything. Moving only the CMakeLists.txt file and changing the include() should work. Or tricks like CMAKE_CURRENT_BINARY_DIR/../better_dir/ might work too. As long as it does not mess with the build directory behind CMake's back then it's worth a try.
On the other hand, renaming everything would be more consistent with your overarching and admirable quest for... consistency and simplification.
There was a problem hiding this comment.
The reason for the change in the build-tools.sh was solely to move (which I should have not been doing, at most copy!) the *.tplg` files to a place from where the whole thing can be just copied.
├── sof-ace-tplg -> sof-ipc4-tplg
└── sof-ipc4-tplg
├── sof-adl-rt711-4ch.tplg
...
But if we really want to be precise we should have another level at the top with intel, right?
If we move the CMake files to that struct then we also need (need == should) move the development CMake around, probably the include files as well and few more can of worms...
There was a problem hiding this comment.
Update: I have found a way to do it cleanly with CMake!
I'm afraid CI has typically been downloading (and... renaming) topology files one by one: because no one ever did the massive clean-up work you've been doing. There are multiple steps but some rely on HTTP which does not support symlinks. The topologies should always be "packaged" in a tarball which obviously does preserve symlinks but I'm afraid we may not be there yet. I could be wrong, @keqiaozhang should know 10 times more. |
|
I just verified that there is no risk in applying TPLG changes in CI, we just need to update the TPLG name and path accordingly in the hardware/software configurations. @ujfalusi please help to address the comments from Marc, I will merge these PRs next Monday. |
ujfalusi
force-pushed
the
peter/pr/merge_ipc4_tplgs_01
branch
from
d3f690e
to
5f67375
Compare
Merge the avs-tplg and sof-ace-tplg under a common production directory.
After a successful build CMake will copy the topology files to a target
directory from where they can be copied to DUT/release:
$ tree tools/build_tools/topology/topology2/target
tools/build_tools/topology/topology2/target
├── development
│ ├── cavs-sdw-hdmi.tplg
│ ├── cavs-sdw-src-gain-mixin.tplg
...
│ ├── sof-tgl-nocodec-rtcaec.tplg
│ └── sof-tgl-nocodec.tplg
├── sof-ace-tplg -> sof-ipc4-tplg
└── sof-ipc4-tplg
├── sof-adl-rt711-4ch.tplg
├── sof-adl-rt711-l0-rt1316-l12-rt714-l3.tplg
...
├── sof-tgl-rt712.tplg
└── sof-tgl-rt715-rt711-rt1308-mono.tplg
The sof-hda-generic-2/4ch.tplg will be generated without embedded NHLT as
it is not used under normal circumstance.
Two flavor of the generic topology is generated for CAVS2.5 and ACE1/2 with
included NHLT binary in case it is used by existing users, but it is
unlikely.
As noted in the documentation, on the deployed system a symlink is needed
for ACE1/2 platforms for backwards compatibility:
sof-ace-tplg -> sof-ipc4-tplg
Link: https://github.com/thesofproject/sof-docs/blob/master/getting_started/intel_debug/introduction.rst#2-topology-file
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi
changed the title
|
Changes since v4:
Note: The target path is changed from previous version!! |
| # copy the topology files only to target | ||
| COMMAND ${CMAKE_COMMAND} -E copy_if_different production/*.tplg ${CMAKE_CURRENT_BINARY_DIR}/target/sof-ipc4-tplg/ | ||
| COMMAND ${CMAKE_COMMAND} -E copy_if_different development/*.tplg ${CMAKE_CURRENT_BINARY_DIR}/target/development/ | ||
| ) |
There was a problem hiding this comment.
Seems to do the trick. Given the baseline has no install logic at all, this seems like a huge value-add.
It would seem the dependencies are also set for topologies2 target, so good in that sense as well.
There was a problem hiding this comment.
So this is basically a DIY "install". I suspect there is a slightly more "native" and performant way to do this but this is very simple and easy to maintain later so plenty good enough to get started.
| # copy the topology files only to target | ||
| COMMAND ${CMAKE_COMMAND} -E copy_if_different production/*.tplg ${CMAKE_CURRENT_BINARY_DIR}/target/sof-ipc4-tplg/ | ||
| COMMAND ${CMAKE_COMMAND} -E copy_if_different development/*.tplg ${CMAKE_CURRENT_BINARY_DIR}/target/development/ | ||
| ) |
There was a problem hiding this comment.
So this is basically a DIY "install". I suspect there is a slightly more "native" and performant way to do this but this is very simple and easy to maintain later so plenty good enough to get started.
There was a problem hiding this comment.
This PR did way too many things in a single commit: renaming AND splitting files AND making HDA code changes at the same time. When you add the usual branch divergence on top, this is proving near impossible to backport (#8858).
git is really bad at tracking file renames, so a best practice in complex cases like this one is to keep renames "pure" in one commit and then make changes in the next commit - EVEN if that first commit does not work temporarily. Just flag it as "don't use"; git bisect has a git bisect skip command for these situations.
Merge the avs-tplg and sof-ace-tplg under a common production directory.
After a successful build CMake will copy the topology files to a target
directory from where they can be copied to DUT/release:
The sof-hda-generic-2/4ch.tplg will be generated without embedded NHLT as
it is not used under normal circumstance.
Two flavor of the generic topology is generated for CAVS2.5 and ACE1/2 with
included NHLT binary in case it is used by existing users, but it is
unlikely.
As noted in the documentation, on the deployed system a symlink is needed
for ACE1/2 platforms for backwards compatibility:
sof-ace-tplg -> sof-ipc4-tplg
Link: https://github.com/thesofproject/sof-docs/blob/master/getting_started/intel_debug/introduction.rst#2-topology-file
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com