Skip to content

Tools: Topology: Change occurrences of codec_master and codec_slave - #6326

Closed
singalsu wants to merge 2 commits into
thesofproject:mainfrom
singalsu:change_to_codec_consumer
Closed

Tools: Topology: Change occurrences of codec_master and codec_slave#6326
singalsu wants to merge 2 commits into
thesofproject:mainfrom
singalsu:change_to_codec_consumer

Conversation

@singalsu

@singalsu singalsu commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Avoids a lot of warnings prints in topologies build.

@singalsu
Tools: Topology: Change codec_slave to codec_consumer
aef364d 
This patch avoids warnings print for:
ALSA lib pcm.c:1471:(tplg_parse_hw_config) deprecated bclk
value 'codec_slave', use 'codec_consumer'
ALSA lib pcm.c:1523:(tplg_parse_hw_config) deprecated fsync
value 'codec_slave', use 'codec_consumer'"

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Tools: Topology: Change occurrences of codec_master to codec_provider
9a7aca2 
This patch avoids print of warning:
ALSA lib pcm.c:1477:(tplg_parse_hw_config) deprecated bclk
value 'codec_master', use 'codec_provider
ALSA lib pcm.c:1529:(tplg_parse_hw_config) deprecated fsync
value 'codec_master', use 'codec_provider'

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
ujfalusi
ujfalusi approved these changes Sep 23, 2022
View reviewed changes

@ujfalusi ujfalusi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singalsu, thank you!

@plbossart

plbossart commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Last time @marc-hb did the conversion we had to revert it, I can't recall why exactly (maybe results with older alsa-lib or alsa-utils packages). Let's make sure this doesn't break anything.

@marc-hb

marc-hb commented Sep 23, 2022

Copy link
Copy Markdown
Collaborator

This was previously reverted in #5192 because the new terms basically "crash" the older ALSA versions in a very subtle and time consuming way, see #5192 for details.

Transitions like this normally have some "overlap" period where the new term is accepted while the old one is not spamming yet. Too late I guess?

@plbossart

plbossart commented Sep 23, 2022

Copy link
Copy Markdown
Member

with the topology2 dependency on latest bleeding alsa-lib/alsa-utils, I think we should add a dependency check on the alsa-lib package and fail the build early, rather than allow for old versions to proceed and generate bad results silently. It may annoy people who don't have the latest stuff installed but that will be safer than allowing for uncontrolled older versions to be used.

@plbossart

plbossart commented Sep 23, 2022

Copy link
Copy Markdown
Member

@ranj063 @juimonen would you happen to know which alsa-utils/alsa-lib versions are required for the NHLT and topology2 stuff?

@singalsu
singalsu marked this pull request as ready for review September 23, 2022 11:09
@singalsu

singalsu commented Sep 23, 2022

Copy link
Copy Markdown
Collaborator Author

@plbossart Topology2 build is passed by scripts/build-tools.sh if alsa-lib and alsatplg version is < 1.2.7.

@ranj063

ranj063 commented Sep 23, 2022

Copy link
Copy Markdown
Collaborator

@ranj063 @juimonen would you happen to know which alsa-utils/alsa-lib versions are required for the NHLT and topology2 stuff?

@plbossart we need a minimum of 1.2.7 for alsatplg for topology2 to build. In fact we have annoying problem with topology2 builds that we skip building all topology2 files if the alsatplg version is older but the warning gets lost in the sea of other build messages. Maybe we should just enforce 1.2.7 for all topology builds and force users to upgrade. What do you think?

@plbossart

plbossart commented Sep 23, 2022

Copy link
Copy Markdown
Member

@ranj063 alsa-lib 1.2.7 was released on 2022/05/31. I have 1.2.6 on the latest Ubuntu 22.04. It'd be pushing it to require this dependency unilaterally.

If we require the use of 1.2.7, we also provide a solution with Docker or share the Debian packages for 1.2.7 IMHO.

@ranj063

ranj063 commented Sep 23, 2022

Copy link
Copy Markdown
Collaborator

@ranj063 alsa-lib 1.2.7 was released on 2022/05/31. I have 1.2.6 on the latest Ubuntu 22.04. It'd be pushing it to require this dependency unilaterally.

If we require the use of 1.2.7, we also provide a solution with Docker or share the Debian packages for 1.2.7 IMHO.

I think we have updated the docker image @fredoh9 please correct me if I am wrong

@marc-hb

marc-hb commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

