Generic process implementation #2956

keyonjie · 2020-05-20T04:31:55Z

This is a simple series to support generic process component as described in:
https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32

UUID support is not added yet, but this is sufficient to unblock adding new process components like crossover, beamformer, ...

The kernel corresponding PR is: thesofproject/linux#2129

After this series is applied, all existed process components including mux/demux/eq/kpb/detect_test/selector/smart_amp/dcblock will be switched to this generic process type automatically.

The ABI minor will be bumped to 17 with this change.

keyonjie · 2020-05-20T04:39:58Z

@lgirdwood @mmaka1 @kv2019i I am still using 32bit subtype as the 16Bytes UUID dereference is not implemented yet inside the FW, if we decide to go with that, we can add incremental changes once it is implemented.

kv2019i

Needs to be added to ABI classifier, but otherwise looks good.

dbaluta · 2020-05-20T13:34:18Z

src/audio/component.c

-		       comp->type);
-		return NULL;
+		tr_info(&comp_tr, "comp_new(), no type %d, fallback to use subtype",
+			comp->type);


I think this should be tr_dbg instead of tr_info, when we will have full support for traces this will make a difference.

I wanted this message to be logged most of the time, that's why I uses tr_info, not tr_dbg.

lgirdwood

We need to use this in conjunction with passing full UUID in constructing the topology.

lgirdwood · 2020-05-20T13:32:19Z

src/include/ipc/topology.h

