Skip to content

Audio: SRC: Audio: SRC: Use only module API and fix IPC4 capture direction #7440

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
Merged
kv2019i merged 2 commits into from
May 10, 2023
Merged

Audio: SRC: Audio: SRC: Use only module API and fix IPC4 capture direction #7440

kv2019i merged 2 commits into from
May 10, 2023

Conversation

@singalsu
Copy link
Collaborator

@singalsu singalsu commented Apr 13, 2023 •
edited
Loading 

This patch cleans up SRC component. There is no need to keep
IPC3 version build with legacy component interface. The patch
mainly removes code under CONFIG_IPC_MAJOR_3 and makes some
functions available for all builds from CONFIG_IPC_MAJOR_4.

The IPC4 capture issue is fixed by using the src_init()
IPC rates for source and sink rate initialize. It needs a
similar kernel update to always set those values. In practical
topologies in prepare the buffers may not contain the correct
stream information because the order of host copier and
dai copier pipeline prepare can be host first where capture
SRC is usually placed.

@singalsu singalsu force-pushed the src_ipc3_module_adapter branch from 19b70c4 to 147f602 Compare April 14, 2023 12:37
@lgirdwood
Copy link
Member

lgirdwood commented Apr 25, 2023

@singalsu any ETA ?

paulstelian97
paulstelian97 reviewed Apr 25, 2023
View reviewed changes
src/audio/src/src.c Outdated
Copy link
Collaborator

@paulstelian97 paulstelian97 Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Does this work with XCC builds? IPC3 on xtensa isn't dead just yet. Do those implementations not have stricter alignment requirements?

Edit: I just noticed that this function wouldn't be called at all on IPC3 on the old code. Still worth checking.

Copy link
Collaborator Author

@singalsu singalsu Apr 25, 2023

Choose a reason for hiding this comment

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

Yep, I'm testing with IPC3 too and will do for SRC after I can get back to this topic. I'm still using IPC3 builds for tests since we don't have everything yet converted to module adapter and IPC4. I prefer to develop in smaller steps like legacy -> module_adapter -> IPC3 to IPC4. This is my personal work preference.

Copy link
Collaborator

@paulstelian97 paulstelian97 Apr 28, 2023

Choose a reason for hiding this comment

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

I have tested this patch on i.MX8QM (platform "imx8") and it seems to be fine on GCC (XTOS and Zephyr) and XCC (Zephyr, support not yet upstream).

Copy link
Collaborator Author

@singalsu singalsu Apr 28, 2023

Choose a reason for hiding this comment

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

I have tested this patch on i.MX8QM (platform "imx8") and it seems to be fine on GCC (XTOS and Zephyr) and XCC (Zephyr, support not yet upstream).

Great! I just fixed some IPC4 issues and will test a bit more before update. Did you try also capture direction?

Copy link
Collaborator

@paulstelian97 paulstelian97 Apr 28, 2023

Choose a reason for hiding this comment

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

I don't think we have any SRC + capture topologies on i.MX so nothing to test. So only playback tested.

Copy link
Collaborator Author

@singalsu singalsu Apr 28, 2023

Choose a reason for hiding this comment

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

I got a capture direction SRC fail in testbench that is IPC3 but I haven't replicated it in a real device, so I'm not sure it's a real issue.

Copy link
Collaborator

@paulstelian97 paulstelian97 Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, thankfully @iuliana-prodan really was quick to point out that we do in fact have the capture direction as well. Just tested it, seems fine on my end. Maybe it takes multiple iterations to reproduce? (I only tried once on two rates where SRC has to do work)

Now we don't have DMIC but just classical stuff like the wm8960 (I'm on that one for testing). This should exclude SRC related issues though.

Copy link
Collaborator Author

@singalsu singalsu Apr 28, 2023

Choose a reason for hiding this comment

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

That's very good news! The pipelines are not fully accurate in testbench with file instead of host/dai. In my case fail happened immediately in prepare() where one of the requested rates was zero.

@singalsu
Copy link
Collaborator Author

singalsu commented Apr 25, 2023

@singalsu any ETA ?

Should be few days after finding solution to current DMIC issue. I need to make IPC3 and IPC4 test topologies to avoid breaking things with this, we've removed from SOF platforms those previously had test topologies for SRC and we can't use v2.2 to test this change.

@singalsu singalsu force-pushed the src_ipc3_module_adapter branch 3 times, most recently from 66d02fb to 3112ec2 Compare May 4, 2023 12:55
@singalsu singalsu changed the title Audio: SRC: Use module adapter also in IPC3 build Audio: SRC: Audio: SRC: Use only module API and fix IPC4 capture direction May 4, 2023
@singalsu singalsu marked this pull request as ready for review May 4, 2023 12:57
@singalsu singalsu mentioned this pull request May 8, 2023
Merged
@singalsu singalsu force-pushed the src_ipc3_module_adapter branch from 3112ec2 to 6a0ac04 Compare May 8, 2023 15:48
ranj063
ranj063 reviewed May 8, 2023
View reviewed changes
Copy link
Collaborator

@ranj063 ranj063 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 @singalsu. Just a few minor comments to address from my side

@singalsu singalsu force-pushed the src_ipc3_module_adapter branch from 6a0ac04 to a32a97b Compare May 9, 2023 10:21
@singalsu singalsu mentioned this pull request May 9, 2023
Open
singalsu added 2 commits May 10, 2023 10:48
@singalsu
Audio: Module adapter: Pass entire new() IPC to SRC
9ad539e 
This patch differentiates SRC component from normal processing
component IPC. The corrected size allows the client component to
receive all of init() IPC.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Audio: SRC: Use only module API and fix IPC4 capture direction
c12866f 
This patch cleans up SRC component. There is no need to keep
IPC3 version build with legacy component interface. The patch
mainly removes code under CONFIG_IPC_MAJOR_3 and makes some
functions available for all builds from CONFIG_IPC_MAJOR_4.

The IPC4 capture issue is fixed by using the src_init()
IPC rates for source and sink rate initialize. It needs a
similar kernel update to always set those values. In practical
topologies in prepare the buffers may not contain the correct
stream information because the order of host copier and
dai copier pipeline prepare can be host first where capture
SRC is usually placed.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu force-pushed the src_ipc3_module_adapter branch from a32a97b to c12866f Compare May 10, 2023 07:49
@singalsu
Copy link
Collaborator Author

singalsu commented May 10, 2023

No changes, new push to restart CI.

btian1
btian1 approved these changes May 10, 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.

@singalsu , there are two patches in this PR, however, the comments part is totally same with the second patch, may need change PR title and comments slightly to reflect the first patch?

@singalsu
Copy link
Collaborator Author

singalsu commented May 10, 2023

@singalsu , there are two patches in this PR, however, the comments part is totally same with the second patch, may need change PR title and comments slightly to reflect the first patch?

Could it be a problem in your browser or github? The commits should be:

Audio: Module adapter: Pass entire new() IPC to SRC

This patch differentiates SRC component from normal processing
component IPC. The corrected size allows the client component to
receive all of init() IPC.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

and

Audio: SRC: Use only module API and fix IPC4 capture direction

This patch cleans up SRC component. There is no need to keep
IPC3 version build with legacy component interface. The patch
mainly removes code under CONFIG_IPC_MAJOR_3 and makes some
functions available for all builds from CONFIG_IPC_MAJOR_4.

The IPC4 capture issue is fixed by using the src_init()
IPC rates for source and sink rate initialize. It needs a
similar kernel update to always set those values. In practical
topologies in prepare the buffers may not contain the correct
stream information because the order of host copier and
dai copier pipeline prepare can be host first where capture
SRC is usually placed.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

@kv2019i
Copy link
Collaborator

kv2019i commented May 10, 2023

One case of "ASSERTION FAIL [aligned_addr == addr]" on one DUT in https://sof-ci.01.org/sofpr/PR7440/build7322/devicetest/index.html . Not related to this PR.

Otherwise looks good, proceeding with merge.

@kv2019i kv2019i merged commit 1cc853d into thesofproject:main May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulstelian97 paulstelian97 paulstelian97 left review comments

@ranj063 ranj063 ranj063 approved these changes

@btian1 btian1 btian1 approved these changes

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

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

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

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

6 participants

@singalsu @lgirdwood @kv2019i @ranj063 @paulstelian97 @btian1