Skip to content

Generic process implementation#2129

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.

Sign up for GitHub

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

Closed
keyonjie wants to merge 4 commits into from
Closed

Generic process implementation #2129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5884830
topology: add a generic process component type
keyonjie Apr 30, 2020
21febe6
topology: add subtype to sof_ipc_comp as flavour of the component type
keyonjie May 9, 2020
ac1c78b
topology: change to use generic process components
keyonjie May 9, 2020
2f5d64e
ASoC: SOF: bump the ABI minor version
keyonjie May 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions include/sound/sof/topology.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -39,6 +39,12 @@ enum sof_comp_type {
SOF_COMP_ASRC, /**< Asynchronous sample rate converter */

@kv2019ikv2019iMay 25, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@keyonjiekeyonjieMay 26, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good points, thank you.

SOF_COMP_DCBLOCK,
Comment thread
keyonjie marked this conversation as resolved.
SOF_COMP_SMART_AMP, /**< smart amplifier component */

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

@kv2019ikv2019iMay 25, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@keyonjiekeyonjieMay 26, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

*/
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 */
Comment thread
keyonjie marked this conversation as resolved.
Expand All@@ -57,8 +63,7 @@ struct sof_ipc_comp {
uint32_t pipeline_id;

@dbalutadbalutaMay 20, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

s/flavour/favour in commit message?

@keyonjiekeyonjieMay 20, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oops, thanks.

uint32_t core;

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

@kv2019ikv2019iMay 25, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@keyonjiekeyonjieMay 26, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay.


/*
Expand Down
2 changes: 1 addition & 1 deletion include/uapi/sound/sof/abi.h
View file
Open in desktop
Original file line numberDiff line numberDiff 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
8 changes: 7 additions & 1 deletion sound/soc/sof/topology.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2183,7 +2183,13 @@ static int sof_process_load(struct snd_soc_component *scomp, int index,
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;

@plbossartplbossartMay 21, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@plbossartplbossartMay 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@keyonjiekeyonjieMay 22, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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).

@keyonjiekeyonjieMay 22, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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)

@plbossartplbossartMay 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@keyonjiekeyonjieMay 25, 2020•
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@kv2019ikv2019iMay 25, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@keyonjiekeyonjieMay 26, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@plbossartplbossartMay 26, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

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

@lyakhlyakhMay 25, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be = type?

@keyonjiekeyonjieMay 25, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yep, good catch, thanks.

}
process->comp.pipeline_id = index;
process->config.hdr.size = sizeof(process->config);

Expand Down