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.

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

Audio: Component: HiFi5 implementation of functions. #8695

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

andrula-song
Copy link
Contributor

@andrula-song andrula-song commented Jan 4, 2024

  1. Add HiFi5 implementation of audio_stream_copy, compares with HiFi3 version can save about 29% cycles.
  2. Add HiFi5 implementation of cir_buf_copy, compares with generic c version can save about 40% cycles.

@andrula-song
Copy link
Contributor Author

andrula-song commented Jan 4, 2024

with the xtensa simulator, here are the test results:
Screenshot from 2024-01-04 14-40-56

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.

Good stuff, we can address the Kconfig work later as part of teh work that @btian1 is doing.

src/audio/component.c Outdated Show resolved Hide resolved
@@ -240,7 +240,7 @@ int audio_stream_copy(const struct audio_stream *source, uint32_t ioffset,
int ssize = audio_stream_sample_bytes(source); /* src fmt == sink fmt */
ae_int16x4 *src = (ae_int16x4 *)((int8_t *)audio_stream_get_rptr(source) + ioffset * ssize);
ae_int16x4 *dst = (ae_int16x4 *)((int8_t *)audio_stream_get_wptr(sink) + ooffset * ssize);
int shorts = samples * ssize >> 1;
size_t shorts = samples * ssize >> 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, I don't understand this. Firstly, I think size_t is supposed to be used as a number of bytes, whereas here it's a number of shorts. Secondly, "reducing the risk of a forever loop" - shouldn't such a risk actually be made to be equal to 0? Besides, I don't see how this change affects that risk at all - the loop is over while (shorts) so it doesn't seem to be affected by this type change. Thirdly, IIUC AE_MIN_32_signed(shorts, shorts_copied) is a signed comparison, so I don't understand why shorts now has to be unsigned.

Copy link
Contributor Author

@andrula-song andrula-song Mar 8, 2024

Choose a reason for hiding this comment

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

so better to keep the signed type and use while(short > 0) to avoid the risk, though it shouldn't happen.

@singalsu
Copy link
Collaborator

singalsu commented Feb 8, 2024

Good improvement! Approve - with fix suggested by Guennadi.

Add HiFi5 implementation if function audio_stream_copy, compared
with HiFi3 version, the HiFi5 method can save about 29% cycles.

Signed-off-by: Andrula Song <andrula.song@intel.com>
Use while (shorts > 0) instead of while (short) to reduce the forever
loop risk.

Use general instruction AE_MIN32 replace AE_MIN_32_signed which is an
internal proto intended for Xtensa compiler.

Signed-off-by: Andrula Song <andrula.song@intel.com>
Add HiFi3 & HiFi5 implementation of function cir_buf_copy.
Compared with generic C version, the HiFi3 version can save
about 3% cycles and HiFi5 version can save about 40% cycles.

Signed-off-by: Andrula Song <andrula.song@intel.com>
@kv2019i kv2019i merged commit 02e8837 into thesofproject:main Mar 8, 2024
38 of 44 checks passed
@marc-hb marc-hb requested a review from andyross March 8, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants