UUID Firmware support #2920

keyonjie · 2020-05-09T04:10:08Z

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

keyonjie · 2020-05-09T09:25:05Z

the corresponding tplg PR: #2919
the corresponding Linux PR: thesofproject/linux#2215

lgirdwood

Please do the generic process comp type first. It should be usptream before any other patches are merged.

src/include/ipc/topology.h

src/audio/component.c

keyonjie · 2020-05-29T09:35:58Z

This is ready for review now. @lgirdwood @kv2019i @mmaka1 @ktrzcinx

ktrzcinx · 2020-05-29T09:48:23Z

src/audio/component.c

+		if (!rstrncmp((const char *)info->drv->uid,
+			      (const char *)uuid, UUID_SIZE)) {
+			tr_info(&comp_tr,
+				"get_drv_from_uuid(), found driver type %d, uuid %s",
+				info->drv->type, info->drv->uid);


There is some mismatch - UUID_SIZE is equal to 16, what is binary UUID number size, so it should be compared with memcmp and printing this value can't be done with %s, or when UUID should be passed in text form, then drv->uid size is too small.

The uuid is passed in binary mode for the comparing, the printing here is for the full text(including the naming).

why are we comparing strings ? I would expect us to compare the binary a.b.c.d data/struct

changed now.

src/platform/apollolake/apollolake.x.in

lgirdwood

@keyonjie @ktrzcinx we have several PRs now for UUID, I need you guys to work out the merge ordering so we dont break anything. Please add PR title prefix to [DO NO MERGE]xxxx if teh PR is blocking on other PRs, this can then be removed when it's ready to merge.

lgirdwood · 2020-05-29T15:18:43Z

src/include/ipc/topology.h

@@ -64,6 +64,7 @@ struct sof_ipc_comp {
 	enum sof_comp_type type;
 	uint32_t pipeline_id;
 	uint32_t core;
+	uint8_t uuid[16];	/**< 16 Bytes UUID of the component */


can we use the same a.b.c.d structure that @mmaka1 has already defined. This will need to be a new IPC struct too.

Andy from the kernel side suggest not to use uuid_t or guid_t struct as ABI, see here: thesofproject/linux#2090 (comment)

use the FW structure here exactly.

lgirdwood · 2020-05-29T15:20:13Z

src/audio/component.c

+		if (!rstrncmp((const char *)info->drv->uid,
+			      (const char *)uuid, UUID_SIZE)) {
+			tr_info(&comp_tr,
+				"get_drv_from_uuid(), found driver type %d, uuid %s",
+				info->drv->type, info->drv->uid);


why are we comparing strings ? I would expect us to compare the binary a.b.c.d data/struct

keyonjie · 2020-05-29T15:42:21Z

@lgirdwood It's in byte(or binary, not a(4B)/b(2B)/c(2B)/d(B * 8)) from the driver, so we are comparing bytes here actually.

lgirdwood

@ktrzcinx @keyonjie seems like we may be duplicating some patches, can you guys align. Thanks.

lgirdwood · 2020-06-01T09:08:56Z

src/platform/apollolake/apollolake.x.in

@@ -464,6 +460,11 @@ SECTIONS
    KEEP (*(.fw_ready_metadata))
  } >sof_fw :sof_fw_phdr

+  .static_uuid_entries : ALIGN(4)


Is this the same change that @ktrzcinx has just submitted ? Can you guys align.

No, this one is about .static_uuid_entries, I've made PR for .trace_ctx section.

ok, so we have any dependencies for merging ?

I don't think so

src/lib/lib.c

lgirdwood · 2020-06-01T09:23:07Z

src/include/ipc/topology.h

@@ -64,6 +64,7 @@ struct sof_ipc_comp {
 	enum sof_comp_type type;
 	uint32_t pipeline_id;
 	uint32_t core;
+	uint8_t uuid[16];	/**< 16 Bytes UUID of the component */


use the FW structure here exactly.

kv2019i

Implementation needs some iteration, but main principles seem right.

marc-hb

I think extended_data_length is a good trade-off between backward compatibility, protocol simplicity and code clarity, thanks! I think the sanity check ("validation" sounds a bit pretentious) can be both clearer and stricter, see below.

marc-hb · 2020-07-17T17:15:28Z

src/audio/component.c

+
+	/* validate the hdr.size */
+	if (comp->hdr.size < comp->ext_data_length) {
+		tr_err(&comp_tr, "Invalid size, hdr.size:0x%x, ext_data_length:0x%x\n",


Assuming I got this right, replace with:

// Basic sanity check of total length and extended data length. // In this generic code we don't know which specific comp and how much it adds if (comp->hdr.size < sizeof(sof_ipc_comp) + comp->ext_data_length) {

stricter so catches more errors

makes it clearer that hdr.size is actually hdr.total_size without changing its name.

ack - with C style comments.

@marc-hb, this is not included in your fixup commits?

marc-hb · 2020-07-17T17:27:57Z

src/include/ipc/header.h

@@ -86,10 +86,12 @@
 *     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 *     | Core                                                          |
 *     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- *     | Reserved                                                      |
+ *     | ext_data_length                                               |


I doubt ext_data_length will ever need 32bits / 4GB, will it? If not then some bits should be kept reserved.

PS: thanks to little-endianness these bits could easily be reserved again even after this gets merged.

Lets use full extended size - anything new here will be added to extended data and this keeps it simple prior to IPC2.0

@marc-hb I agree with Liam. 32bit fields are faster to parse on the DSP and if we need to add something more, we now have the extended data defined, so you can add any new fields at end of extended data.

marc-hb · 2020-07-21T07:09:05Z

I shared 2-3 fixup! commits at https://github.com/marc-hb/sof/commits/uuid-2020-07-21

lgirdwood · 2020-07-21T10:25:39Z

I shared 2-3 fixup! commits at https://github.com/marc-hb/sof/commits/uuid-2020-07-21

Thank you @marc-hb, these all look good. @keyonjie can you squash these into the PR.

marc-hb · 2020-07-21T14:57:24Z

In these fixups I removed the word "new" from struct sof_ipc_comp_new_ext. However based on a more recent comment in the corresponding kernel review (which we want to keep consistent), I'm not 100% sure I understood "new"

keyonjie · 2020-07-22T04:25:23Z

I shared 2-3 fixup! commits at https://github.com/marc-hb/sof/commits/uuid-2020-07-21

Thank you @marc-hb, these all look good. @keyonjie can you squash these into the PR.

Sure. I prefer to rename the sof_ipc_comp_ext_data to sof_ipc_comp_ext to avoid too long naming, is that sounds good for you @liam and @marc?

liam · 2020-07-22T07:28:19Z

Doesn't make much difference to me. It's still too long.

…

On Wed, 22 Jul 2020, 05:25 Keyon Jie, ***@***.***> wrote: I shared 2-3 fixup! commits at https://github.com/marc-hb/sof/commits/uuid-2020-07-21 Thank you @marc-hb <https://github.com/marc-hb>, these all look good. @keyonjie <https://github.com/keyonjie> can you squash these into the PR. Sure. I prefer to rename the sof_ipc_comp_ext_data to sof_ipc_comp_ext to avoid too long naming, is that sounds good for you @liam <https://github.com/liam> and @marc <https://github.com/marc>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2920 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGRFHYNUT2GRVBQZRTYEDR4ZS4BANCNFSM4M4UDAEA> .

lgirdwood · 2020-07-22T08:49:49Z

@marc-hb removing "new" is good.
@keyonjie short name is fine.

keyonjie · 2020-07-22T09:11:42Z

@marc-hb removing "new" is good.
@keyonjie short name is fine.

OK, let me squashing and updating them now. @marc-hb for the documentation update, I would prefer to keep the name of each member and add the corresponding explain to the subsequent '()', which will be easier for understanding while reading document with the struct definition header file together.

lgirdwood

@plbossart @ranj063 @keyonjie how is the kernel side looking ?

lgirdwood · 2020-07-22T14:45:16Z

CI bat sounds fine at 997.

keyonjie · 2020-07-23T01:34:42Z

@plbossart @ranj063 @keyonjie how is the kernel side looking ?

The kernel side is also updated and aligned with FW, @plbossart @ranj063 can you help to review as @kv2019i won't be available in the coming 2 weeks?

marc-hb

I prefer to rename the sof_ipc_comp_ext_data to sof_ipc_comp_ext to avoid too long naming, is that sounds good for you @liam and @marc?

Either is fine by me. Just for the record sof_ipc_comp_ext_data was only one character longer than the original sof_ipc_comp_new_ext! :-)

It's still too long.

C lacks namespaces.

@marc-hb for the documentation update, I would prefer to keep the name of each member and add the corresponding explain to the subsequent '()',

While this should certainly not hold back this PR, I disagree and for at least for IPC2.0 let me explain why. The purpose of the protocol documentation is to take a bit of distance with respect to the too short or too long C-symbol-of-the-day or other implementation details and use plain English instead to describe and focus on the protocol design, on the corresponding high level logic and on any corner cases. This is how @kv2019i started this document and the ext_data_length symbol you're adding is inconsistent with all the other lines in the same table.

which will be easier for understanding while reading document with the struct definition header file together.

In general, if someone finds it difficult to match the protocol description in plain English with the code when they are side by side (even though the fields are in the exact same order!) then it means there is a much bigger problem: either the English is really bad, or the symbol names are bad, or they both are.

In this particular case I can't imagine anyone struggling to match the "Extended Data Size" in English with extended_data_length in the code.

liam · 2020-07-23T05:43:24Z

Hello, please remove me from this mailing list. Thanks, Liam

…

On Thu, 23 Jul 2020, 02:37 Marc, ***@***.***> wrote: ***@***.**** approved this pull request. I prefer to rename the sof_ipc_comp_ext_data to sof_ipc_comp_ext to avoid too long naming, is that sounds good for you @liam <https://github.com/liam> and @marc <https://github.com/marc>? Either is fine by me. Just for the record sof_ipc_comp_ext_data was only one character longer than the original sof_ipc_comp_new_ext! :-) It's still too long. C lacks namespaces. @marc-hb <https://github.com/marc-hb> for the documentation update, I would prefer to keep the name of each member and add the corresponding explain to the subsequent '()', While this should certainly not hold back this PR, I disagree and for at least for IPC2.0 let me explain why. The purpose of the protocol documentation is to take a bit of distance with respect to the too short or too long C-symbol-of-the-day or other implementation details and use plain English instead to describe and focus on the protocol design, on the corresponding high level logic and on any corner cases. This is how @kv2019i <https://github.com/kv2019i> started this document and the ext_data_length symbol you're adding is inconsistent with all the other lines in the same table. which will be easier for understanding while reading document with the struct definition header file together. In general, if someone finds it difficult to match the protocol description in plain English with the code when they are side by side (even though the fields are in the exact same order!) then it means there is a much bigger problem: either the English is really bad, or the symbol names are bad, or they both are. In this particular case I can't imagine anyone struggling to match the "Extended Data Size" in English with extended_data_length in the code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2920 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGRFH6ULCPRU5WDKQ3IO3R46H6RANCNFSM4M4UDAEA> .

cujomalainey · 2020-07-23T16:42:42Z

@liam apologies, you can unsubscribe yourself using the web view and clicking "unsubscribe" near the bottom of the right hand menu

keyonjie · 2020-07-24T01:25:14Z

@liam apologies, you can unsubscribe yourself using the web view and clicking "unsubscribe" near the bottom of the right hand menu

Sorry @liam I tagged you by mistake but I can't stop github from sending related email to you, can you unsubscribe or unwatch to fix that?

Initial the struct sof_ipc_comp_file to make sure all no updated flags are 0s. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Initial the definition of struct sof_ipc_comp_new_ext, which will be used to align the extended data between FW and the SW, add UUID array to it. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> Signed-off-by: Marc Herbert <marc.herbert@intel.com>

Use the 32bit reserved member of the struct sof_ipc_comp as the extended data length, this will help to minimize the ABI change for adding new extended data to the struct sof_ipc_comp, usually only minor ABI version bump needed for every update with this new solution. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Add function memcmp for comparing two length-limited memory. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Switch to use UUID for component creation, if it is provided from the host, otherwise, use component type for the component driver matching. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> Signed-off-by: Marc Herbert <marc.herbert@intel.com>

Update the IPC protocol documentation to include the extended data. Signed-off-by: Marc Herbert <marc.herbert@intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

lgirdwood · 2020-08-04T19:53:20Z

SOFCI

lgirdwood · 2020-08-04T19:53:46Z

Will apply after CI completes now that we have both FW and kernel dev approvals.

lgirdwood · 2020-08-05T08:46:39Z

Internal CI showing all green on build and scheduler tests. Jenkins showing unrelated alsabat failure. @zrombel I assume internal CI is good to merge ?

zrombel · 2020-08-05T13:33:10Z

@lgirdwood yes, from internal CI view, PR is good to go.

keyonjie requested review from dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart, tlauda and a team as code owners May 9, 2020 04:10

keyonjie force-pushed the uuid branch from fe86ab3 to a91889b Compare May 9, 2020 08:39

keyonjie mentioned this pull request May 9, 2020

UUID Kernel support thesofproject/linux#2090

Closed

keyonjie mentioned this pull request May 9, 2020

UUID tplg support #2919

Merged

lgirdwood requested changes May 18, 2020

View reviewed changes

src/include/ipc/topology.h Outdated Show resolved Hide resolved

src/include/ipc/topology.h Outdated Show resolved Hide resolved

src/audio/component.c Outdated Show resolved Hide resolved

keyonjie mentioned this pull request May 29, 2020

Generic process implementation #2956

Closed

keyonjie changed the title ~~[RFC]UUID Firmware support~~ UUID Firmware support May 29, 2020

keyonjie force-pushed the uuid branch from a91889b to d5c9918 Compare May 29, 2020 09:32

keyonjie requested review from libinyang, lyakh, RanderWang and xiulipan as code owners May 29, 2020 09:32

ktrzcinx reviewed May 29, 2020

View reviewed changes

src/platform/apollolake/apollolake.x.in Outdated Show resolved Hide resolved

keyonjie force-pushed the uuid branch from d5c9918 to 20be266 Compare May 29, 2020 10:16

lgirdwood requested changes May 29, 2020

View reviewed changes

lgirdwood reviewed Jun 1, 2020

View reviewed changes

kv2019i reviewed Jun 1, 2020

View reviewed changes

This was referenced Jun 2, 2020

sof: Define possible UUID entries usage in IPC during declaration #3010

Merged

trace: Put global trace context entries to separate section #2996

Merged

marc-hb reviewed Jul 17, 2020

View reviewed changes

keyonjie force-pushed the uuid branch from 59b0c15 to 8499183 Compare July 22, 2020 09:21

lgirdwood approved these changes Jul 22, 2020

View reviewed changes

marc-hb approved these changes Jul 23, 2020

View reviewed changes

ranj063 approved these changes Jul 29, 2020

View reviewed changes

keyonjie force-pushed the uuid branch from 8499183 to fa32944 Compare July 30, 2020 08:04

keyonjie and others added 6 commits July 30, 2020 16:04

testbench: topology: initial the sof_ipc_comp_file with 0s

ec7c215

Initial the struct sof_ipc_comp_file to make sure all no updated flags are 0s. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

lib: add memcmp for memory comparing

2f34d77

Add function memcmp for comparing two length-limited memory. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

ipc: update documentation to include extended data

fa32944

Update the IPC protocol documentation to include the extended data. Signed-off-by: Marc Herbert <marc.herbert@intel.com> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

lgirdwood merged commit cf74566 into thesofproject:master Aug 5, 2020

UUID Firmware support #2920

UUID Firmware support #2920

Conversation

keyonjie commented May 9, 2020 • edited Loading

keyonjie commented May 9, 2020 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

keyonjie commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyonjie commented May 29, 2020

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Jul 21, 2020

lgirdwood commented Jul 21, 2020

marc-hb commented Jul 21, 2020

keyonjie commented Jul 22, 2020

liam commented Jul 22, 2020 via email

lgirdwood commented Jul 22, 2020

keyonjie commented Jul 22, 2020

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood commented Jul 22, 2020

keyonjie commented Jul 23, 2020

marc-hb left a comment

Choose a reason for hiding this comment

liam commented Jul 23, 2020 via email

cujomalainey commented Jul 23, 2020

keyonjie commented Jul 24, 2020

lgirdwood commented Aug 4, 2020

lgirdwood commented Aug 4, 2020

lgirdwood commented Aug 5, 2020

zrombel commented Aug 5, 2020

keyonjie commented May 9, 2020 •

edited

Loading

keyonjie commented May 9, 2020 •

edited

Loading