topologies: switch all .m4 files to codec_provider and codec_consumer by marc-hb · Pull Request #5162 · thesofproject/sof

marc-hb · 2022-01-06T23:40:13Z

EDIT: reverted in #5192

All .tplg output files have been compared and are strictly identical
after the change. EDIT: compared identical only with alsatplg 1.2.6! 1.2.2 silently corrupts .tplg files, see below.

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

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>

marc-hb · 2022-01-06T23:48:30Z

Ready for review but keeping as a draft for a day or two until any topology v2 dust from v2 #5153 settles.

Compare the still too verbose but now somewhat readable :
https://github.com/thesofproject/sof/runs/4733427070?check_suite_focus=true

... with the previously unreadable:
https://github.com/thesofproject/sof/runs/4721266153?check_suite_focus=true
Try to find the errors in the latter

cujomalainey

this is awesome! can you update tools/test/topology/ as well?

marc-hb · 2022-01-06T23:59:15Z

this is awesome! can you update tools/test/topology/ as well?

Already done?

cujomalainey · 2022-01-07T00:05:26Z

this is awesome! can you update tools/test/topology/ as well?

Already done?

When i pulled down your branch there was a few outstanding comments

tools/test/topology/test-capture.m4
75:# Clocks masters wrt codec

tools/test/topology/test-tone-playback.m4
60:# Clocks masters wrt codec

tools/test/topology/test-playback.m4
213:# Clocks masters wrt codec

greg-intel

I make the noise, you clean it up. Nice.

marc-hb · 2022-01-07T00:26:03Z

When i pulled down your branch there was a few outstanding comments

OK. This PR is not a complete cleanup, it's only replacing two very specific ALSA keywords in the code. It does not change any comment. Considering the huge number of lines in this PR, I would like to keep it sort of "minimal", I mean 100% focused on a single logical change (two actually: master+slave but you get my point) and 100% focused on getting rid of the build warnings. So this PR breaks someone else's build somewhere else in only one way, minimizing git conflicts, easier to review and automate, etc. etc.

marc-hb · 2022-01-08T18:46:33Z

https://sof-ci.01.org/sofpr/PR5162/build11524/devicetest/?model=TGLU_UP_HDA_ZEPHYR&testcase=check-suspend-resume-with-playback-5 looks like #5120 Everything else is green.

marc-hb · 2022-01-12T01:37:00Z

Just a very early heads-up that this PR breaks alsatplg version 1.2.2 (default version in latest Ubuntu LTS) in an incredibly subtle and time-consuming / nasty way. No error message at build time and the .tplg output files seem to have the expected size. But their content differs very slightly and breaks aplay in weird way(s), example below.

Any clue? EDIT: answers in #5192

diffoscope BTconsumer2/topology/sof-apl-nocodec.tplg BTconsumer6/topology/sof-apl-nocodec.tplg
--- codec_consumer1.2.2/topology/sof-apl-nocodec.tplg
+++ codec_consumer1.2.6/topology/sof-apl-nocodec.tplg
@@ -2096,15 +2096,15 @@
 000082f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008300: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008310: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008320: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008330: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008340: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008350: 0000 0000 0000 0000 0000 0000 0000 0078  ...............x
-00008360: 0000 0000 0000 0001 0000 0000 0000 0000  ................
+00008360: 0000 0000 0000 0001 0000 0000 0000 0101  ................
 00008370: 0100 0000 0077 0100 e02e 0080 bb00 0002  .....w..........
 00008380: 0000 0020 0000 0003 0000 0003 0000 0000  ... ............
 00008390: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 000083a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 000083b0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 000083c0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 000083d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
@@ -2211,15 +2211,15 @@
 00008a20: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008a30: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008a40: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008a50: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008a60: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008a70: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008a80: 0000 0078 0000 0001 0000 0001 0000 0000  ...x............
-00008a90: 0000 0000 0100 0000 0077 0100 e02e 0080  .........w......
+00008a90: 0000 0101 0100 0000 0077 0100 e02e 0080  .........w......
 00008aa0: bb00 0002 0000 0020 0000 0003 0000 0003  ....... ........
 00008ab0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008ac0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008ad0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008ae0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008af0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00008b00: 0000 0000 0000 0000 0000 0000 0000 0000  ................
@@ -2325,15 +2325,15 @@
 00009140: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009150: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009160: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009170: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009180: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009190: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 000091a0: 0000 0000 0000 0078 0000 0002 0000 0001  .......x........
