Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] topology: sof: import sof 1.4.1 topology files #1

Closed
wants to merge 1 commit into from

Conversation

juimonen
Copy link

Import topology files from sof v1.4.1.

Signed-off-by: Jaska Uimonen jaska.uimonen@linux.intel.com

@juimonen
Copy link
Author

@perexg do not merge this, I need comments from other sof folks first... These are the conf files from 1.4.1 taq.

@perexg
Copy link
Member

perexg commented Dec 12, 2019

@juimonen : Thank you for this. I will wait for the feedback.

@dbaluta
Copy link

dbaluta commented Dec 12, 2019

@juimonen @perexg we still need to figure out the correct names for the DAI links on i.MX8 topologies, as we already changed them 3 times until now. I do expect that this to be ready next week.

Also, the biggest problem I see is that pushing just the .conf files would be a pain. We don't really edit that files directly. The best way would be to import all .m4 files and have a script that generates the .conf file for releases.

Import topology files from Sound Open Firmware (sof) v1.4.1.

These files are generated from https://github.com/thesofproject/sof/
from tag v1.4.1. Script used to generate these is scripts/build-tools.sh
found in the same repository.

Signed-off-by: Jaska Uimonen <jaska.uimonen@linux.intel.com>
@juimonen
Copy link
Author

updated:

  • no change for conf files after first push
  • commit message changed to be more clear how and where these are generated

@perexg
Copy link
Member

perexg commented Dec 13, 2019

The files should be "normalized". My proposal is already in alsa-utils (alsatpl -n option). Ideally, it should be combined with -s (sort the compound identifiers), but it should be tested. I get different output binary file sizes with this combination.

EDIT: The sort option was added to keep text sorted for the diffs.

@perexg
Copy link
Member

perexg commented Dec 13, 2019

I found the reason why the binary file sizes differ. The fix is here: alsa-project/alsa-lib@2b50b59 . So, I would like to import only output from alsatplg -n /source-file.conf/ -s -o /normalized-file.conf/ only.

@perexg
Copy link
Member

perexg commented Dec 13, 2019

I would also recommend to add header to each file like:

# This file was generated using 'alsatplg -n /source-file.conf/ -s' from
# giturl: GIT_HTTPS_URL here
# gittag: branch/tag name here
# sha512: SHA512 of the original .conf file

@plbossart
Copy link

plbossart commented Dec 13, 2019

> I would also recommend to add header to each file like:
> 
> # This file was generated using 'alsatplg -n /source-file.conf/ -s' from
> # giturl: GIT_HTTPS_URL here
> # gittag: branch/tag name here
> # sha512: SHA512 of the original .conf file

and a pointer to the license file!

@perexg
Copy link
Member

perexg commented Dec 14, 2019

and a pointer to the license file!

Yes, if it differs from BSD 3-Clause..

@juimonen
Copy link
Author

@perexg @plbossart we don't have the conf files in git as they are generated from m4 macros.

If we are doing the alsatplg normalization, I think it should be added to "normal/basic" sof CMakefiles so that they get tested in our CI.

We can try also to include some form of "comment header" to all conf files in m4, so to describe where/how they are generated and possible the license.

I don't want the import to differ too much how we are compiling the topologies in sof.

@juimonen
Copy link
Author

@perexg so what I'm saying here, this will take me some time... I mailed our CI folks if we can try out the normalization for regressions, I can play around myself with the comment header generation, and then backport this to 1.4.1. So couple of days at least...

@plbossart
Copy link

@perexg I don't understand how we can deal with 'normalized' files. We can't track every single option of the tools and if there is a problem with the files they should be fixed at the M4 level, not reprocessed with the errors not fixed at the source.

@perexg
Copy link
Member

perexg commented Jan 3, 2020

@perexg I don't understand how we can deal with 'normalized' files. We can't track every single option of the tools and if there is a problem with the files they should be fixed at the M4 level, not reprocessed with the errors not fixed at the source.

Make a bold warning to the header in the normalized files that fixes should go to your repository.

