Skip to content

Audio: Mixer: Optimize sources and sink buffers access #4944

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
lgirdwood merged 2 commits into from
Nov 11, 2021
Merged

Audio: Mixer: Optimize sources and sink buffers access #4944

lgirdwood merged 2 commits into from
Nov 11, 2021

Conversation

@singalsu
Copy link
Collaborator

@singalsu singalsu commented Nov 1, 2021 •
edited
Loading

  • The first commit just fixes the issue with S24_LE mixing
  • The second commit replaces the buffer read/write frag loop with block processing until next circular wrap
  • The mixer saturation optimize is handled by PR Audio: Optimize saturation inline functions in format.h #4978 that adds to format.h inline functions with intrinsics versions

@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 4627493 to 2c6b4d1 Compare November 1, 2021 16:55
lyakh
lyakh reviewed Nov 2, 2021
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

a couple of minor improvement proposals

@kv2019i kv2019i mentioned this pull request Nov 2, 2021
Closed
@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 2c6b4d1 to a10ca7d Compare November 2, 2021 17:27
dbaluta
dbaluta reviewed Nov 3, 2021
View reviewed changes
@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from a10ca7d to c4b08e6 Compare November 3, 2021 12:54
@singalsu singalsu requested review from dbaluta and lyakh November 3, 2021 13:01
@singalsu
Copy link
Collaborator Author

singalsu commented Nov 3, 2021

I'm keeping this draft until I'm confident it works fully. Cmocka tests only 16 frames of 32 bit data. Nocodec topologies allow test only with 16 bit so 24 bit untested. I need to tweak a topology to allow 16/24/32 for both PCMs. I testing it myself my playing back to L1, R1, L2, R2 different frequencies and then observe the spectrogram for all frequencies to be present in output mix without glitches.

@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from c4b08e6 to daecc08 Compare November 3, 2021 14:28
@singalsu
Copy link
Collaborator Author

singalsu commented Nov 4, 2021

OK, this optimized version with 32 bit processing for 24 bit passed system/merge/build this. Next I will restore the version with proper 24 bit processing for 24 bit.

@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 65ec6ab to 6c1e916 Compare November 4, 2021 10:01
@singalsu singalsu marked this pull request as ready for review November 4, 2021 10:02
kv2019i
kv2019i approved these changes Nov 4, 2021
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.

Looks good as well. Very good that you added a comment about the 16x4 SIMD experiment. This is very useful to have in git history.

src/audio/mixer.c Outdated
Copy link
Collaborator

@kv2019i kv2019i Nov 4, 2021

Choose a reason for hiding this comment

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

This could have been a separate commit (and even PR) as well.

Copy link
Collaborator Author

@singalsu singalsu Nov 5, 2021

Choose a reason for hiding this comment

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

@dbaluta shared your thoughts. If preferred I can do one more iteration The system/merge/build step still fails so this can't be merged yet. Please respond if separate PR or commit desired.

Copy link
Member

@lgirdwood lgirdwood Nov 8, 2021

Choose a reason for hiding this comment

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

can we split this for bisect. Thanks.

Copy link
Collaborator Author

@singalsu singalsu Nov 9, 2021

Choose a reason for hiding this comment

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

Which way better?

  1. First fix the bug then optimize
  2. Optimize but preserve the bug, then fix it

Copy link
Collaborator Author

@singalsu singalsu Nov 10, 2021

Choose a reason for hiding this comment

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

I'll go for 1.

@singalsu
Copy link
Collaborator Author

singalsu commented Nov 5, 2021

Looks good as well. Very good that you added a comment about the 16x4 SIMD experiment. This is very useful to have in git history.

I learned afterwards the the stuff at main copy() function level is 6 MCPS while the mixer functions I worked with are 4 MCPS for 2x stereo streams. If the copy() function could improve (buffer locking, unlocking) the SIMD might show more gain. But that should be another PR.

@singalsu singalsu mentioned this pull request Nov 5, 2021
Closed
lgirdwood
lgirdwood reviewed Nov 8, 2021
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.

Need to be split patches for bisect (since we have a fix and new feature).

src/audio/mixer.c Outdated
Copy link
Member

@lgirdwood lgirdwood Nov 8, 2021

Choose a reason for hiding this comment

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

can we split this for bisect. Thanks.

src/audio/mixer.c Outdated
Copy link
Member

@lgirdwood lgirdwood Nov 8, 2021

Choose a reason for hiding this comment

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

It would be a lot better if the sat_int24 implementation had the HIFI implementation.

Copy link
Collaborator Author

@singalsu singalsu Nov 9, 2021

Choose a reason for hiding this comment

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

I was thinking that but it would be a quite invasive change. There would be need for saturate from 64b to 24b and 32b to 24b version. 32bi could be with this name, and edit possible uses with 64 bit argument to other components. If you think it's worth the risk (also sat_int16, sat_int32) I could do it. Should I proceed?

Copy link
Collaborator Author

@singalsu singalsu Nov 10, 2021

Choose a reason for hiding this comment

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

I'll do this into a separate PR since it impacts nearly every processing component. The saturate optimization was less significant for mixer so it's OK to proceed without.

@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 6c1e916 to 7d58016 Compare November 10, 2021 08:27
@singalsu singalsu changed the title Audio: Mixer: Optimize sources and sink buffers access [Do not merge] Audio: Mixer: Optimize sources and sink buffers access Nov 10, 2021
@singalsu
Copy link
Collaborator Author

singalsu commented Nov 10, 2021

Do not merge. Testing just the first commit with 24 bit mixer function add (duplicate of 32 bit with saturate fix).

@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 7d58016 to 28c58aa Compare November 10, 2021 08:52
@singalsu
Audio: Mixer: Fix saturate of S24_LE PCM output
891edd1 
This patch adds a 24 bit mixer function. Previously 24 bit was
handled with 32 bit function. It did not properly saturate the
output to 24 bits.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 28c58aa to 50e6eca Compare November 10, 2021 11:32
@singalsu singalsu changed the title [Do not merge] Audio: Mixer: Optimize sources and sink buffers access Audio: Mixer: Optimize sources and sink buffers access Nov 10, 2021
@singalsu
Copy link
Collaborator Author

singalsu commented Nov 10, 2021

@lgirdwood I've addressed your comments now.

@singalsu
Copy link
Collaborator Author

singalsu commented Nov 10, 2021

SOFCI TEST

@singalsu singalsu requested a review from lgirdwood November 10, 2021 16:03
@singalsu
Copy link
Collaborator Author

singalsu commented Nov 11, 2021

SOFCI TEST

@singalsu
Audio: Mixer: Optimize sources and sink buffers access
3ef7e88 
This patch replaces the usage of audio_stream_read/write_frag_sXX()
usage with block processing based on audio_stream_bytes_without_wrap().

There are no processing related changes.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu force-pushed the mixer_readwritefrags_optimize branch from 50e6eca to 3ef7e88 Compare November 11, 2021 11:48
@singalsu
Copy link
Collaborator Author

singalsu commented Nov 11, 2021

Same code, just a minor commit text change to trigger re-run of CI tests.

@lgirdwood lgirdwood merged commit 3bb7cba into thesofproject:main Nov 11, 2021
@singalsu singalsu deleted the mixer_readwritefrags_optimize branch September 15, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lyakh lyakh lyakh approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@kv2019i kv2019i kv2019i approved these changes

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

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

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

@plbossart plbossart Awaiting requested review from plbossart plbossart 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.

5 participants

@singalsu @dbaluta @lyakh @lgirdwood @kv2019i