Ipc4 : add google rtc AEC module support #125
Conversation
|
FW PR: thesofproject/sof#6927 |
this is about the Google RTC AEC, what;s the link with the Windows reference you made above @RanderWang ? |
Thanks, I got it. I may misunderstand that google AEC is sof AEC. I will add entry for google AEC |
RanderWang
force-pushed
the
ipc4-eac
branch
2 times, most recently
from
0ed6676
to
7bdf996
Compare
RanderWang
changed the title
|
@johnylin76 @cujomalainey pls review |
RanderWang
force-pushed
the
ipc4-eac
branch
2 times, most recently
from
0406efa
to
049b209
Compare
|
rebase to latest code |
|
@plbossart update good for you ? |
|
@RanderWang conflict. |
Thanks, updated |
config/mtl.toml
Outdated
| [[module.entry]] | ||
| name = "RTC_AEC" | ||
| uuid = "B780A0A6-269F-466F-B477-23DFA05AF758" | ||
| affinity_mask = "0x3" |
There was a problem hiding this comment.
curious why this is not 0xF?
There was a problem hiding this comment.
It is a duplication of general AEC. Let's use 0xF for RTC AEC
config/mtl.toml
Outdated
| name = "RTC_AEC" | ||
| uuid = "B780A0A6-269F-466F-B477-23DFA05AF758" | ||
| affinity_mask = "0x3" | ||
| instance_count = "1" |
There was a problem hiding this comment.
why 1? What if we want to have one on HS and DMIC?
There was a problem hiding this comment.
@mwasko could you clarify here, if the ext manifest says only 1 module instance but topology says 2 what should happen ?
There was a problem hiding this comment.
@mwasko could you clarify here, if the ext manifest says only 1 module instance but topology says 2 what should happen ?
In such case FW should report an error on attempt to create second module instance.
There was a problem hiding this comment.
ok, wont this mean generating differnt baseFWs for different topologies ?
There was a problem hiding this comment.
@mwasko @lgirdwood can I ask a somewhat unrelated question related to the affinity_mask? Is there a good reason why some modules are restricted to core0, some to core0/1, and most do not have any restrictions.
I am specifically concerned about the core0 restriction, which I view as incompatible with the direction to allow low-latency pipelines to run on all cores. if a mixin is always on core0, then something's not adding up.
There was a problem hiding this comment.
@plbossart agreed, this seems to put component level restrictions on topology configuration without context. I would expect more restrictions like "MIPS estimates cannot violate this amount per core in worst case scenarios" and the stack check for this platform.
There was a problem hiding this comment.
@mwasko @lgirdwood can I ask a somewhat unrelated question related to the affinity_mask? Is there a good reason why some modules are restricted to core0, some to core0/1, and most do not have any restrictions.
This field is used by Windows driver that has more flexible approach on how pipelines and modules are instantiated. The pipeline/component is assigned to specific core based on current CPU resource availability (MCPS).
I am specifically concerned about the core0 restriction, which I view as incompatible with the direction to allow low-latency pipelines to run on all cores. if a mixin is always on core0, then something's not adding up.
Originally Windows was grouping LL modules on Core 0 and I believe that is why Rander defined it like that. However, @plbossart is right here and with SOF we allow low-latency pipelines to run on all cores so all the built-in modules should not have affinity restrictions. The 3rd party modules affinity is up to integrator but the default option should be 'no restrictions'.
There was a problem hiding this comment.
Thanks @mwasko, your explanations match the little understanding I have of the modules.
@RanderWang can we have a follow-up patch to clean-up the affinity definitions for built-in modules, i.e. remove the restriction to core0 and core0-1 for all built-in modules? Thanks!
There was a problem hiding this comment.
sure, I will follow up it.
Signed-off-by: Rander Wang <[email protected]>
Signed-off-by: Rander Wang <[email protected]>
|
@cujomalainey sorry, update late. |
No description provided.