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

[mt8186-v02] waves: store config blob in a cache in waves.c #8613

Merged
merged 1 commit into from
Jan 16, 2024
Merged

[mt8186-v02] waves: store config blob in a cache in waves.c #8613

merged 1 commit into from
Jan 16, 2024

Conversation

barry-waves
Copy link
Contributor

Store/apply config blob in a cache to avoid that
cfg.data will be released after prepare.

btian1
btian1 reviewed Dec 13, 2023
View reviewed changes
src/audio/module_adapter/module/waves.c Show resolved Hide resolved
src/audio/module_adapter/module/waves.c Outdated Show resolved Hide resolved
src/audio/module_adapter/module/waves.c Outdated
@@ -596,95 +652,77 @@ static int waves_codec_init(struct processing_module *mod)
int ret = 0;
void *response = NULL;

comp_dbg(dev, "waves_codec_init() start");
comp_info(dev, "waves_codec_init() start");
Copy link
Contributor

Choose a reason for hiding this comment

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

no need so much print, this is dbg only message, change to comp_dbg.

Copy link
Contributor

Choose a reason for hiding this comment

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

@btian1 i would agree, but we should also avoid modifying cherry picks any more than necessary

src/audio/module_adapter/module/waves.c Outdated Show resolved Hide resolved
src/audio/module_adapter/module/waves.c Outdated Show resolved Hide resolved
lgirdwood
lgirdwood reviewed Dec 13, 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.

btw, feel free to follow up with another PR if you like that changes the API naming to effect instead of codec.

src/audio/module_adapter/module/waves.c Outdated Show resolved Hide resolved
@barry-waves
waves: store config blob in a cache in waves.c
5b3d817 
Store/apply config blob in a cache to avoid that
cfg.data will be released after prepare.

Signed-off-by: barry.jan <[email protected]>
@barry-waves
Copy link
Contributor Author

Hi @lgirdwood , @btian1 and @Zames-Chang ,
Could you please review it? Thank you.

@kv2019i kv2019i changed the title waves: store config blob in a cache in waves.c [mt8186-v02] waves: store config blob in a cache in waves.c Dec 18, 2023
@lgirdwood
Copy link
Member

Hi @lgirdwood , @btian1 and @Zames-Chang , Could you please review it? Thank you.

@barry-waves this is MTK production branch so need @thesofproject/mediatek to review and merge.

@andyross @cujomalainey fyi.

@cujomalainey cujomalainey requested a review from a team December 19, 2023 21:44
btian1
btian1 approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@btian1 btian1 left a comment

Choose a reason for hiding this comment

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

if you'd like, you can follow up with another PR, move waves out of module_adapter folder.
and move it to under codec folder.

src/audio/module_adapter/module/waves.c Show resolved Hide resolved
@barry-waves
Copy link
Contributor Author

Hi @Zames-Chang and @thesofproject/mediatek ,
Could you please merge this patch? Thank you.

@lgirdwood
Copy link
Member

@andyross @cujomalainey fyi - somebody from MTK needs to merge.

@cujomalainey
Copy link
Contributor

@andyross @cujomalainey fyi - somebody from MTK needs to merge.

Why does MTK need to merge? The code change is entirely within the Waves block.

@kv2019i
Copy link
Collaborator

kv2019i commented Jan 5, 2024

@cujomalainey wrote:

Why does MTK need to merge? The code change is entirely within the Waves block.

This pull request is against a MTK release branch, not main -> "mt8186/v0.2"

@barry-waves
Copy link
Contributor Author

Hi,
This pull request cherry-picks from main to mt8186/v0.2

@johnylin76
Copy link
Contributor

This commit needs to be landed on mt8186/v0.2 as a bug fix for the existing Waves-integrated CrOS projects (later than TigerLake precisely, i.e. MT8186, ADL-P/N, RPL.

@barry-waves
Copy link
Contributor Author

Hi @Zames-Chang and @thesofproject/mediatek ,
Could you please merge this patch? Thank you.

@barry-waves
Copy link
Contributor Author

Hi @Zames-Chang and @thesofproject/mediatek ,
Could you please merge this pull request? Thank you.

@Zames-Chang Zames-Chang merged commit 475be1e into thesofproject:mt8186/v0.2 Jan 16, 2024
7 of 11 checks passed
@barry-waves
Copy link
Contributor Author

Hi @Zames-Chang,
Thank you.

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

@lgirdwood lgirdwood lgirdwood left review comments

@btian1 btian1 btian1 approved these changes

@chunxu-li chunxu-li chunxu-li 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

@Zames-Chang Zames-Chang Awaiting requested review from Zames-Chang

@cujomalainey cujomalainey Awaiting requested review from cujomalainey

@johnylin76 johnylin76 Awaiting requested review from johnylin76

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

Successfully merging this pull request may close these issues.
8 participants
@barry-waves @lgirdwood @cujomalainey @kv2019i @johnylin76 @chunxu-li @btian1 @Zames-Chang