Generic process implementation #2129

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

@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:16:53Z

@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".

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

s/flavour/favour in commit message?

oops, thanks.

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

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

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

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

shouldn't this be = type?

yep, good catch, thanks.

-Original file line number
+Diff line change
@@ Expand Up / @@ -39,6 +39,12 @@ enum sof_comp_type { @@
     	SOF_COMP_ASRC,		/**< Asynchronous sample rate converter */
     	SOF_COMP_DCBLOCK,
     	SOF_COMP_SMART_AMP,             /**< smart amplifier component */
+    	/*
+    	 * No more _COMP_ types to be added.
+    	 * Use SOF_COMP_PROCESS from now on.
+    	 */
+    	SOF_COMP_PROCESS = 1000,	/**< generic process component */
     	/* keep FILEREAD/FILEWRITE as the last ones */
     	SOF_COMP_FILEREAD = 10000,	/**< host test based file IO */
     	SOF_COMP_FILEWRITE = 10001,	/**< host test based file IO */
@@ Expand All / @@ -57,8 +63,7 @@ struct sof_ipc_comp { @@
     	uint32_t pipeline_id;
     	uint32_t core;
-    	/* reserved for future use */
-    	uint32_t reserved[1];
+    	uint32_t subtype;	/**< flavour for generic component type */
     } __packed;
     /*
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -26,7 +26,7 @@ @@
     /* SOF ABI version major, minor and patch numbers */
     #define SOF_ABI_MAJOR 3
-    #define SOF_ABI_MINOR 16
+    #define SOF_ABI_MINOR 17
     #define SOF_ABI_PATCH 0
     /* SOF ABI version number. Format within 32bit word is MMmmmppp */
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	process->comp.hdr.size = ipc_size;
     	process->comp.hdr.cmd = SOF_IPC_GLB_TPLG_MSG | SOF_IPC_TPLG_COMP_NEW;
     	process->comp.id = swidget->comp_id;
-    	process->comp.type = type;
+    	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;
+    	} else {
+    		process->comp.type = SOF_COMP_PROCESS;
+    	}
     	process->comp.pipeline_id = index;
     	process->config.hdr.size = sizeof(process->config);
@@ Expand Down @@

Generic process implementation #2129

Uh oh!

Generic process implementation #2129

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

plbossart May 21, 2020 •

edited

Loading

keyonjie May 25, 2020 •

edited

Loading