I think we should add a dependency check on the alsa-lib package and fail the build early, ...

This can be done but it would be quite a disruptive change. This was already discussed in #5192 BTW

topology2 also requires a recent ALSA but just a reminder that is a totally unrelated problem. Also, missing topology2 files is a problem quick and easy to find.

... rather than allow for old versions to proceed and generate bad results silently. It may annoy people who don't have the latest stuff installed but that will be safer than allowing for uncontrolled older versions to be used.

Bad results are generated only with the new terms codec_consumer, otherwise older versions have been fine so far.

If we require the use of 1.2.7, we also provide a solution with Docker or share the Debian packages for 1.2.7 IMHO.

Our ALSA debian packaging story "never went anywhere":

I think we have updated the docker image @fredoh9 please correct me if I am wrong

Yes the container has 1.2.7, this can be checked with a single-line:

./scripts/docker-run.sh alsatplg --version

I find it worrying to recommend solutions that we either don't have or don't use ourselves.

I guess the 3rd option is what most current developers use:
https://thesofproject.github.io/latest/getting_started/build-guide/build-from-scratch.html#build-alsa-lib-and-alsa-utils-from-source

If you run into alsa-lib linking errors, try (?!) to re-build it with the libdir parameter.
...
If the gitcompile script does not work, refer to the INSTALL file for manual build instructions.

Not a smooth developer experience either.

@marc-hb

marc-hb commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What compounds the issue is the lack of incremental build. Change one topology and they ALL get rebuilt. Wait, even "better": change zero topology and they all get rebuilt. I've been wanting to fix that for a long time, should a relatively small CMake fix. Because of this issue and other incoming topology changes I will do it now. Among others this will make the topology2 warning much more visible.

@fredoh9

fredoh9 commented Sep 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

SOF docker is always up-to-date with SOF toolchain requirement. Last update was done on Aug 9th. ALSA version is v1.2.7

fredoh9
fredoh9 approved these changes Sep 23, 2022
View reviewed changes

@fredoh9 fredoh9 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, most anticipated fix. thanks!

marc-hb
marc-hb requested changes Sep 25, 2022
View reviewed changes

@marc-hb marc-hb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't think we should do this until we offer a clearer and safer ALSA experience. A couple of us wasted a lot of time last time I did this.

@singalsu

singalsu commented Sep 26, 2022

Copy link
Copy Markdown
Collaborator Author

OK, I'll close this!

What compounds the issue is the lack of incremental build. Change one topology and they ALL get rebuilt. Wait, even "better": change zero topology and they all get rebuilt. I've been wanting to fix that for a long time, should a relatively small CMake fix. Because of this issue and other incoming topology changes I will do it now. Among others this will make the topology2 warning much more visible.

Yep. Simple command line "make" should always work from build directory

@singalsu singalsu closed this Sep 26, 2022
@marc-hb

marc-hb commented Sep 27, 2022

Copy link
Copy Markdown
Collaborator

What compounds the issue is the lack of incremental build. Change one topology and they ALL get rebuilt. Wait, even "better": change zero topology and they all get rebuilt. I've been wanting to fix that for a long time, should a relatively small CMake fix. Because of this issue and other incoming topology changes I will do it now.

Submitted in #6342 :

Run ./scripts/build-tools.sh twice: the second time builds the minimum required and prints almost nothing when nothing changed.

@marc-hb

marc-hb commented Sep 27, 2022

Copy link
Copy Markdown
Collaborator

The previous revert was 7 months ago. Since then Ubuntu 22.04 LTS got ALSA 1.2.6 as noted by @plbossart and Fedora was upgraded a long time ago. The requirement for codec_consumer is 1.2.5, so we could make 1.2.5 a hard requirement to block alsatplg corruption. It would also block debian stable but who's using that? :-) And there's still our container and compiling from source.

But first please help test and review incremental build fix #6342, you should like it whatever happens here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marc-hb marc-hb marc-hb requested changes
@ujfalusi ujfalusi ujfalusi approved these changes
@lgirdwood lgirdwood Awaiting requested review from lgirdwood
@plbossart plbossart Awaiting requested review from plbossart
@mmaka1 mmaka1 Awaiting requested review from mmaka1
@lbetlej lbetlej Awaiting requested review from lbetlej
@dbaluta dbaluta Awaiting requested review from dbaluta
+1 more reviewer
@fredoh9 fredoh9 fredoh9 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@singalsu @plbossart @marc-hb @ranj063 @fredoh9 @ujfalusi