-000091b0: 0000 0000 0000 0000 0100 0000 0077 0100  .............w..
+000091b0: 0000 0000 0000 0101 0100 0000 0077 0100  .............w..
 000091c0: e02e 0080 bb00 0002 0000 0020 0000 0003  ........... ....
 000091d0: 0000 0003 0000 0000 0000 0000 0000 0000  ................
 000091e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 000091f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009200: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009210: 0000 0000 0000 0000 0000 0000 0000 0000  ................
 00009220: 0000 0000 0000 0000 0000 0000 0000 0000  ................

aiChaoSONG · 2022-01-12T02:00:49Z

@marc-hb I suggest you to compare the generated conf, which should be human readable.

marc-hb · 2022-01-12T02:08:59Z

The generated conf files are the same. AFAIK they come from m4 not alsatplg.

Revert submitted in #5192

aiChaoSONG · 2022-01-12T02:13:42Z

@marc-hb No, they are not the same
The macro PIPELINE_FORMAT is not expanded for all PGA

AFTER THE PR

SectionVendorTuples."MIXER1.0_tuples_str" {
	tokens "sof_comp_tokens"
	tuples."string" {
		SOF_TKN_COMP_FORMAT	"PIPELINE_FORMAT"
	}
}

BEFORE THE PR

SectionVendorTuples."MIXER1.0_tuples_str" {
	tokens "sof_comp_tokens"
	tuples."string" {
		SOF_TKN_COMP_FORMAT	"s32le"
	}
}

Maybe c687815 is related to the issue

marc-hb · 2022-01-12T02:23:19Z

The generated conf files are the same. AFAIK they come from m4 not alsatplg.

Sorry I meant: the .conf files are exactly the same between 1.2.2 and 1.2.6, I just checked again.

With codec_slave, all the .conf files stay the same between ALSA 1.2.2 and ALSA 1.2.6
With codec_consumer, all the .conf files stay the same between ALSA 1.2.2 and ALSA 1.2.6

Of course this PR changes the .conf files, that's expected.

The macro PIPELINE_FORMAT is not expanded for all mixers

Please provide a specific topology filename so I can try to reproduce.

Maybe c687815 is related to the issue

Maybe you're looking at a different issue. For all comparisons I've reverted this PR; no other change involved.

aiChaoSONG · 2022-01-12T02:24:34Z

Please provide a specific topology filename so I can try to reproduce.

it's the same sof-apl-nocodec.tplg topology

I build three sof-apl-nocodec.conf from the latest main, the one commit before your PR, and the one commit before c687815, I found the PIPELINE_FORMAT not expanded issue is caused by c687815

my env: alsatplg version 1.2.5.1 libasound version 1.2.5 libatopology version 1.2.5

marc-hb · 2022-01-12T02:41:55Z

I build three sof-apl-nocodec.conf from the latest main, the one commit before your PR, and the one commit before c687815,

That is the problem: you're involving other commits. To test, revert this commit only (at any point you like) and don't change anything else.

marc-hb · 2022-01-12T02:43:39Z

my env: alsatplg version 1.2.5.1 libasound version 1.2.5 libatopology version 1.2.5

1.2.5 is close to 1.2.6 so it's probably fine. I found the problem with 1.2.2

aiChaoSONG · 2022-01-12T05:45:05Z

The hex diff 0101 is the value of bclk_master(u8) and fsync_master(u8) in the snd_soc_tplg_hw_config in the snd_soc_tplg_link_config.

#define SND_SOC_TPLG_BCLK_CP         0 /* codec is bclk provider */
#define SND_SOC_TPLG_BCLK_CC         1 /* codec is bclk consumer */

#define SND_SOC_TPLG_FSYNC_CP         0 /* codec is fsync provider */
#define SND_SOC_TPLG_FSYNC_CC         1 /* codec is fsync consumer */

Here is how the alsa-lib process the hardware config. https://github.com/alsa-project/alsa-lib/blob/c687c482107f746332dd18f7407f6c6efbffccb2/src/topology/pcm.c#L1400

marc-hb · 2022-01-12T06:55:07Z

I don't think it will cause any aplay error.

While you think, I test aplay.

I still think the aplay error you mentioned is caused by c687815

Absolutely none of the many test results and evidence I have on UP2 APL point at commit c687815