I did a lot of cleanups in the topology library and alsatplg tool. All is pushed to alsa-lib and alsa-utils now. If you have any issue, let me know. The normalization is now much better. Also, the output can be grouped by the index (component domains) for the verification and I have to say, that the output is much more readable than your m4 processed configuration and we can do clear diffs now.

@juimonen
Copy link
Author

juimonen commented Jan 7, 2020

@perexg sorry about the delay... I think we are little bit puzzled about the normalization thing. So should the topologies work exactly same way if they are normalized or not? So they should be parsed the same? And the normalization here means that widgets are grouped somehow differently that helps diffing? I kind of missed the original issue you had.

Anyway this is kind of painful to test as we need to update the CI docker images for new alsa-lib and compare the results to original ones... Just explaining why this is taking time, we can't do this kind of tests very easily in our CI. So need to be sure this is the thing to do...

@perexg
Copy link
Member

perexg commented Jan 7, 2020

@juimonen : The topology files generated from the normalized files are same (in other words - they contain identical configuration for the driver), but there are some differences:

  1. I fixed DAPM widget sorting by index (domain) thus the DAPM widget blocks may be shorter with the new topology library; the -z,--dapm-nosort option turns this sorting off (library behaves like previously)
  2. normalization sorts keys; it means that the configuration blocks are shuffled in the final binary topology generated from the normalized file, but again - the information / configuration for the driver is same (I believe)

BTW: I added the binary topology decoder to the current alsatplg tool / topology library, too. The result is saved in the .conf file syntax. It might help to check the contents.

I cannot say that there are no bugs, but I tried to test the code with the current available topologies and checked the generated / decoded binary topologies.

@plbossart
Copy link

I really completely disagree with the idea of importing .conf files that are not maintained or even version-controlled by the SOF team.
It'd be a better idea to just provide a stand-alone script to convert M4 files into .tplg ones, as suggested by @lrgirdwo

@juimonen
Copy link
Author

juimonen commented Jan 7, 2020

@plbossart that still doesn't change the question, should this script normalize or not, so what should be the alsatplg switches and/or do we care? As I understood from @perexg, the final binary size is differing depending on these switches. I'm also just asking that the resulting tplg files are the same as we are testing in CI (normalized or not).

@perexg
Copy link
Member

perexg commented Jan 7, 2020

Again, the contents (information) does not change. If you normalize the .conf and create .tplg from this .conf file, the functionality must be identical. So you can test this in CI etc. I think that you just don't understand what I'm trying to propose. The m4 macros are not very readable anyway. Positional (instead named) arguments lose the readability etc...

I don't insist to use my solution, I'm just trying to give a tool to manage this for us (work with the text .conf files, decode the binary .tplg files back and allow the comparisons to check things with the original souce). If you have another idea, what distributions should do, I'm open for it. Happy new year to all.

@perexg
Copy link
Member

perexg commented Jan 7, 2020

The suggested path: m4 -> .conf -> normalized .conf -> .tplg -> testing/verification -> all pass -> push normalized .conf to upstream (controlled by the m4 or whatever text processor owner), ok?

alsatplg -n m4.conf -o norm.conf
alsatplg -c norm.conf -o my.tplg

@plbossart
Copy link

My alternative proposal is
m4 + script > .tplg -> testing/verification -> all pass -> push m4+script to upstream

No one needs to care about .conf files that are just intermediate representations and not intended to be used.

Note that we will at some point ditch M4 and have a more structured way of generating topologies, but the .conf will remain an intermediate step, not the end-goal

@perexg
Copy link
Member

perexg commented Jan 7, 2020

Quoting my reply to ML:

"The .conf files can be very easily compared with the original source.

Basically, I really want to let things to move forward. Everything is better
than to have the topology configuration in the big repo with all other things,
so I can accept this, too.

I had to probably use my time to work on something different than
libatopology, but at least I found several problems and did a lot
of cleanups.''

Please, open new PR, if you have something ready. Maybe, I can do this import myself.

@perexg perexg closed this Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants