Skip to content

audio: add aligned limits for component copy function#5266

Merged
lgirdwood merged 1 commit intothesofproject:mainfrom
andrula-song:add-API
May 20, 2022
Merged

audio: add aligned limits for component copy function#5266
lgirdwood merged 1 commit intothesofproject:mainfrom
andrula-song:add-API

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 xiaoyuan.song@intel.com

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.

@andrula-song andrula-song requested a review from singalsu January 27, 2022 02:52
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).

@andrula-song andrula-song requested a review from lyakh February 14, 2022 03:14
@andrula-song andrula-song force-pushed the add-API branch 2 times, most recently from d2fd48e to d9824de Compare February 17, 2022 07:58
@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

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 dbaluta and plbossart and removed request for dbaluta, lbetlej, mmaka1 and plbossart 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
Contributor

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

@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 <xiaoyuan.song@intel.com>
@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
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