git bisect (with ALSA 1.2.2 of course) pointed at this codec_consumer rename instantly with zero ambiguity: aplay runs before it, fails after it.
reverting this codec_consumer rename while keeping c687815 fixes aplay for me.
reverting c687815 does NOT fix aplay for me.
c687815 and c687815~1 fail aplay exactly the same (cause they're both after this codec_consumer rename)

The hex diff 0101 is the value of bclk_master(u8) and fsync_master(u8) in the snd_soc_tplg_hw_config in the snd_soc_tplg_link_config.

Thanks! How would alsatplg 1.2.2 corrupting these values in the .tplg file (see revert PR #5192) NOT cause any issue?

Maybe commit c687815 has some problems but they are not problems I have and they are not related to this 1.2.2 + codec_consumer issue.

aiChaoSONG · 2022-01-12T06:57:31Z

How would alsatplg 1.2.2 corrupting these values in the .tplg file (see revert PR #5192) NOT cause any issue?

Root caused, see my comment in

Revert "topologies: switch all .m4 files to codec_provider and codec_consumer #5192 (review)

…consumer" This reverts commit f50b6fe. I discovered the hard way that this change causes alsatplg version 1.2.2 (the default version in the current LTS Ubuntu) to corrupt just a few bytes in .tplg output in an incredibly discrete and time-consuming way: no error message at build time and same file size. See example at thesofproject#5162. We could/should just require a minimum alsatplg version at the CMake level but at this moment we don't even know which minimum version is needed and we would also need to take some time to test a few alsatplg versions. If version 1.2.2 would just fail with an decent error message that can be searched and discussed then everything would be fine but silent corruption is really not OK. So users of recent versions will unfortunately have to live with the huge number of warnings for now. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

…consumer" This reverts commit f50b6fe. I discovered the hard way that this change causes alsatplg version 1.2.2 (the default version in the current LTS Ubuntu) to corrupt just a few bytes in .tplg output in an incredibly discrete and time-consuming way: no error message at build time and same file size. See example at #5162. We could/should just require a minimum alsatplg version at the CMake level but at this moment we don't even know which minimum version is needed and we would also need to take some time to test a few alsatplg versions. If version 1.2.2 would just fail with an decent error message that can be searched and discussed then everything would be fine but silent corruption is really not OK. So users of recent versions will unfortunately have to live with the huge number of warnings for now. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb mentioned this pull request Jan 6, 2022

test: topology: fix build with latest alsa-lib #5156

Merged

marc-hb marked this pull request as ready for review January 6, 2022 23:48

marc-hb requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners January 6, 2022 23:48

marc-hb marked this pull request as draft January 6, 2022 23:48

marc-hb requested review from aiChaoSONG, brentlu, cujomalainey, keqiaozhang, kv2019i, lyakh, macchian, perexg and ranj063 January 6, 2022 23:50

cujomalainey reviewed Jan 6, 2022

View reviewed changes

cujomalainey approved these changes Jan 6, 2022

View reviewed changes

ranj063 approved these changes Jan 7, 2022

View reviewed changes

greg-intel approved these changes Jan 7, 2022

View reviewed changes

marc-hb marked this pull request as ready for review January 8, 2022 18:46

lgirdwood merged commit f50b6fe into thesofproject:main Jan 10, 2022

marc-hb deleted the inclusive-codec branch January 10, 2022 18:55

marc-hb mentioned this pull request Jan 12, 2022

Revert "topologies: switch all .m4 files to codec_provider and codec_consumer #5192

Merged

aiChaoSONG removed request for brentlu, dbaluta, keqiaozhang, kv2019i, lbetlej, lgirdwood, lyakh, macchian, mmaka1, perexg and plbossart January 12, 2022 06:59

Conversation

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!

cujomalainey left a comment • 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 commented Jan 6, 2022

Uh oh!

cujomalainey commented Jan 7, 2022

Uh oh!

greg-intel left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

marc-hb commented Jan 8, 2022

Uh oh!

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

Uh oh!

Uh oh!

aiChaoSONG commented Jan 12, 2022

Uh oh!

marc-hb commented Jan 12, 2022

Uh oh!

aiChaoSONG commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

aiChaoSONG commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

marc-hb commented Jan 12, 2022

Uh oh!

aiChaoSONG commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marc-hb commented Jan 6, 2022 •

edited

Loading

marc-hb commented Jan 6, 2022 •

edited

Loading

cujomalainey left a comment •

edited

Loading

marc-hb commented Jan 7, 2022 •

edited

Loading

marc-hb commented Jan 12, 2022 •

edited

Loading

aiChaoSONG commented Jan 12, 2022 •

edited

Loading

marc-hb commented Jan 12, 2022 •

edited

Loading

aiChaoSONG commented Jan 12, 2022 •

edited

Loading

marc-hb commented Jan 12, 2022 •

edited

Loading

aiChaoSONG commented Jan 12, 2022 •

edited

Loading

marc-hb commented Jan 12, 2022 •

edited

Loading

aiChaoSONG commented Jan 12, 2022 •

edited by marc-hb

Loading