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

UUID tplg support #2919

Merged
merged 18 commits into from
Aug 13, 2020
Merged

UUID tplg support #2919

merged 18 commits into from
Aug 13, 2020

Conversation

keyonjie
Copy link
Contributor

@keyonjie keyonjie commented May 9, 2020

This is the series for UUID support -- tplg part, it should be used together with the FW part and kernel part.

@keyonjie
Copy link
Contributor Author

keyonjie commented May 9, 2020

the corresponding Firmware PR: #2920
the corresponding Linux PR: thesofproject/linux#2215

` tokens "sof_comp_tokens"'
` tuples."uuid" {'
` SOF_TKN_COMP_UUID' "c4:b2:68:68:14:30:47:0e:a0:89:15:d1:c7:7f:85:1a"
` }'
Copy link
Member

Choose a reason for hiding this comment

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

It's better here to have a uuid.m4 file that contains all UUIDs and is included by all topologies.

# uuid.m4 - set default UUIDs if NOT already set in topology.
# allows users to override the UUID if needed.
ifndef UUID_SRC
define UUID_SRC "blah blah blah"
endif

# and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, will implement a m4 helper to generate this UUID byte array from the string copied from the firmware source directly.

`SectionVendorTuples."'N_MUXDEMUX($1)`_tuples_uuid" {'
` tokens "sof_comp_tokens"'
` tuples."uuid" {'
` SOF_TKN_COMP_UUID' "c4:b2:68:68:14:30:47:0e:a0:89:15:d1:c7:7f:85:1a"
Copy link

Choose a reason for hiding this comment

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

Could you document the format used for uuid in this token?
If this is just a byte array, is endianess correct? There are uint32, uint32, uint8.
Example from the fw source code:

/* c4b26868-1430-470e-a089-15d1c77f851a */
DECLARE_SOF_UUID("demux", demux_uuid, 0xc4b26868, 0x1430, 0x470e,
		 0xa0, 0x89, 0x15, 0xd1, 0xc7, 0x7f, 0x85, 0x1a);

So if it is byte array and little-endian is assumed, it should rather be 68:68:b2:c4:30:14....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, thanks for spotting this.

@keyonjie keyonjie changed the title [RFC]UUID tplg support UUID tplg support May 28, 2020
@keyonjie keyonjie requested a review from lgirdwood May 28, 2020 08:51
@keyonjie
Copy link
Contributor Author

@lgirdwood @mmaka1 I want to let this (adding UUID tokens to toplogies) go first, they are verified with the corresponding FW and kernel changes, but the topology changes can go first as they won't be used if driver and FW is not ready yet, no any regression with these changes applied.

@keyonjie
Copy link
Contributor Author

keyonjie commented May 28, 2020

Not all components are covered until now, there will be more commits added for them, e.g. asrc/buffer/dai/dcblock/eq/mixer/pcm/pga/src/tone...

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

I guess there is nothing stopping this being merged after kernel token is updated ? i.e. it's not used so wont change anything ?

@keyonjie
Copy link
Contributor Author

I guess there is nothing stopping this being merged after kernel token is updated ? i.e. it's not used so wont change anything ?

Right. We can merge this even before the kernel token is updated, which means we provide those tokens in tplg, but the kernel doesn't parse them so nothing will change to others.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

What's the status of the kernel PR for the new token ID ?

@lgirdwood lgirdwood added the ABI ABI change involved label Jun 1, 2020
@keyonjie
Copy link
Contributor Author

keyonjie commented Jun 1, 2020

What's the status of the kernel PR for the new token ID ?

That is the third commit of the driver PR, thesofproject/linux@04a7e7a

@kv2019i @plbossart I think we are all aligned on adding this token on both tplg and driver side?

@plbossart
Copy link
Member

What's the status of the kernel PR for the new token ID ?

That is the third commit of the driver PR, thesofproject/linux@04a7e7a

@kv2019i @plbossart I think we are all aligned on adding this token on both tplg and driver side?

I am not, there are 4 parallel threads (sof-developers, sof-docs, sof, linux), it's beyond my reviewer ability quite frankly to figure out what the big picture looks like.

@kv2019i
Copy link
Collaborator

kv2019i commented Jun 1, 2020

@plbossart wrote:

I am not, there are 4 parallel threads (sof-developers, sof-docs, sof, linux), it's beyond my reviewer ability quite frankly to figure out what the big picture looks like.

I suggest looking at the sof-docs:
thesofproject/sof-docs#245

.. let's all align on that first. If we all approve, then reviewing the others should be easier as they should all comply with the sof-docs. Ok @plbossart @keyonjie @lgirdwood ?

@keyonjie
Copy link
Contributor Author

keyonjie commented Jun 2, 2020

@plbossart wrote:

I am not, there are 4 parallel threads (sof-developers, sof-docs, sof, linux), it's beyond my reviewer ability quite frankly to figure out what the big picture looks like.

I suggest looking at the sof-docs:
thesofproject/sof-docs#245

.. let's all align on that first. If we all approve, then reviewing the others should be easier as they should all comply with the sof-docs. Ok @plbossart @keyonjie @lgirdwood ?

Good point, ack.

Move the including of utils.m4 to first as it is needed by other
component m4 files.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to smart_amp widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to muxdemux widgets for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to detect widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to ch_sel widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to kpb widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to asrc widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to dai widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to dcblock widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to eq_fir widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to eq_iir widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to mixer widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to host widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to volume widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to src widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Add the uuid token to tone widget for the future use.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
@lgirdwood
Copy link
Member

@zrombel can you check CI (nothing is showing red) - I'm told by @keyonjie that the test should be passing.
@keyonjie does the CI needs updated for this test to pass now ?

@keyonjie
Copy link
Contributor Author

@zrombel can you check CI (nothing is showing red) - I'm told by @keyonjie that the test should be passing.
@keyonjie does the CI needs updated for this test to pass now ?

I can't see the QB log, instead seeing all SUCCESS while it reports Tools failed. And for the alsa-lib version in CI, yes, let's align using latest version for building.

@keyonjie
Copy link
Contributor Author

@slawblauciak could you help to take a look?

@keyonjie
Copy link
Contributor Author

SOFCI TEST

@keyonjie
Copy link
Contributor Author

@lgirdwood , @xiulipan told me that it is known issue for QB on TGL, so I think we are good to go with this.

@keyonjie
Copy link
Contributor Author

@zrombel please make sure the QB server is using alsa-lib later than v1.2.1

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 11, 2020

@zrombel please make sure the QB server is using alsa-lib later than v1.2.1

I have a branch that lets you build 1.2.3.2 .deb files for Ubuntu in two commands:
thesofproject/linux#2211 (comment)

Feedback appreciated there.

@keyonjie
Copy link
Contributor Author

@zrombel forgot to mention, please upgrade both alsa-lib and alsa-utils to the same version v1.2.3.

@zrombel
Copy link

zrombel commented Aug 13, 2020

Process Team is working on updating QB machines with newest alsa-lib and utils. We were able to run one test build on updated machine on this PR and it'a PASS so this PR can be merged. Rest of the machines are still under update. QB should be fully ready at the end of the day.

@keyonjie
Copy link
Contributor Author

Thanks @zrombel . @lgirdwood Let's merge this and then kernel PR will go together.

@keyonjie
Copy link
Contributor Author

SOFCI TEST

@zrombel
Copy link

zrombel commented Aug 13, 2020

QB CI is now fully updated.

@keyonjie keyonjie merged commit 624faa9 into thesofproject:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI ABI change involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants