Skip to content

Convert the EQ IIR component to use the module interface#5926

Merged
lgirdwood merged 6 commits intothesofproject:mainfrom
ranj063:fix/eq_iir_drv
Jul 18, 2022
Merged

Convert the EQ IIR component to use the module interface#5926
lgirdwood merged 6 commits intothesofproject:mainfrom
ranj063:fix/eq_iir_drv

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Jun 15, 2022

No description provided.

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 15, 2022

Is it possible to fix the regression #5911 introduced by #5766 first?

@ranj063 ranj063 force-pushed the fix/eq_iir_drv branch 2 times, most recently from d8e1386 to 5e2f6b2 Compare June 15, 2022 23:29
@ranj063
Copy link
Collaborator Author

ranj063 commented Jun 15, 2022

Is it possible to fix the regression #5911 introduced by #5766 first?

this PR is not related to that issue at all. But I have a potential fix for it

@ranj063 ranj063 force-pushed the fix/eq_iir_drv branch 2 times, most recently from df0e7f2 to 3890d33 Compare June 16, 2022 05:43
singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
src/audio/eq_iir/eq_iir.c Outdated
Copy link
Collaborator

@singalsu singalsu Jun 16, 2022

Choose a reason for hiding this comment

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

This HiFi3 version difference is not yet needed but I think it can be left this way since @andrula-song will implement a more efficient version of IIR in Q3.

singalsu
singalsu reviewed Jun 16, 2022
View reviewed changes
Copy link
Collaborator

@singalsu singalsu left a comment

Choose a reason for hiding this comment

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

Looks quite good to me. Consider some improvements those I proposed to clean up this and successive components.

@fredoh9
Copy link
Contributor

fredoh9 commented Jun 16, 2022

I quickly checked this PR on my ADL RVP which has a DMIC transducer board. Captured wavefile is fine also

@ranj063
Copy link
Collaborator Author

ranj063 commented Jun 16, 2022

Looks quite good to me. Consider some improvements those I proposed to clean up this and successive components.

@singalsu I've addressed your comments in this version now

andrula-song
andrula-song reviewed Jun 17, 2022
View reviewed changes
@marc-hb marc-hb removed their request for review June 17, 2022 18:52
@lyakh
Copy link
Collaborator

lyakh commented Jun 22, 2022

@ranj063 this PR now has conflicts after my cache / sparse PR with a patch for eq-iir has been merged, please update

singalsu
singalsu approved these changes Jun 22, 2022
View reviewed changes
Copy link
Collaborator

@singalsu singalsu left a comment

Choose a reason for hiding this comment

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

Looks good now, but need the rebase.

@ranj063 ranj063 force-pushed the fix/eq_iir_drv branch 2 times, most recently from 33de4dd to caf94ab Compare June 22, 2022 16:07
lyakh
lyakh previously requested changes Jun 23, 2022
View reviewed changes
@lgirdwood
Copy link
Member

lgirdwood commented Jun 29, 2022

@ranj063 fwiw, All the sparse cache component updates now merged.

@lgirdwood
Copy link
Member

lgirdwood commented Jul 7, 2022

@ranj063 ping

ranj063 added 3 commits July 7, 2022 20:32
@ranj063
module_adapter: No need to include data_blob.h
89c1a45 
It is not needed.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
data_blob: Introduce a new helper for setting the blob
dc263c9 
Introduce a new helper function to set the blob to be used with the
module adapter set_configuration op. When all modules are converted to
use the new op, the comp_data_blob_set_cmd() can be removed.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
tplg_parser: Add support for parsing process components by UUID
5ba302a 
Add support for loading the process components by UUID. This needs a
modification in the IPC3 helper to modify the ipc data pointer to
address followed by the UUID in the component extended data.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
Copy link
Collaborator Author

ranj063 commented Jul 8, 2022

@ranj063 ping

@lgirdwood updated now

lgirdwood
lgirdwood reviewed Jul 12, 2022
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.

