Skip to content

Make NEON an option #566

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kijsskidata
Copy link

We use a Tegra SOC, that does not have the neon extension. By default, the library uses NEON instructions for some operation if it is built for ARM architecture. This branch adds an option to disable NEON support.

FYI, I am not so well trained with cmake, so if you have some advice to improve this, I am happy to hear them.

some ARM platforms don't have the NEON extension,
to also support those, we add a possibility to disable NEON support
Copy link
Member

@TIS-Edgar TIS-Edgar left a comment

Choose a reason for hiding this comment

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

Small improvements. Looks fine for the most part.

@@ -24,6 +24,7 @@ option(TCAM_BUILD_TOOLS "Build additional utilities" ON)
option(TCAM_BUILD_DOCUMENTATION "Build internal code documentation" ON)
option(TCAM_BUILD_TESTS "Build tests." OFF)
option(TCAM_BUILD_VIRTCAM "Build virtual camera backend" ON)
option(TCAM_BUILD_USE_ARM_NEON "Use ARM neon extention" ON)
Copy link
Member

Choose a reason for hiding this comment

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

extension

@@ -136,3 +137,9 @@ if (TCAM_BUILD_GIGETOOL_ONLY)
set(TCAM_ENABLED_MODULES "tcam-gigetool")

endif (TCAM_BUILD_GIGETOOL_ONLY)

if (TCAM_BUILD_USE_ARM_NEON)
set(DUTILS_USE_NEON ON)
Copy link
Member

Choose a reason for hiding this comment

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

What is this variable for?
DUTILS_USE_NEON is not used anywhere else.
So if in doubt just remove it.

Copy link
Member

Choose a reason for hiding this comment

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

You can also write:

if(TCAM_BUILD_USE_ARM_NEON)
    target_link_libraries(tcamconvert
        PRIVATE
        dutils_img::img_filter_optimized
endif(TCAM_BUILD_USE_ARM_NEON)

Put it after the initial link_libraries and your down.
If there where multiple optional libraries that have to be collected your variable approach would be the preferable one. I consider this one optional.

@TIS-Edgar
Copy link
Member

Thanks for the merge request.
Just so you know, with holidays approaching, there will not be much happening for 2-3 weeks.
After today replies for this merge request will probably be delayed until mid January.

May I ask which NVidia SoC you are using?
It was my understanding that NEON pretty much exists on all arm64 chips nowadays....

@kijsskidata
Copy link
Author

@TIS-Edgar
The delay is no problem, fixed it for use by patching in our build system for now. Will address the issues you mentioned soon.

That it is an ARM64 was misinformation by me, sorry for this. I assumed, that it happens on all platforms, did not check good enough. We have some products, that use Nvidia tegra2 SOC, that we still have to support.

Thank you so far for your help and enjoy the holidays.

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.

2 participants