Skip to content

Conversation

@bkokoszx
Copy link
Collaborator

@bkokoszx bkokoszx commented Apr 29, 2020

Smart amplifier should be used in following topology:

                     +------------------------------------------+
         +------+    | +---+  +-------------+  +---+  +-------+ |
         | Host |----->|Buf|->|  Smart Amp  |->|Buf|->|SSP Dai|-----------+
         +------+    | +---+  +-------------+  +---+  +-------+ |         |
                     +-----------------^------------------------+         |
                                       |                                  |
                                     +---+                                |
                                     |Buf|                                |
                                     +---+                                |
                                       ^                                  |
                     +-----------------|--------------------------+       |
         +------+    | +---+  +--------|------+  +---+  +-------+ |       |
         | Host |<-----|Buf|<-|     Demux     |<-|Buf|<-|SSP Dai|<--------+
         +------+    | +---+  +---------------+  +---+  +-------+ |
                     +--------------------------------------------+

Smart amplifier test component does not implement any specific processing
algorithm at the moment. I can pass chosen channels from source (buffer
between Host and Smart amplifier) or feedback (buffer between Smart
amplifier and Demux component) buffers. Those channels are selected by
sending proper config to this component. Smart amplifier can be
configured via SOF_SMART_AMP_CONFIG or SOF_SMART_AMP_MODEL data
(similarly as it happens in detector case).

This PR also rewrites functions responsible for large config configuration
from detect_test component and puts them in component.c file as
generic ones - as a result they can be used by other components.

@bkokoszx bkokoszx requested a review from keyonjie April 29, 2020 12:15
@bkokoszx bkokoszx force-pushed the topic/smart-amp branch 5 times, most recently from 22c66a3 to 544cb25 Compare April 30, 2020 14:17
@bkokoszx bkokoszx force-pushed the topic/smart-amp branch 13 times, most recently from 992fac8 to 72ebc07 Compare May 5, 2020 08:39
@bkokoszx
Copy link
Collaborator Author

bkokoszx commented May 26, 2020

@lgirdwood
If this PR looks correct, it should be merge with #2788 (or I can reopen #2789 - #2788 and #2789 are not mutually exclusive)

Copy link

@mmaka1 mmaka1 left a comment

Choose a reason for hiding this comment

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

There could be some improvement applied in future version with a single loop in copy() and illustration of priority of the main input over the feedback source.

@lgirdwood
Copy link
Member

DO NOT MERGE until #2788 is merged.
@bkokoszx can you check CI build.

@mmaka1
Copy link

mmaka1 commented May 27, 2020

@singalsu Pls take a look at the *model*() helpers proposed. The PR re-uses the code in two components, kd and smart-amp-test. But there are others (eq*) that potentially could use (call into) them to reduce the footprint. Are they ready for re-use or need some adjustments?

@singalsu
Copy link
Collaborator

@mmaka1 The name "model" does not sound very generic, it's related to keyword and speech recognition paradigm but it's minor and could renamed. There's a bit more in similar EQ and beamformer functions to allow updates during streaming (I'm not sure this isn't capable - there's some code to prevent re-configuration with incomplete data and flag re-configuration when ready). All components have their own parsing but they could be harmonized and these for keyword look close. Reuse would also improve testing coverage. Like that recent unnoticed bug in FIR with memcpy_s() destination sized mismatch.

@lgirdwood lgirdwood changed the title smart_amp: add smart amplifier test component [DO NOT MERGE] smart_amp: add smart amplifier test component May 29, 2020
@lgirdwood
Copy link
Member

I've updated the title so it obvious we dont merge until all dependencies are merged.

@keyonjie
Copy link
Contributor

keyonjie commented Jun 5, 2020

Hi @lgirdwood this doesn't have dependency to #2788, it is basic driver for smart_amp component.

@keyonjie
Copy link
Contributor

keyonjie commented Jun 5, 2020

@bkokoszx can you fix the checkpatch issues below, and change to use the RT UUID macros just updated from @ktrzcinx for smart_amp:

