Generic process implementation #2129

keyonjie · 2020-05-20T04:33:37Z

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 FW corresponding PR is: thesofproject/sof#2956

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:40:09Z

@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

Thanks @keyonjie . I think this is a good step. If the full UUID is accepted, we could start to use it immediately and change this patch to pass the UUID. If not (we need ext-manifest to be deployed, or we go with uuids but it just takes a lot of time), then this is a good intermediate solution.

But, but, we need to address backwards compatibility on kernel side. We can't use the PROCESS type with old firmware versions.

kv2019i · 2020-05-20T11:30:23Z

sound/soc/sof/topology.c

 	process->comp.id = swidget->comp_id;
-	process->comp.type = type;
+	process->comp.type = SOF_COMP_PROCESS;
+	process->comp.subtype = type;


@keyonjie Should this be conditional to FW version? If kernel is used with older firmware, this would break this case, right?

Good point, let me add a check here.

dbaluta · 2020-05-20T13:27:56Z

include/sound/sof/topology.h

 	struct sof_ipc_cmd_hdr hdr;
 	uint32_t id;
 	enum sof_comp_type type;
 	uint32_t pipeline_id;


s/flavour/favour in commit message?

oops, thanks.

include/sound/sof/topology.h

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>

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>

Change to pass the SOF_COMP_PROCESS component type + subtype to the firmware for the process component creation, if the firmware can accept that (ABI >= 3.17.0). 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>

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>

plbossart

This is missing a lot of information on how the subtype is defined and how we will use UUID. It does not look mature enough to be merged if I am honest.

plbossart · 2020-05-21T21:48:02Z

sound/soc/sof/topology.c

+	if (sdev->fw_ready.version.abi_version >= SOF_ABI_VER(3, 17, 0)) {
+		/* use generic process component if supported */
+		process->comp.type = SOF_COMP_PROCESS;
+		process->comp.subtype = type;


how is this subtype defined? and how does this help make the difference between e.g. an encoder and an EQ? You are missing one layer of description IMHO.

also how does this relate to PR #2090? Please don't provide disjoint PRs for the same topic, it's not nice for reviewers.

how is this subtype defined? and how does this help make the difference between e.g. an encoder and an EQ? You are missing one layer of description IMHO.

At the moment I just use the component type as the subtype for the process components, as the EQ already has its component type.

Thanks for your feedback Pierre. We are actually not quite aligned about what direction should we go (see the discussion here thesofproject/sof#2956 (comment)).

You are right that once we have decided to go with subtype, I need to add definition for the subtype (e.g. make the subtype another enum, move all existed process component types' definition into this enum, and this new defined enum should be kept in the header file shared by FW and topology only, the linux driver won't know about this).

also how does this relate to PR #2090? Please don't provide disjoint PRs for the same topic, it's not nice for reviewers.

Sorry for the confusion, yes, these 2 PRs are disjoint, they are 2 different solutions as described here: thesofproject/sof#2956 (comment)

Man, you are really not making it easy for reviewers to look into your work...Please try and put yourself in the shoes of someone not familiar with your ideas and without context on another email thread.

As shown with PR #2138 I had to do the integration work myself to figure out what to do with your patches and what you meant in commit messages. Try and explain more what you are trying to fix, it'll help have your patches merged quicker and with fewer review cycles.

Thanks for your suggestion here, I don't know how to say, I would forgive myself here, after joined the related discussion meetings, created 2 discussion topics, verified and provided 2 solutions for it, maybe the commit messages are still not good, but had tried my best.

I'll also chime in that this has been exceptionally confusing case, but for me @keyonjie 's PR have actually helper to clarify the problem. It seems the SOF discussions pages (like https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32 ) work fairly well to brainstorm ideas, but we don't really seem to get actual decisions done there. To force a decision, submitting a PR works better (like @keyonjie you did here), but when you have issues that affect both driver and FW arch, this approach does not seem to work very well as the discussion threads get distributed across multiple PRs.

Not sure how to improve this. One option is drive to conclusion with a patch to sof-docs that documents the proposed design.

Thank you @kv2019i for the constructive suggestion, let's try to finalize the solution in the next tech sync meeting, and then I will close PRs for one of the two solutions.

having an sof-docs PR is really the only way to close on this.

lyakh · 2020-05-25T09:14:42Z

sound/soc/sof/topology.c

+		process->comp.type = SOF_COMP_PROCESS;
+		process->comp.subtype = type;
+	} else {
+		process->comp.type = SOF_COMP_PROCESS;


shouldn't this be = type?

yep, good catch, thanks.

kv2019i

The FW guidance (thesofproject/sof#2956) seems to be to use full 128bit UUID in IPC interface, so kernel side should target this (with no intermediary steps). Some additional comments to the commit messages.

kv2019i · 2020-05-25T17:16:53Z

include/sound/sof/topology.h

+
+	/*
+	 * No more _COMP_ types to be added.
+	 * Use SOF_COMP_PROCESS from now on.


@keyonjie @lgirdwood Shouldn't this be "No more COMP types should be added for effects" (snd_soc_dapm_effect/sof_ipc_comp_process)?

If want to have main type that defines the ALSa controls and IPC msg, and flavor which can vary, we can't put everything under same component id right? The above comment would mean our current list of main types is exhaustive and it will never be extended. This doesn't make sense. If we add a new component class, it should be identified by a unique component type.

Yes, let me change it to "No more COMP types should be added for effects/process".

kv2019i · 2020-05-25T17:23:17Z

include/sound/sof/topology.h

 	SOF_COMP_KPB,			/* A key phrase buffer component */
 	SOF_COMP_SELECTOR,		/**< channel selector component */
 	SOF_COMP_DEMUX,
 	SOF_COMP_ASRC,		/**< Asynchronous sample rate converter */


@keyonjie The discussions page is not public for all and we didn't really conclude anything there, so I would not refer to it from the commit message, but rather summarize the design here. I'd also not refer to the FW PR in the commit message (we have not done that traditionally in the kernel commit messages -> rather have these in the kernel PR description). I'd also drop the paragraph on "After this series is applied.."

Good points, thank you.

kv2019i · 2020-05-25T17:26:12Z

include/sound/sof/topology.h

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


The FW discussion ( thesofproject/sof#2956 ) seems to be moving to use UUIDs directly, so let's target that.

keyonjie · 2020-05-26T10:35:34Z

The aligned solution is stated here https://github.com/orgs/thesofproject/teams/sof-developers/discussions/36/comments/22. According to it, we will need to switch to use UUID for all component types in one shot, with a major ABI bump for it. So no need for this generic process component anymore if that is implemented, let me close PRs and work on the new solution.

keyonjie · 2020-05-29T08:31:56Z

@kv2019i @plbossart Let me close this PR and use the only one PR for this UUID implementation on kernel side: #2090 to avoid confusion.

keyonjie requested review from bardliao, dbaluta, juimonen, lyakh, ranj063 and singalsu as code owners May 20, 2020 04:33

keyonjie mentioned this pull request May 20, 2020

Generic process implementation thesofproject/sof#2956

Closed

kv2019i requested changes May 20, 2020

View reviewed changes

dbaluta reviewed May 20, 2020

View reviewed changes

include/sound/sof/topology.h Show resolved Hide resolved

dbaluta reviewed May 20, 2020

View reviewed changes

include/sound/sof/topology.h Show resolved Hide resolved

keyonjie force-pushed the generic_process branch from c528952 to 2f5d64e Compare May 21, 2020 08:59

keyonjie added 4 commits May 21, 2020 17:01

topology: add subtype to sof_ipc_comp as flavour of the component type

21febe6

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>

topology: change to use generic process components

ac1c78b

Change to pass the SOF_COMP_PROCESS component type + subtype to the firmware for the process component creation, if the firmware can accept that (ABI >= 3.17.0). Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

ASoC: SOF: bump the ABI minor version

2f5d64e

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>

plbossart requested changes May 21, 2020

View reviewed changes

lyakh requested changes May 25, 2020

View reviewed changes

kv2019i requested changes May 25, 2020

View reviewed changes

keyonjie closed this May 29, 2020

Generic process implementation #2129

Generic process implementation #2129

Uh oh!

Conversation

keyonjie commented May 20, 2020

Uh oh!

keyonjie commented May 20, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart May 21, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie May 25, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented May 26, 2020

Uh oh!

keyonjie commented May 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

plbossart May 21, 2020 •

edited

Loading

keyonjie May 25, 2020 •

edited

Loading