Skip to content
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

Jump to bottom

Fixes for smart amp test module #7101

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fixes for smart amp test module #7101

wants to merge 2 commits into from

Conversation

ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Feb 15, 2023

No description provided.

@ranj063 ranj063 force-pushed the fix/smart_amp_source branch from 072f025 to 48be8c4 Compare February 15, 2023 17:34
@fredoh9
Copy link
Contributor

fredoh9 commented Feb 15, 2023

For alsabat test failure, captured wave file shows empty/silence. But I can generate sinewave and find frequency response right. Trying to find why we have empty wave file.

@fredoh9
Copy link
Contributor

fredoh9 commented Feb 15, 2023

Too early updated above.
Left channel is fine, 821Hz detected but only right channel is empty/silence.

Channel 1 - Checking for target frequency 821.00 Hz
Amplitude: 31127.0; Percentage: [94]
Detected peak at 821.04 Hz of 41.83 dB
Total 42.7 dB from 818.85 to 823.24 Hz
PASS: Peak detected at target frequency
Detected at least 1 signal(s) in total

Channel 2 - Checking for target frequency 821.00 Hz
Amplitude: 0.0; Percentage: [0]
Detected at least 0 signal(s) in total

@ranj063 ranj063 force-pushed the fix/smart_amp_source branch 4 times, most recently from ba26f8b to 37e01d2 Compare February 15, 2023 20:33
@ranj063
Copy link
Collaborator Author

ranj063 commented Feb 15, 2023

Left channel is fine, 821Hz detected but only right channel is empty/silence.

Thanks @fredoh9. fixed now.

@aiChaoSONG
Copy link
Collaborator

aiChaoSONG commented Feb 16, 2023 •
edited
Loading

@fredoh9 Maybe there is an issue or configuration issue, I remembered that when I was in lab, I only hear front left only.

@ranj063 ranj063 force-pushed the fix/smart_amp_source branch from 37e01d2 to d85baad Compare February 16, 2023 02:32
@ranj063
smart_amp_test: remove bind/unbind ops
6588ade 
Set the source_buf during prepare if the buffer source queue ID does not
match that of the feedback buffer. Set the feedback_buf during copy
to also handle the case where the playback stream is started before the
feedback stream.

Signed-off-by: Ranjani Sridharan <[email protected]>
@ranj063
smart_amp_test: clean up the copy() function
d6733b7 
Clean up the variable names in the copy() function and make it easier to
follow.

Signed-off-by: Ranjani Sridharan <[email protected]>
@ranj063 ranj063 force-pushed the fix/smart_amp_source branch from d85baad to d6733b7 Compare February 16, 2023 14:37
lyakh
lyakh reviewed Feb 20, 2023
View reviewed changes
src/samples/audio/smart_amp_test.c
@@ -692,6 +669,9 @@ static int smart_amp_copy(struct comp_dev *dev)

buffer_release(buf);
}
#if CONFIG_IPC_MAJOR_4
sad->feedback_buf = NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

so on every copy you have to re-assign the feedback buffer... TBH I'm not sure what smart-amp-test is actually for (it's "just" a test, right? So it isn't used in any real audio streaming?) but this seems rather expensive

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this will need a comment to explain the specific case if needed other wise it could be copy and pasted into other samples.

@gkbldcig
Copy link
Collaborator

Can one of the admins verify this patch?

lgirdwood
lgirdwood approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Lots of code removal !

@kv2019i
Copy link
Collaborator

kv2019i commented Mar 30, 2023

Good now @RanderWang ?

kv2019i
kv2019i approved these changes Mar 30, 2023
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Code seems good, but I don't fully understand the conclusion on the long discussion in this PR on the multi-thread case. It is marked as resolved, so I guess this PR is now good to go, but would be good for @RanderWang @lyakh to ack with a +1 that this is ok to proceed.

@RanderWang
Copy link
Collaborator

RanderWang commented Mar 31, 2023 •
edited
Loading

@lgirdwood @kv2019i. I also discussed my method (#7259) to remove bind & unbind with @ranj063 @lyakh and got a conclusion that we will do bind & unbind job in module adapter. @andrula-song will adapt smart_amp to module interface and bind & unbind will be removed in her PR. We don't need to check feedback buffer in smart_amp_copy

@andrula-song
Copy link
Contributor

hi @RanderWang
since the there is a limit of avail_frames = MIN(avail_passthrough_frames, avail_feedback_frames); and the avail_frames and avail_passthrough_frames could be different(#7100), I am afraid we have to check feedback buffer in smart_amp_copy function.

@RanderWang
Copy link
Collaborator

hi @RanderWang since the there is a limit of avail_frames = MIN(avail_passthrough_frames, avail_feedback_frames); and the avail_frames and avail_passthrough_frames could be different(#7100), I am afraid we have to check feedback buffer in smart_amp_copy function.

you can discuss with Ranjani

@lgirdwood
Copy link
Member

@andrula-song good for you now, if so pls approve or reject.

@andrula-song
Copy link
Contributor

andrula-song commented Apr 6, 2023 •
edited by lgirdwood
Loading

@andrula-song good for you now, if so pls approve or reject

apart of this PR already exist in our code, and I am using module adapter to remove bind & unbind. So I don't think merge this PR is necessary.

@lgirdwood
Copy link
Member

@ranj063 still needed ?

@kv2019i kv2019i mentioned this pull request Apr 17, 2023
Merged
@kv2019i
Copy link
Collaborator

kv2019i commented Apr 28, 2023

Turning into draft for now...

@kv2019i kv2019i marked this pull request as draft April 28, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh left review comments

@RanderWang RanderWang RanderWang left review comments

@lgirdwood lgirdwood lgirdwood approved these changes

@kv2019i kv2019i kv2019i approved these changes

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ranj063 @fredoh9 @aiChaoSONG @gkbldcig @kv2019i @RanderWang @andrula-song @lgirdwood @lyakh