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: add aligned limits for component copy function #5266

Merged
merged 1 commit into from
May 20, 2022

Conversation

andrula-song
Copy link
Contributor

add comp_get_copy_limits_with_lock_aligned API to meet requirement
of some xtensa intrinsics.

Signed-off-by: Andrula Song [email protected]

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.

@singalsu fyi, lets optimise all the stream API calls after this is merged.

src/audio/component.c Outdated Show resolved Hide resolved
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.

@andrula-song @singalsu it would also be good to remove the stream API calls today that are not used as a first step and then align the remaining calls to align on frame or SIMD frames (without the divisions).

src/audio/component.c Outdated Show resolved Hide resolved
src/include/sof/audio/audio_stream.h Outdated Show resolved Hide resolved
src/include/sof/audio/audio_stream.h Outdated Show resolved Hide resolved
src/include/sof/audio/audio_stream.h Outdated Show resolved Hide resolved
@andrula-song
Copy link
Contributor Author

the frame_aligned set in prepare function would be reset somewhere. Need some time to analyze

@andrula-song
Copy link
Contributor Author

some error should be nothing about code i commited, trigger test again

src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/audio/component.c Show resolved Hide resolved
src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/math/numbers.c Outdated Show resolved Hide resolved
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 to me, thanks!

@andrula-song andrula-song marked this pull request as ready for review February 25, 2022 08:54
@andrula-song andrula-song requested review from plbossart and dbaluta and removed request for dbaluta, plbossart, lbetlej and mmaka1 February 25, 2022 08:55
@andrula-song
Copy link
Contributor Author

since the volume hifi3 implementation version needs 8 bytes align, but the generic version or other components can have different align requirement

@cujomalainey I've answered the question before I close it , but it is pending. Sorry to close too fast.

@andrula-song
Copy link
Contributor Author

andrula-song commented May 13, 2022

it seems like the codestyle management has become more stringent,
WARNING: Possible repeated word: 'sink'
#262: FILE: src/include/sof/audio/component.h:858:

@cujomalainey
Copy link
Member

since the volume hifi3 implementation version needs 8 bytes align, but the generic version or other components can have different align requirement

@cujomalainey I've answered the question before I close it , but it is pending. Sorry to close too fast.

Ah you were tricked by githubs' review system, i think you have to submit your review first to create the comment then resolve it :)

@marc-hb
Copy link
Collaborator

marc-hb commented May 13, 2022

ERROR: "foo * bar" should be "foo *bar"

This should be fixed on your next push thanks to my #5806 (no need to rebase)

Like most linters, checkpatch is not always right, you don't always have to fix every single warning.

Checkpatch is right most of the time, you must TRY to fix every warning.

https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html

@marc-hb
Copy link
Collaborator

marc-hb commented May 13, 2022

ERROR: "foo * bar" should be "foo *bar"
struct comp_buffer __sparse_cache * source

This should be fixed on your next push thanks to my > #5806 (no need to rebase)

I got confused sorry: you must change this code as told.

@lgirdwood
Copy link
Member

@wszypelt dont think this should impact the DMIC test, I will rerun CI again to be sure.

@lgirdwood
Copy link
Member

SOFCI TEST

@andrula-song
Copy link
Contributor Author

andrula-song commented May 16, 2022

I recall the commit 08c234b passed all the test cases and Internal Intel CI System/merge/build . but these two commit just changed the code comment and Kconfig(remove the useless item), I don't think modify code comment would lead to those failed cases,trigger test again.

@andrula-song
Copy link
Contributor Author

SOFCI TEST

@andrula-song
Copy link
Contributor Author

@wszypelt can you help to check Internal Intel CI System/merge/build test ? I found that many PRs failed this item today.

@andrula-song
Copy link
Contributor Author

in fact wondering, we expect these functions to also be useful for other components in the future, not only for volume, right?
@lyakh yes, so put this function in audio_stream.h. So other components can call this function with their own byte_align & frame_align_req as in volume_set_alignment function which is called by volume_prepare function.

@wszypelt
Copy link

@andrula-song @lgirdwood Of course there was a problem with one machine, it should work fine now. Tests rerun in progress

src/include/sof/audio/audio_stream.h Outdated Show resolved Hide resolved
src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/audio/volume/volume.c Outdated Show resolved Hide resolved
src/audio/component.c Show resolved Hide resolved
@andrula-song
Copy link
Contributor Author

-->Is there some reason for frame_align_req to be inside the #if #else?
@marc-hb Since the byte_align and frame_align_req can be different between these two versions, even though now they are the same, but I think it is better to write it twice.

@andrula-song andrula-song force-pushed the add-API branch 5 times, most recently from fe05b68 to 9e32670 Compare May 17, 2022 06:03
Add API function such as comp_get_copy_limits_frame
_aligned(). This function would finally call audio_
stream_get_avail_frames_aligned to use right shift
instead of division, whichh reduce about 26% MCPS than
the original division method.
Developers should set the byte_align for xtensa intrinsics
and frame_align_req for algorithm limits only once in
component prepare or param function.

Signed-off-by: Andrula Song <[email protected]>
@andrula-song
Copy link
Contributor Author

@wszypelt can you help to check Internal Intel CI System/merge/build test again? I don't think the modification would lead to such error, thanks.

@marc-hb
Copy link
Collaborator

marc-hb commented May 18, 2022

@wszypelt can you help to check Internal Intel CI System/merge/build test again?

Always provide direct links because they keep changing:
https://sof-ci.01.org/sof-pr-viewer/#/build/PR5266/build9358965
Screenshot 2022-05-17 at 21 00 54

@andrula-song
Copy link
Contributor Author

@lgirdwood Can you help to review this PR? Thanks.

@lgirdwood lgirdwood merged commit 9611d12 into thesofproject:main May 20, 2022
@andrula-song andrula-song deleted the add-API branch October 19, 2022 06:54
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.

8 participants