Skip to content

Conversation

@kristofer-jonsson-arm
Copy link
Contributor

MPS3 AN547

  • Updating mps3_an547 board files adding DTB entries for Ethos-U.
  • Adding DTS bindings for the Ethos-U DTB entry.

Modules

  • Adding module files for building the Arm Ethos-U Core driver.
  • Updating TFLU module file to build Ethos-U operator.

Samples

  • Adding TFLU Ethos-U sample program.

Manifest

  • Adding hal_ethos_u to west manifest.

Signed-off-by: Kristofer Jonsson [email protected]
Signed-off-by: Fredrik Knutsson [email protected]

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_ethos_u N/A zephyrproject-rtos/hal_ethos_u@90ada2e (main) N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@microbuilder microbuilder requested review from microbuilder and removed request for galak and ioannisg February 8, 2022 13:50
west.yml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up that the typical way to review changes like this is to generate a PR with the changes in the module, and then reference that PR in the west.yml manifest here via:

revision: pull/54/head

Where 54 is the PR number.

The changes were already pushed directly to hal_ethos_u so it's no longer relevant in this case, but it's useful to go through the PR route in case something also needs to be changed in the module code.

@github-actions github-actions bot added the DNM This PR should not be merged (Do Not Merge) label Feb 9, 2022
@mbolivar-nordic
Copy link
Contributor

Can you please rebase?

@microbuilder
Copy link
Member

@kristofer-jonsson-arm I have to setup the AN547 FVP image to properly test this since QEMU doesn't have U55 support, but looking through the code quickly, is tflu used elsewhere that you've seen? We've been using tflm in our APIs at Linaro, and it probably makes sense to be consistent one way or the other since this will all end up in Zephyr.

I usually see TensorFlow Lite Micro referenced as TFLM in documentation, and I suspect the m might be more recognisable to people?

@kristofer-jonsson-arm
Copy link
Contributor Author

Sorry for the late response. I was on vacation last week.

I have rebased the patch and also renamed TFLu to TFLM.

Please note as Kevin points out that QEMU will not be able to run the provided unit test, because it is lacking a NPU. For this the Corstone-300 FVP needs to be used.

https://developer.arm.com/tools-and-software/open-source-software/arm-platforms-software/arm-ecosystem-fvps

@microbuilder
Copy link
Member

@kristofer-jonsson-arm I've asked @theotherjimmy to have a look at this for testing, if you can just monitor the CI output and fix anything that shows up? Since you're a new committer, I might need to fire up the CI to run after any changes, though. Just ping me if I miss an update. Thanks!

@microbuilder microbuilder self-assigned this Mar 18, 2022
@theotherjimmy
Copy link
Contributor

it looks to me like a step in the process is rebasing onto main, which seems to fail. Perhaps some of the merges are non-trivial?

@theotherjimmy
Copy link
Contributor

Since much of the commit log was trivial fixes or renames, I have squashed them all into a single commit. The branch is here: main...theotherjimmy:ethos-u and you're welcome to push that branch over this one.

@mbolivar-nordic
Copy link
Contributor

@kristofer-jonsson-arm could you take a look at the twister errors in the CI results?

@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Nov 15, 2022
Adding module files for building the Arm Ethos-U Core driver.
Updating TFLU module file to build Ethos-U operator.

Adding hal_ethos_u to west manifest.

Signed-off-by: Kristofer Jonsson <[email protected]>
Signed-off-by: Fredrik Knutsson <[email protected]>
Add a driver for the Arm Ethos-U NPU, including a Devicetree entry for
the mps3_an547 platform.

Signed-off-by: Kristofer Jonsson <[email protected]>
Signed-off-by: Fredrik Knutsson <[email protected]>
microbuilder
microbuilder previously approved these changes Nov 15, 2022
@carlescufi
Copy link
Member

@kristofer-jonsson-arm I have had to make quite a few changes to clean up your sample:

  • Moved it to samples/modules/tflite-micro
  • Added a sample.yaml
  • Cleaned up the CMakeLists.txt

Aside from multiple changes in the driver itself.

@carlescufi carlescufi removed the DNM This PR should not be merged (Do Not Merge) label Nov 15, 2022
@carlescufi
Copy link
Member

carlescufi commented Nov 15, 2022

@microbuilder @kristofer-jonsson-arm now that the sample is actually building in CI, there are errors that must be looked at by those with platform knowledge:

INFO    - 215/634 mps3_an547                zephyr/samples/modules/tflite-micro/tflm_ethosu/sample.drivers.tflm_ethosu FAILED Timeout (qemu 59.967s)
INFO    - /__w/zephyr/zephyr/twister-out/mps3_an547/zephyr/samples/modules/tflite-micro/tflm_ethosu/sample.drivers.tflm_ethosu/handler.log
ERROR   - E: Failed to switch security state and privilege level (ethosu_device_u55_u65.c:244)

E: Failed to initialize Ethos-U device (ethosu_driver.c:418)

*** Booting Zephyr OS build zephyr-v3.2.0-1611-g47a6e23aab63 ***
sender 0: Sending inference. job=0x30002a68, name=keyword_spotting_cnn_small_int8
runner 0: Received inference job. job=0x30002a68
W: No NPU driver handle available.

sender 0: Sending inference. job=0x30002ab4, name=keyword_spotting_cnn_small_int8
sender 1: Sending inference. job=0x30003270, name=keyword_spotting_cnn_small_int8
sender 1: Sending inference. job=0x300032bc, name=keyword_spotting_cnn_small_int8
[00:00:00.000,000] <err> ethos_u: Failed to initialize NPU with ethosu_init().

@microbuilder
Copy link
Member

microbuilder commented Nov 15, 2022

@microbuilder @kristofer-jonsson-arm now that the sample is actually building in CI, there are errors that must be looked at by those with platform knwoledge:

I'll have someone here dig into it, thanks for the cleanup.

EDIT: QEMU doesn't support the NPU, so that's the most likely explanation. This has to be run in the FVP. I'll get someone on the team with some experience with TFLM to kick the tires in, though.

@gmarull gmarull dismissed their stale review November 15, 2022 12:54

main concerns addressed, approvals left for people with more experience in this area.

@microbuilder microbuilder dismissed their stale review November 15, 2022 12:56

Stale review

@carlescufi
Copy link
Member

EDIT: QEMU doesn't support the NPU, so that's the most likely explanation. This has to be run in the FVP. I'll get someone on the team with some experience with TFLM to kick the tires in, though.

Oh, it doesn't? Then this should be as easy as setting this as "build only" for now. Let me do that then.

@microbuilder
Copy link
Member

microbuilder commented Nov 15, 2022

EDIT: QEMU doesn't support the NPU, so that's the most likely explanation. This has to be run in the FVP. I'll get someone on the team with some experience with TFLM to kick the tires in, though.

Oh, it doesn't? Then this should be as easy as setting this as "build only" for now. Let me do that then.

I've filed an action internally (to Linaro) to get this working with the FVP in CI, but that is likely a new PR entirely, which I'll follow up separately and enable this sample in CI when that's working.

@carlescufi
Copy link
Member

EDIT: QEMU doesn't support the NPU, so that's the most likely explanation. This has to be run in the FVP. I'll get someone on the team with some experience with TFLM to kick the tires in, though.

Oh, it doesn't? Then this should be as easy as setting this as "build only" for now. Let me do that then.

I've filed an action internally (to Linaro) to get this working with the FVP in CI, but that is likely a new PR entirely, which I'll follow up separately and enable this sample in CI when that's working.

Great! The sample will be built in CI, it just won't be run for now.

Add a sample program that demonstates how to run inferences on
Arm Ethos-U.

Signed-off-by: Kristofer Jonsson <[email protected]>
Signed-off-by: Fredrik Knutsson <[email protected]>
Signed-off-by: Carles Cufi <[email protected]>
@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Nov 15, 2022
@carlescufi carlescufi removed the DNM This PR should not be merged (Do Not Merge) label Nov 15, 2022
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

This is now good enough to get in, but please @kristofer-jonsson-arm refactor the sample so that it is generic, and uses the Ethos-U if available as a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.