CHECK: Unnecessary parentheses around 'bs > 0'
#142: FILE: src/audio/smart_amp_test.c:69:
+	if ((bs > 0) && (bs < sizeof(struct sof_smart_amp_config))) {

CHECK: Unnecessary parentheses around 'bs < sizeof(struct sof_smart_amp_config)'
#142: FILE: src/audio/smart_amp_test.c:69:
+	if ((bs > 0) && (bs < sizeof(struct sof_smart_amp_config))) {

WARNING: 'assumming' may be misspelled - perhaps 'assuming'?
#690: FILE: src/include/sof/audio/smart_amp_test.h:38:
+ * E.g. assumming that a smart amplifier input stream is configured

total: 0 errors, 1 warnings, 2 checks, 686 lines checked

bkokoszx added 3 commits June 8, 2020 10:15
This commit rewrites functions responsible for large config configuration
from detect_test component and puts them in component.c file as
generic ones - as a result they can be used by other components.

Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>
This commit replaces private detector component functions with
generic ones in order to remove code duplication.

Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>
Smart amplifier should be used in following topology:

                 +------------------------------------------+
     +------+    | +---+  +-------------+  +---+  +-------+ |
     | Host |----->|Buf|->|  Smart Amp  |->|Buf|->|SSP Dai|-----------+
     +------+    | +---+  +-------------+  +---+  +-------+ |         |
                 +-----------------^------------------------+         |
                                   |                                  |
                                 +---+                                |
				 |Buf|                                |
                                 +---+                                |
                                   ^                                  |
                 +-----------------|--------------------------+       |
     +------+    | +---+  +--------|------+  +---+  +-------+ |       |
     | Host |<-----|Buf|<-|     Demux     |<-|Buf|<-|SSP Dai|<--------+
     +------+    | +---+  +---------------+  +---+  +-------+ |
                 +--------------------------------------------+

Smart amplifier test component does not implement any specific processing
algorithm at the moment. I can pass chosen channels from source (buffer
between Host and Smart amplifier) or feedback (buffer between Smart
amplifier and Demux component) buffers. Those channels are selected by
sending proper config to this component. Smart amplifier can be
configured via SOF_SMART_AMP_CONFIG or SOF_SMART_AMP_MODEL data
(similarly as it happens in detector case).

Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>
@bkokoszx
Copy link
Collaborator Author

bkokoszx commented Jun 8, 2020

@keyonjie @lgirdwood
I've updated warning in checkpatch and and RT UUID's.
In current DSM scenario we need also use #2789 instead of #2788. It's because DSM components sets feedback buffer parameter and DEMUX should select proc function based on source buffer, because sink (feedback buffer in thath case) could not be set yet (e.g. we start capture and then playback). It could lead to format mismatch and wrong proc function selection.

@lgirdwood
Copy link
Member

@bkokoszx can you check the CI, looks like UT failure.

@bkokoszx
Copy link
Collaborator Author

@lgirdwood
I see that CI passes, maybe this PR was retriggered.

@lgirdwood
Copy link
Member

@bkokoszx any blockers for merge now ? Subject says DNM

@bkokoszx
Copy link
Collaborator Author

@lgirdwood
As I mentioned in:
#2897 (comment)
we can merge it with #2789 or wait for #2788. IMHO we can merge it #2789 - it is small fix, in which we just change buffer using in selecting processing function in DEMUX component (from sink to source, as DEMUX has only one source).

@keyonjie
Copy link
Contributor

@lgirdwood we need this PR since tplg and driver are already run with assuming it merged for quite a long time.
@bkokoszx can you remove the prefix '[DO NOT MERGE]' in the PR subject please?

@bkokoszx bkokoszx changed the title [DO NOT MERGE] smart_amp: add smart amplifier test component smart_amp: add smart amplifier test component Jun 16, 2020
@bkokoszx
Copy link
Collaborator Author

@lgirdwood
Ok, we can merge it.

@jajanusz
Copy link
Contributor

DO NOT MERGE until #2788 is merged.
@bkokoszx can you check CI build.

Is it still valid? I think that #2789 is enough.
We would like to merge it, cos we need it for release.

@bkokoszx
Copy link
Collaborator Author

@jajanusz
#2789 is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants