Skip to content

Cherry-pick RTNR commits from main branch #4905

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
Closed
MingJenTai wants to merge 5 commits into from
Closed

Cherry-pick RTNR commits from main branch #4905

MingJenTai wants to merge 5 commits into from

Conversation

@MingJenTai
Copy link
Contributor

@MingJenTai MingJenTai commented Oct 21, 2021

No description provided.

@lgirdwood
Copy link
Member

lgirdwood commented Oct 21, 2021

@mwasko for adl-003

@cujomalainey
Copy link
Contributor

cujomalainey commented Oct 24, 2021

@MingJenTai please "backport" the patches as in their current form they do not compile according to the CI

@cujomalainey cujomalainey self-requested a review October 24, 2021 17:36
cujomalainey
cujomalainey suggested changes Oct 24, 2021
View reviewed changes
Copy link
Contributor

@cujomalainey cujomalainey left a comment

Choose a reason for hiding this comment

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

patches do not compile

@MingJenTai
Copy link
Contributor Author

MingJenTai commented Oct 26, 2021 •
edited
Loading

@MingJenTai please "backport" the patches as in their current form they do not compile according to the CI

Hi @cujomalainey ,

Are you referring to the CI build https://sof-ci.01.org/sof-pr-viewer/#/build/PR4905/build7705879 ?

I found that the build failure on platforms other than TGL was caused by the undefined macro used in src/platform/intel/cavs/lib/memory.c
With the commit d407332#diff-ba34032c99aa8d14b8f99935dd82c83b9ec0549d27cb13662fb4e8889ace018f , the macro definitions in PLATFORM/include/platform/lib/memory.h should also be updated. Is this required in this branch? Thanks!

@cujomalainey cujomalainey requested a review from mwasko October 26, 2021 20:21
@cujomalainey
Copy link
Contributor

cujomalainey commented Oct 26, 2021

@mwasko should make the call

@mwasko
Copy link
Contributor

mwasko commented Oct 28, 2021

@MingJenTai I would like to get clear CI to merge this changes. Please also provide validation details with cherry-picked changes. I would like to get confirmation that branch with new changes was verified and we do not rely only on CI.

@MingJenTai
Copy link
Contributor Author

MingJenTai commented Oct 28, 2021

@MingJenTai I would like to get clear CI to merge this changes. Please also provide validation details with cherry-picked changes. I would like to get confirmation that branch with new changes was verified and we do not rely only on CI.

@mwasko Do you mean that I need to create another PR to clear all the CI build failures?
Besides, I would like to know more about the validation details you mentioned. RTNR requires proprietary libraries to build. Should I provide additional files or logs to you? Thanks!

@MingJenTai
Copy link
Contributor Author

MingJenTai commented Oct 29, 2021

@MingJenTai I would like to get clear CI to merge this changes. Please also provide validation details with cherry-picked changes. I would like to get confirmation that branch with new changes was verified and we do not rely only on CI.

@mwasko Do you mean that I need to create another PR to clear all the CI build failures? Besides, I would like to know more about the validation details you mentioned. RTNR requires proprietary libraries to build. Should I provide additional files or logs to you? Thanks!

Hi @bzhg, I am merging the commits from upstream to support RTNR to branch adl-003-drop-stable, and might need to verify these cherry-picked changes. Would you be able to help with that, or provide any suggestions? Thank you!

@mwasko
Copy link
Contributor

mwasko commented Oct 29, 2021

@mwasko Do you mean that I need to create another PR to clear all the CI build failures?
Besides, I would like to know more about the validation details you mentioned. RTNR requires proprietary libraries to build. Should I provide additional files or logs to you? Thanks!

I would recommend to add commits to this PR that will clear CI build failures.

However the CI is just a basic test scope to catch some obvious regression. Since this is a release stable branch then the full/stress test scope should be executed. I don't need logs or files just information who did the stress test validation and confirm the branch stability.

@cujomalainey I assume this backported changes are required for Google then can (or did) you run your full scope validation?

@cujomalainey
Copy link
Contributor

cujomalainey commented Nov 1, 2021

@mwasko you are correct, in theory validation should be isolated to the RTK component and can be left to RTK

@MingJenTai please cherry-pick commits from main and don't add custom commits, also please remove your merge commits, we do not merge in this tree. It is better to amend your original commits so they work on the current branch (and state after a cherry pick line) rather than attempting to patch code that is not relevant to the branch or the feature

@MingJenTai MingJenTai force-pushed the adl-003-drop-stable branch 3 times, most recently from 7382194 to 7e255fb Compare November 2, 2021 11:22
@MingJenTai
Support RTNR on Tigerlake platform
d1c9a12 
This PR adds RTNR Noise Reduction/Suppression(NR/NS) component by
Realtek Semiconductor Corp. This feature links to proprietary libraries.
Please contact antz0525@realtek.com for any question about the library.

Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>

Support RTNR on Tigerlake platform

This PR adds RTNR Noise Reduction/Suppression(NR/NS) component by
Realtek Semiconductor Corp. This feature links to proprietary libraries.
Please contact antz0525@realtek.com for any question about the library.

Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>

Cherry-picked from bbc5209, with the following changes:
Move files added under tools/topology/topology1/ to tools/topology/
Migrate changes in tools/topology/topology1/CMakeLists.txt to tools/topology/CMakeLists.txt
Modify rtnr_new() in rtnr.c in accordance with API changes.
@MingJenTai
Reset sink_format at rtnr_reset()
33a6c31 
Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>

Cherry-picked ffrom a226f0b
@MingJenTai
Integrate RTNR into sof-tgl-max98357a-rt5682.m4
8464e04 
Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>

Cherry-picked from d3381ea, with following changes:
Migrate changes in tools/topology/topology1/CMakeLists.txt  to tools/topology/CMakeLists.txt
Migrate changes in tools/topology/topology1/sof-tgl-max98357a-rt5682.m4 to tools/topology/sof-tgl-max98357a-rt5682.m4
@MingJenTai
Integrate RTNR into sof-adl-max98357a-rt5682
9f76ccd 
Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>

Cherry-picked from e4e23d4, with the following changes:
Migrate changes in tools/topology/topology1/CMakeLists.txt to  tools/topology/CMakeLists.txt
@MingJenTai
Support both 48kHz and 16kHz recording
4e86843 
Add 48kHz recording support to RTNR
Signed-off-by: Ming Jen Tai <mingjen_tai@realtek.com>

Cherry-picked from bf1e3b7, with the following changes:
Migrate modifications under tools/topology/topology1/ to an upper level.
cujomalainey
cujomalainey approved these changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@cujomalainey cujomalainey left a comment

Choose a reason for hiding this comment

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

@mwasko do have any idea why everything is failing?

@MingJenTai
Copy link
Contributor Author

MingJenTai commented Nov 3, 2021

@mwasko you are correct, in theory validation should be isolated to the RTK component and can be left to RTK

@MingJenTai please cherry-pick commits from main and don't add custom commits, also please remove your merge commits, we do not merge in this tree. It is better to amend your original commits so they work on the current branch (and state after a cherry pick line) rather than attempting to patch code that is not relevant to the branch or the feature

@cujomalainey I've created a new PR( #4952 ) that picks the same commits, but with original commits and modification stated in commit message. Please use that one instead, thank you!

@mwasko There are build errors on current HEAD of adl-003-drop-stable. It should be the cause of CI build errors.

mwasko
mwasko previously approved these changes Nov 4, 2021
View reviewed changes
@mwasko mwasko dismissed their stale review November 4, 2021 09:12

Wrong PR, I will accept PR( #4952 )

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

Reviewers

@mwasko mwasko mwasko left review comments

@dbaluta dbaluta Awaiting requested review from dbaluta

@lbetlej lbetlej Awaiting requested review from lbetlej

@lgirdwood lgirdwood Awaiting requested review from lgirdwood

@mmaka1 mmaka1 Awaiting requested review from mmaka1

@plbossart plbossart Awaiting requested review from plbossart

@ranj063 ranj063 Awaiting requested review from ranj063

+1 more reviewer

@cujomalainey cujomalainey cujomalainey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MingJenTai @lgirdwood @cujomalainey @mwasko