@@ -47,6 +47,8 @@ enum sof_comp_type {
 	SOF_COMP_ASRC,		/**< Asynchronous sample rate converter */
 	SOF_COMP_DCBLOCK,
 	SOF_COMP_SMART_AMP,		/**< smart amplifier component */
+
+	SOF_COMP_PROCESS = 1000,        /**< generic process component */


Can you add.
/*No more _COMP_ types to be added. Use SOF_COMP_PROCESS from now on.

Sure thing.

lgirdwood · 2020-05-20T13:33:43Z

src/include/ipc/topology.h

@@ -67,8 +67,7 @@ struct sof_ipc_comp {
 	uint32_t pipeline_id;
 	uint32_t core;

-	/* reserved for future use */
-	uint32_t reserved[1];
+	uint32_t subtype;       /**< favour for generic component type */


This should be using UUID now. There's no pint in introducing a short lived ABI change.

@lgirdwood as I stated above, the problem is that the 16Bytes UUID will not be accepted (Firmware can't understand it) by today's firmware.

If it needs a new IPC then we add a new IPC. This subtype is just the old method.

dbaluta · 2020-05-20T13:35:42Z

@keyonjie lets make sure that the link https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32 to github discussion is pasted in one of the commits.

This could be helpful for someone trying to understand the reason behind changes later in time.

keyonjie · 2020-05-20T14:33:57Z

@keyonjie lets make sure that the link https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32 to github discussion is pasted in one of the commits.

This could be helpful for someone trying to understand the reason behind changes later in time.

Good idea, will do that.

keyonjie · 2020-05-20T15:14:01Z

We need to use this in conjunction with passing full UUID in constructing the topology.

@lgirdwood the 16Bytes UUID will not be accepted (Firmware can't understand it) by today's firmware, so that is still open to me, looks we have to introduce this maybe not-so-short-lived ABI change?

lgirdwood · 2020-05-20T21:24:13Z

We need to use this in conjunction with passing full UUID in constructing the topology.

@lgirdwood the 16Bytes UUID will not be accepted (Firmware can't understand it) by today's firmware, so that is still open to me, looks we have to introduce this maybe not-so-short-lived ABI change?

The change to UUID should have no intermediate states, it should be done atomically in one ABI MINOR update. The new IPC can still carry the old "type" value to help with backwards compat, but the UUID should be introduced alongside the PROCESS_TYPE update.

keyonjie · 2020-05-21T02:01:48Z

We need to use this in conjunction with passing full UUID in constructing the topology.

@lgirdwood the 16Bytes UUID will not be accepted (Firmware can't understand it) by today's firmware, so that is still open to me, looks we have to introduce this maybe not-so-short-lived ABI change?

The change to UUID should have no intermediate states, it should be done atomically in one ABI MINOR update. The new IPC can still carry the old "type" value to help with backwards compat, but the UUID should be introduced alongside the PROCESS_TYPE update.

So, summarize the several solutions here:

share the full 16Bytes UUIDs between FW/SW.
This is not ready in the FW side, we need to change to store the full UUID for each component, that is about extra 54 * (16 - 4) = 648 Bytes consumed of the FW SRAM and the number will keep increasing, this could be challenge for e.g. byt/cht platforms. @ktrzcinx @mmaka1 can you comment? Need your help to implement this first if we decide to go with this.
share only the 32bit UUID key between FW/SW.
This is what I did with the "[RFC] UUID xxx" PRs. We need all @ktrzcinx 's extended manifest PRs being merged as the prerequisite of this solution.
not use (or depending on) UUID at all.
This is what I am doing with this series, we can use the type + subtype to implement the exact same feature, no need for 32b or 16B UUID introduction at all.

The solution 2 above was what @mmaka1 and @ktrzcinx wanted and implemented and I believe parts (or most?) of the code changes are already merged on FW/rimage/kernel sides.

So I think the solution 1 is what you wanted @lgirdwood ? It is somewhat like 2 (use the UUID concept to try to fix the "possible" existed flavor collision) + 3 (UUID dictionary in extended manifest not needed). I don't think the 16Bytes UUID introduction here can benefit us anything, the 32bit subtype is good enough:

No matter UUID or subtype, we need to pre-define (before build time) them in both FW source and topology .h header files, and the pre-define for subtype (32bit enum, like the component type we are using today) can avoid the collision better than the 16bytes UUID.

So I will suggest/advertise the solution 3 above if you want the solution 1, @lgirdwood @kv2019i , I can accept solution 2, but I like 3 most.

Add SOF_COMP_PROCESS as generic process component type, which can be used for all process components (e.g. demux, eq, detect_test, smart_amp, ...) in the future. This and the subsequent commits are made to support generic process component as described in: https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32 The corresponding FW PR is: thesofproject/sof#2956. After this series is applied, all existed process/effect components including mux/demux/eq/kpb/detect_test/selector/smart_amp/dcblock will be switched to this generic process type automatically. The ABI minor will be bumped to 17 to reflect this change. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

lgirdwood · 2020-05-21T09:13:57Z

@keyonjie please do 1 above. This requires changes to

All topology component M4 files need to include the component UUID (ASCII format is fine, but this should be 16 byte format in binary tplg file).
Kernel should send new TPLG comp new() IPC if UUID is found in topology otherwise it sends existing comp new IPC.
existing comp new IPC and comps types are supported until transition tto IPC2.0 (MAJOR ABI update).
If FW received new IPC then it does the UUID lookup to get component otherwise it does existing comp lookup.

Add SOF_COMP_PROCESS as generic process component type, which can be used for all process components (e.g. demux, eq, detect_test, smart_amp, ...) in the future. This and the subsequent commits are made to support generic process component as described in: https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32 The corresponding Linux PR is: thesofproject/linux#2129. After this series is applied, all existed process/effect components including mux/demux/eq/kpb/detect_test/selector/smart_amp/dcblock will be switched to this generic process type automatically. The ABI minor will be bumped to 17 to reflect this change. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Add subtype member to struct sof_ipc_comp, which will be used as flavour of some generic component type like process. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

In comp_new(), match the component driver with comp->type first, if no driver matched (e.g. for SOF_COMP_PROCESS type), will try to match with subtype (e.g. for demux/detect_test/eq/smart_amp...). With this change applied, if the new generic SOF_COMP_PROCESS type and subtype entry address are passed from the driver, we can get the correct driver for it, all these are verified with demux component. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

Now, generic process/effect component is implemented, bump the ABI minor version to 17 for that. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2020-05-21T09:29:01Z

@keyonjie please do 1 above. This requires changes to

All topology component M4 files need to include the component UUID (ASCII format is fine, but this should be 16 byte format in binary tplg file).

Kernel should send new TPLG comp new() IPC if UUID is found in topology otherwise it sends existing comp new IPC.

existing comp new IPC and comps types are supported until transition tto IPC2.0 (MAJOR ABI update).

All these 3 items were actually initially implemented with my "[RFC] UUID xxx" series, though I change to use the 32b UUID key later on. So this will be very easy to do.

If FW received new IPC then it does the UUID lookup to get component otherwise it does existing comp lookup.

This 4th item is the real trouble, need @mmaka1 @ktrzcinx 's help on it.

lgirdwood · 2020-05-21T13:13:44Z

If FW received new IPC then it does the UUID lookup to get component otherwise it does existing comp lookup.

This 4th item is the real trouble, need @mmaka1 @ktrzcinx 's help on it.

@keyonjie why is this difficult ? What are we missing that make it's a blocker today ?

keyonjie · 2020-05-22T10:05:28Z

If FW received new IPC then it does the UUID lookup to get component otherwise it does existing comp lookup.

This 4th item is the real trouble, need @mmaka1 @ktrzcinx 's help on it.

@keyonjie why is this difficult ? What are we missing that make it's a blocker today ?

We don't store the full UUID inside the ADSP SRAM at the moment, so unable to use the UUID to do the lookup.

lgirdwood · 2020-05-25T08:49:37Z

We don't store the full UUID inside the ADSP SRAM at the moment, so unable to use the UUID to do the lookup.

@keyonjie sounds like a minor change. You need to put them in RODATA or another section.

ktrzcinx · 2020-05-25T08:57:10Z

To add UUID to RODATA it should be enough to change:

  .static_uuid_entries (COPY) : ALIGN(1024)
  {
    *(*.static_uuids)
  } > static_uuid_entries_seg :static_uuid_entries_phdr

to

   .static_uuid_entries : ALIGN(4)
  {
    *(*.static_uuids)
  } >sof_fw :sof_fw_phdr

plus removing static_uuid_entries_seg, UUID_ENTRY_ELF_BASE and UUID_ENTRY_ELF_SIZE

*) I didn't test it

keyonjie · 2020-05-26T06:06:40Z

UUID_ENTRY_ELF_BASE

Thanks for this, but if we want to put it into the RODATA section, we should do change like this, no?

diff --git a/src/include/sof/lib/uuid.h b/src/include/sof/lib/uuid.h
index 022697cc1..d798ae0d4 100644
--- a/src/include/sof/lib/uuid.h
+++ b/src/include/sof/lib/uuid.h
@@ -83,7 +83,6 @@ struct sof_uuid_entry {
 #define DECLARE_SOF_UUID(entity_name, uuid_name,                       \
                         va, vb, vc,                                    \
                         vd0, vd1, vd2, vd3, vd4, vd5, vd6, vd7)        \
-       __section(".static_uuids")                                      \
        static const struct sof_uuid_entry uuid_name = {                \
                {.a = va, .b = vb, .c = vc,                             \
                 .d = {vd0, vd1, vd2, vd3, vd4, vd5, vd6, vd7}},        \

keyonjie · 2020-05-26T06:11:51Z

then we might need to change the ldc.c and smex.c also, the uuid entries will not be organized to be a separated section anymore.

ktrzcinx · 2020-05-26T06:27:53Z

@keyonjie If UUID entries won't be in separate section, then generating UUID dictionary for ldc file will be awkward process.

keyonjie · 2020-05-26T07:06:18Z

@ktrzcinx with your method mentioned above, the UUID entries is arranged in a separated section started from 0xbe120000 is out of the .rodata section range (0xbe052000~0xbe06e028) so I suppose it won't be loaded into SRAM.

keyonjie · 2020-05-26T07:12:04Z

And back to the original, if the UUID dictionary is not reliable at dynamic module load/unload scenarios, does the UUID dictionary still valuable, shall we remove this part and not generate it in either ldc or extended manifest?

ktrzcinx · 2020-05-26T07:39:02Z

@ktrzcinx with your method mentioned above, the UUID entries is arranged in a separated section started from 0xbe120000 is out of the .rodata section range (0xbe052000~0xbe06e028) so I suppose it won't be loaded into SRAM.

@keyonjie yes, it's out of .rodata section, because it's in .static_uuid_entries section, but both of them are loaded into .sof_fw segment. In the same manner .fw_ready section is organized.

And back to the original, if the UUID dictionary is not reliable at dynamic module load/unload scenarios, does the UUID dictionary still valuable, shall we remove this part and not generate it in either ldc or extended manifest?

our trace system already use this UUID dictionary from ldc file to format trace logs.

keyonjie · 2020-05-26T07:48:04Z

@ktrzcinx with your method mentioned above, the UUID entries is arranged in a separated section started from 0xbe120000 is out of the .rodata section range (0xbe052000~0xbe06e028) so I suppose it won't be loaded into SRAM.

@keyonjie yes, it's out of .rodata section, because it's in .static_uuid_entries section, but both of them are loaded into .sof_fw segment. In the same manner .fw_ready section is organized.

OK, then it's fine, let me try it. Thanks.

And back to the original, if the UUID dictionary is not reliable at dynamic module load/unload scenarios, does the UUID dictionary still valuable, shall we remove this part and not generate it in either ldc or extended manifest?

our trace system already use this UUID dictionary from ldc file to format trace logs.

keyonjie · 2020-05-29T08:39:17Z

@lgirdwood @kv2019i @dbaluta @ktrzcinx I have figured out the solution and will push changes to #2920, will close the PR here, thanks.

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

keyonjie mentioned this pull request May 20, 2020

Generic process implementation thesofproject/linux#2129

Closed

kv2019i reviewed May 20, 2020

View reviewed changes

lgirdwood added the ABI ABI change involved label May 20, 2020

dbaluta reviewed May 20, 2020

View reviewed changes

lgirdwood requested changes May 20, 2020

View reviewed changes

keyonjie force-pushed the generic_process branch from 2a54431 to d46ccc6 Compare May 21, 2020 09:22

keyonjie added 4 commits May 21, 2020 17:22

ipc:: add subtype to sof_ipc_comp

361382e

Add subtype member to struct sof_ipc_comp, which will be used as flavour of some generic component type like process. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

ABI: bump the ABI minor version to 17

d46ccc6

Now, generic process/effect component is implemented, bump the ABI minor version to 17 for that. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie closed this May 29, 2020

keyonjie mentioned this pull request Jun 1, 2020

UUID usage on SOF thesofproject/sof-docs#245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic process implementation #2956

Generic process implementation #2956

keyonjie commented May 20, 2020 •

edited

Loading

keyonjie commented May 20, 2020

kv2019i left a comment

dbaluta May 20, 2020

keyonjie May 20, 2020

lgirdwood left a comment

lgirdwood May 20, 2020

keyonjie May 20, 2020

lgirdwood May 20, 2020

keyonjie May 20, 2020

lgirdwood May 20, 2020

dbaluta commented May 20, 2020

keyonjie commented May 20, 2020

keyonjie commented May 20, 2020

lgirdwood commented May 20, 2020

keyonjie commented May 21, 2020

lgirdwood commented May 21, 2020 •

edited

Loading

keyonjie commented May 21, 2020

lgirdwood commented May 21, 2020

keyonjie commented May 22, 2020

lgirdwood commented May 25, 2020

ktrzcinx commented May 25, 2020

keyonjie commented May 26, 2020

keyonjie commented May 26, 2020

ktrzcinx commented May 26, 2020

keyonjie commented May 26, 2020

keyonjie commented May 26, 2020

ktrzcinx commented May 26, 2020

keyonjie commented May 26, 2020

keyonjie commented May 29, 2020

Generic process implementation #2956

Generic process implementation #2956

Conversation

keyonjie commented May 20, 2020 • edited Loading

keyonjie commented May 20, 2020

kv2019i left a comment

Choose a reason for hiding this comment

dbaluta May 20, 2020

Choose a reason for hiding this comment

keyonjie May 20, 2020

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood May 20, 2020

Choose a reason for hiding this comment

keyonjie May 20, 2020

Choose a reason for hiding this comment

lgirdwood May 20, 2020

Choose a reason for hiding this comment

keyonjie May 20, 2020

Choose a reason for hiding this comment

lgirdwood May 20, 2020

Choose a reason for hiding this comment

dbaluta commented May 20, 2020

keyonjie commented May 20, 2020

keyonjie commented May 20, 2020

lgirdwood commented May 20, 2020

keyonjie commented May 21, 2020

lgirdwood commented May 21, 2020 • edited Loading

keyonjie commented May 21, 2020

lgirdwood commented May 21, 2020

keyonjie commented May 22, 2020

lgirdwood commented May 25, 2020

ktrzcinx commented May 25, 2020

keyonjie commented May 26, 2020

keyonjie commented May 26, 2020

ktrzcinx commented May 26, 2020

keyonjie commented May 26, 2020

keyonjie commented May 26, 2020

ktrzcinx commented May 26, 2020

keyonjie commented May 26, 2020

keyonjie commented May 29, 2020

keyonjie commented May 20, 2020 •

edited

Loading

lgirdwood commented May 21, 2020 •

edited

Loading