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

Change RKNN default core mask to all cores #1744

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

levyishai
Copy link

@levyishai levyishai commented Jan 24, 2025

After some testing, we've found that using all cores all the time is the fastest option, even when running more than one object detection pipeline simultaneously. This is using RKNN_NPU_CORE_0_1_2 instead of RKNN_NPU_CORE_AUTO

@levyishai levyishai requested a review from a team as a code owner January 24, 2025 03:37
@crschardt
Copy link
Contributor

Can you share some testing data that supports this?

@levyishai
Copy link
Author

levyishai commented Jan 25, 2025

Can you share some testing data that supports this?

I'm currently having trouble testing with the latest PV build (camera disconnects, camera feed looks pink for OV9782). Fwiw, that's what we've found to be the only real difference between the high-performance fork, which for us had more fps and less latency.
Hoping to find the issues with my testing setup/submit and issue/create a PR to fix the issues I'm having and bring more concrete numbers.

@levyishai levyishai changed the title Change default core mask to all cores Change RKNN default core mask to all cores Jan 25, 2025
@levyishai
Copy link
Author

levyishai commented Feb 6, 2025

Screenshot (44)
Before updating from the fork

Screenshot (46)
After changing the core mask to all cores

The current way gave me a stable 33 fps (even for two cameras), while changing it gave a stable 43 fps for one camera (which did sometimes jump to around 47), and two cameras resulted in a not-so-stable 32-37 fps.

While this might reduce efficiency for two cameras (by not so much), it ups the efficiency for the use-case which I assume is the most common - one camera. I believe it's worth a shot, or at least making this the solution when having one camera.

Feel free to test my data!

Edit: This testing was done with two OV9782 cameras at max res.

@spacey-sooty
Copy link
Member

Can this setting be changed in the dashboard at all?

@levyishai
Copy link
Author

Can this setting be changed in the dashboard at all?

Can be done, though this is defined on init as far as I see.

@samfreund
Copy link
Contributor

Given that the performance of multiple cameras is worse, it might be desirable to have it as a toggle for teams wishing to run multiple pipelines. If you do decide to add it, I would suggest putting it under the Object Detection card in the settings.

@spacey-sooty
Copy link
Member

I agree with Sam but would also say we need to add docs information documenting this testing so people can make an informed decision on what to select for their usecase. Happy to default on this though

@levyishai
Copy link
Author

Given that the performance of multiple cameras is worse, it might be desirable to have it as a toggle for teams wishing to run multiple pipelines. If you do decide to add it, I would suggest putting it under the Object Detection card in the settings.

I'd love to add it as some kind of toggle button, I'm just not so sure on how to do it, especially considering this is set on init. Do you have any pointers?

@spacey-sooty
Copy link
Member

You could either change the rknn JNI repo to allow setting it at runtime or recreate it when the option is changed

@levyishai
Copy link
Author

You could either change the rknn JNI repo to allow setting it at runtime or recreate it when the option is changed

Just found I can use this. I'll try to see how I add something you can select from.

@levyishai
Copy link
Author

All seems to work, would love a CR.

v-model="currentPipelineSettings.useAllCores"
label="Use All Cores"
:label-cols="4"
tooltip="Enables or Disables using all the cores of the NPU. When on, this might yield faster results for one camera, but slower/more unstable results for multiple cameras. Might take a few seconds to take effect, or require a restart."
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a docs section on neural networks? if so we should add that there as well

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah we do, it's about object detection

Copy link
Member

Choose a reason for hiding this comment

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

This behaviour should be documented there

@spacey-sooty
Copy link
Member

Logic generally looks good but I don't have any hardware I can test this on

@steve-carpenter
Copy link

Any config change available in the interim to make this the default (w/ out PR)?

@samfreund
Copy link
Contributor

Any config change available in the interim to make this the default (w/ out PR)?

You can always grab the build from this PR. Afaik, there are no config options other than that.

@spacey-sooty
Copy link
Member

Any config change available in the interim to make this the default (w/ out PR)?

No, this PR adds a config option

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