[stable-v2.8] Merge avs-tplg and sof-ace-tplg under production directory #8861
Conversation
This also drops the NHLT-free, cavs25 variants of the HDA generic topologies, keeping only the ace variants. See below. This prepares for merging avs-tplg/ and sof-ace-tplg/ as done in main branch mega commit 65e4c1f ("topology2: Merge avs-tplg and sof-ace-tplg under production directory") But unlike that mega commit, it's easy to compare outputs before and after this commit and make sure they're the same (they are). Note this does _not_ include the "Deep Buffer cleanups" commit 80f283d ("topology2: Mark DeepBuffer D0I3 compatible by default") and commit 4bb4875 ("topology2: Remove redundant DEEPBUFFER_FW_DMA_MS=100 settings") that got rid of the copy/paste/diverge between cavs and ace variants HDA generic and ace HDA generic. Signed-off-by: Marc Herbert <[email protected]>
Keeping only the ace variants that would clash with these. This prepares for merging avs-tplg/ and sof-ace-tplg/ as was done in main branch mega commit 65e4c1f ("topology2: Merge avs-tplg and sof-ace-tplg under production directory") But unlike that mega commit, it's easy to compare outputs before and after this commit and make sure they're the same (they are). Note this does _not_ include the "Deep Buffer cleanups" commit 80f283d ("topology2: Mark DeepBuffer D0I3 compatible by default") and commit 4bb4875 ("topology2: Remove redundant DEEPBUFFER_FW_DMA_MS=100 settings") that got rid of the copy/paste/diverge between cavs and ace variants HDA generic and ace HDA generic. Signed-off-by: Marc Herbert <[email protected]>
Main branch commit 65e4c1f ("topology2: Merge avs-tplg and sof-ace-tplg under production directory") did too many things in the same commit. It should have been split into 2 or 3 different commits. Renaming and splitting a file in the same commit is almost never a good idea. Making actual coding changes at the same time adds insult to injury: it makes these changes almost impossible to spot in the git diff. When you add the usual "stable versus main" branch divergence on top, it all compounds to make a backport incredibly complex, time-consuming and error-prone. So this backport to stable-v2.8 does NOT do all those things at once. It does only the MINIMUM to achieve the "merge of avs-tplg/ and sof-ace-tplg/ under production/" and that's it. Unlike the mega commit, it's very easy to look at the diff. It's also easy to compare outputs before and after this commit and make sure they're the same - they are. When a file is renamed, it _only_ renames that file - as should have been done in the original pull request. All other changes from that original commit are dropped and should be manually cherry-picked to stable-v2.8 if desired or wanted as a dependency and foundation for further backports. -------- original commit message for reference ------- 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 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 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. Signed-off-by: Peter Ujfalusi <[email protected]> (cherry picked from commit 65e4c1f)
Zero functional change. Signed-off-by: Marc Herbert <[email protected]>
Addition modelled after main branch mega commit 65e4c1f ("topology2: Merge avs-tplg and sof-ace-tplg under production directory") As the "Deep Buffer cleanups" commit 80f283d ("topology2: Mark DeepBuffer D0I3 compatible by default") and commit 4bb4875 ("topology2: Remove redundant DEEPBUFFER_FW_DMA_MS=100 settings") are not in this stable-v2.8 branch, the following parameters are present here: DEEPBUFFER_FW_DMA_MS=100, DEEPBUFFER_D0I3_COMPATIBLE=true Signed-off-by: Marc Herbert <[email protected]>
marc-hb
changed the title
|
https://sof-ci.01.org/sofpr/PR8861/build2705/devicetest/index.html has only one suspend/resume failure. There was one DUT-specific issue with https://sof-ci.01.org/sofpr/PR8861/build2704/devicetest/index.html (internal run 37897) so I aborted it. |
|
SOFCI TEST |
|
Only one multiple pause/resume failure in ACE https://sof-ci.01.org/sofpr/PR8861/build2707/devicetest/index.html after 30 minutes. Maybe related to #8770? Everything else is green. Only one suspend/resume failure in CAVS https://sof-ci.01.org/sofpr/PR8861/build2708/devicetest/index.html QB cannot only test the main branch, known issue: https://sof-ci.01.org/sof-pr-viewer/#/build/PR8861/build13564131 |
| # 2 or 4 DMIC, no NHLT blob included in topology | ||
| "sof-hda-generic\;sof-hda-generic-2ch\;HDA_CONFIG=mix,NUM_DMICS=2,\ | ||
| DEEPBUFFER_FW_DMA_MS=100,DEEPBUFFER_D0I3_COMPATIBLE=true" | ||
| "sof-hda-generic\;sof-hda-generic-4ch\;HDA_CONFIG=mix,NUM_DMICS=4,\ |
There was a problem hiding this comment.
when you have NUM_DMICS as 4 you also need PDM1_MIC_A_ENABLE=1,PDM1_MIC_B_ENABLE=1
There was a problem hiding this comment.
Please correct me but I think it is there... I copied it from 65e4c1f
There was a problem hiding this comment.
Looks good. So this will drop the NHLT blobs from the generic topologies (to implement #8683), but as use of these blobs is behind a kernel module parameter that is off by default, this will not affect any default configuration.
There was a problem hiding this comment.
@marc-hb, I'm not sure what benefit this specific split brings, patch three is still - as you called it - a mega patch...
If you must split, I would recommend:
- avs-tplg: introduce the set of hda-generic tplgs w/ and w/o NHLT
- sof-ace-tplg: introduce the set of hda-generic tplgs w/ and w/o NHLT
- mega patch to merge
imho a single revert to get back to the old state is cleaner in some ways when doing such intrusive change anyways.
| @@ -1,18 +1,18 @@ | |||
| # SPDX-License-Identifier: BSD-3-Clause | |||
|
|
|||
| # Array of "input-file-name;output-file-name;comma separated pre-processor variables" | |||
| set(TPLGS | |||
| list(APPEND TPLGS | |||
There was a problem hiding this comment.
This change is not related to topic, unless it is part of the 'preparation' umbrella.
There was a problem hiding this comment.
Correct, this could have been another "preparation" commit. However I felt like this commit was small and easy enough to review. Moreover this CMake change makes no output difference at this early stage.
| # CAVS HDMI only topology with passthrough pipelines | ||
| "sof-hda-generic\;sof-hda-generic-idisp\;DEEPBUFFER_FW_DMA_MS=100" | ||
| # CAVS HDA topology with mixer-based pipelines for HDA and passthrough pipelines for HDMI | ||
| "sof-hda-generic\;sof-hda-generic\;HDA_CONFIG=mix,DEEPBUFFER_FW_DMA_MS=100" |
There was a problem hiding this comment.
Otherwise these would clash in the next commit with the identical ones in sof-ace-tplg/
PS: to see this thread in the relevant context you must open the commit "avs-tplg: drop the NHLT-free, cavs25 variants of HDA generic". Otherwise Github makes a mess.
| DEEPBUFFER_D0I3_COMPATIBLE=true" | ||
| # HDA topology with mixer-based pipelines for HDA and passthrough pipelines for HDMI | ||
| "sof-hda-generic\;sof-hda-generic\;HDA_CONFIG=mix,DEEPBUFFER_FW_DMA_MS=100,\ | ||
| DEEPBUFFER_D0I3_COMPATIBLE=true" |
There was a problem hiding this comment.
could have been done in the previous patch..
| # HDA topology with mixer-based pipelines for HDA and | ||
| # passthrough pipelines for HDMI and | ||
| # 2 or 4 DMIC, no NHLT blob included in topology | ||
| "sof-hda-generic\;sof-hda-generic-2ch\;HDA_CONFIG=mix,NUM_DMICS=2,\ |
There was a problem hiding this comment.
so, at the previous patch the tplg2 build was incomplete?
I know bisecting is not a primary concern, just pointing out.
Possibly yes.
It depends what you mean by "bisecting". If bisecting implies running tests then no, it's not a concern inside this PR. Running tests in CI is not possible before this PR anyway (and obviously possible after this complete PR - that's the entire purpose). On the other hand, I did "bisect" this PR by carefully comparing the build directories before versus after each commit. Each commit is crafted to make that sort of "bisect" easier. For the same reason, each commit is focused on one change and one change only to make it easier to review. |
I respectfully disagree. Patch 3 has two "pure" renames + small CMake changes + 1 deleted CMakeLists.txt. This makes it easier to test and IMHO to review. |
This is a backport to stable-v2.8 of a subset of mega commit 65e4c1f ("topology2: Merge avs-tplg and sof-ace-tplg under production directory") on the main branch.
This is the absolute minimum subset that meets CI's new topology expectations and allows to permanently run all tests on stable-v2.8 going forward.
Unlike that mega commit, this PR is not just smaller as a whole but also split into much smaller, separate commits that can be easily reviewed for correctness and to make sure it does exactly what its commit message says. Each commit builds and it's easy to compare build directories from one commit to the next and check that topology files are only moved around without any change (and I actually did that).