Just a few minor questions.

ranj063 added 3 commits July 13, 2022 19:11
@ranj063
eq_iir: Use the new module interface
4116fab 
Use the module interface instead of the comp drv interface for the
EQ IIR component.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
audio_format: Add macro for converting bytes <-> samples
8173baa 
And macros for converting bytes to samples and samples to bytes and use
it in all components to avoid duplication.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
module_adapter: Add a new helper to update input/output stream buffers
c43983d 
Add a new helper to update the consumed bytes in the input stream
buffer and the produces bytes in the output stream buffer.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063 ranj063 requested a review from lgirdwood July 14, 2022 02:13
@keqiaozhang
Copy link
Collaborator

keqiaozhang commented Jul 14, 2022

SOFCI TEST

1 similar comment
@lgirdwood
Copy link
Member

lgirdwood commented Jul 15, 2022

SOFCI TEST

@lgirdwood
Copy link
Member

lgirdwood commented Jul 15, 2022

rerun CI, there has been a west issue.

lyakh
lyakh reviewed Jul 18, 2022
View reviewed changes
src/audio/eq_iir/eq_iir.c
ret = eq_iir_setup(cd, source_c->stream.channels);
ret = eq_iir_setup(mod, source_c->stream.channels);
buffer_release(sink_c);
buffer_release(source_c);
Copy link
Collaborator

@lyakh lyakh Jul 18, 2022

Choose a reason for hiding this comment

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

you now have 3 locations where you release buffers. Wouldn't it be better to keep the original if {} else {} code structure and release the buffers once at the end?

@lyakh lyakh dismissed their stale review July 18, 2022 09:56

comments addressed, thanks!

@lgirdwood lgirdwood merged commit ea72e3e into thesofproject:main Jul 18, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 19, 2022 •
edited
Loading

This PR did not pass PR CI: https://sof-ci.01.org/sofpr/PR5926/build903/devicetest/

Unsurprisingly, there are now a lot of failures in daily test run 14058
Start Time: 2022-07-18 21:29:41 UTC
End Time: 2022-07-19 02:32:40 UTC
Kernel Branch: topic/sof-dev
Kernel Commit: c02c3b653c73
SOF Branch: main
SOF Commit: ea72e3e97e10

SOF#6020: [BUG] DSP panic when testing signal stop/start on DMIC pipeline
SOF#6022: [BUG][APL_UP2_PCM512X] kcontrol xx read failed for widget EQIIR y.z

(Thanks @keqiaozhang for the identification)

@ranj063
Copy link
Collaborator Author

ranj063 commented Jul 19, 2022

This PR did not pass PR CI: https://sof-ci.01.org/sofpr/PR5926/build903/devicetest/

Unsurprisingly, there are now a lot of failures in daily test run 14058 Start Time: 2022-07-18 21:29:41 UTC End Time: 2022-07-19 02:32:40 UTC Kernel Branch: topic/sof-dev Kernel Commit: [c02c3b653c73 (https://github.com/thesofproject/linux/commit/c02c3b653c73) SOF Branch: main SOF Commit: ea72e3e97e10

SOF#6020: [BUG] DSP panic when testing signal stop/start on DMIC pipeline SOF#6022: [BUG][APL_UP2_PCM512X] kcontrol xx read failed for widget EQIIR y.z

(Thanks @keqiaozhang for the identification)

Thanks @marc-hb looking at this now

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 19, 2022

Temporary revert submitted in #6029, thx @ranj063

@RanderWang RanderWang mentioned this pull request Sep 7, 2022
Merged
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

@andrula-song andrula-song andrula-song left review comments

@lgirdwood lgirdwood lgirdwood approved these changes

@singalsu singalsu singalsu approved these changes

@aborisovich aborisovich Awaiting requested review from aborisovich

@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.

8 participants

@ranj063 @marc-hb @fredoh9 @lyakh @lgirdwood @keqiaozhang @singalsu @andrula-song