Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical workflow, SDK environment, packaging, and signing issues block reliable builds.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds CI and release packaging for Windows Arm64 Hexagon NPU binaries.
Changes:
- Installs Snapdragon SDKs and builds Hexagon artifacts.
- Packages and uploads the Hexagon build.
- Integrates the artifact into release dependencies and documentation links.
File summaries
| File | Description |
|---|---|
.github/workflows/release.yml |
Adds the Windows Arm64 Hexagon build and release integration. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
cc: @ggml-org/ggml-hexagon |
There was a problem hiding this comment.
🟡 Changes recommended
The unsigned artifact lacks the required catalog/signing path, and the unused OpenCL setup adds release risk.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/release.yml:1135
- This installs and configures the OpenCL SDK, but the build target list only builds
ggml-hexagonand the HTP skels, soggml-opencl.dllnever reaches this archive. That adds an unnecessary external download to every release and another failure point. Remove the OpenCL setup/options, or explicitly build and packageggml-openclif this is intended to be a dual-backend artifact.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The linked documentation does not explain how users can sign and install the unsigned release artifact.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Catalog OS compatibility and package upgrade behavior must be corrected before release.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/backend/snapdragon/windows.md:158
- This install command is not a reliable upgrade path for recurring releases. The bundled INF still has the fixed
DriverVer = 01/01/2026,1.0.0.0and marks every skel withCOPYFLG_NO_OVERWRITE(ggml/src/ggml-hexagon/libggml-htp.inf:6,29-32), so a machine that installed an earlier release can retain that same-ranked package and its old HTP libraries. Generate/bump the INF version for each release, or provide an explicit uninstall/replacement procedure before publishing this as the release installation flow.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| > $certificate="c:\Users\MyUser\Certs\ggml-htp-v1.pfx" | ||
| > $windowsSdkBin="c:\Program Files (x86)\Windows Kits\10\bin\10.0.26100.0" | ||
|
|
||
| > & "$windowsSdkBin\arm64\Inf2Cat.exe" /driver:"$package" /os:10_25H2_ARM64 |
|
can you PTAL @max-krasnyansky @CISC? @ericcurtin mentioned that you would be the best to ping |
Looks good overall. btw we also have signed releases available via https://github.com/qualcomm/GenieX/releases/ @zhiyuan8 Just FYI. |
I think the main problem is the lack of a signed llama-server binary, that one is particular is kinda useful (as well has having binaries/libs per llama.cpp upstream release). @max-krasnyansky would it be possible to have signed releases here also? |
Hmm. Not sure what you mean. There is no need to sign the host side apps and tools. |
I guess it would work... FWIW I don't have one of these devices :) So I haven't tinkered around with what does and doesn't need to be signed... The driving feature is this I guess llmmanorg/llmman#516 ... If we can pull from llama.cpp releases and it works... Everything is good... |
Co-authored-by: Sigbjørn Skjæret <sigbjorn.skjaeret@huggingface.co>
|
@CISC can you PTAL again? I have applied your suggestion |
|
@tfenster Can you run |
|
https://github.com/tfenster/llama.cpp/actions/runs/35724025761 Update: Fails :( Need to take a closer look to figure out why |
| set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY) | ||
| set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY) | ||
| set(CUSTOM_RUNELF_PATH "") | ||
| set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) |
There was a problem hiding this comment.
Hmm. Why do we need this?
Did you test with our docker toolchains and scripts/snapdragon/build.py?
There was a problem hiding this comment.
When you get the chance please try ./scripts/snapdragon/build.py --target adb. --target ubuntu also works.
You don't need anything other than the working docker setup. It'll run local build in snapdragon-toolchain docker container.
The change looks like it's not needed to me. And actually looks wrong. We don't build anything static on HTP. I'll take another look why your first build has failed.
|
I wonder should we do Linux as a follow on PR, just for future proofing... And I'm sure a lot of the inference nerds like myself want to run on Linux anyway |
| os: ubuntu-22.04 | ||
| - build: 'arm64' | ||
| os: ubuntu-24.04-arm | ||
| - build: 's390x' |
There was a problem hiding this comment.
I'm guessing it's because they are not using a branch, but PRing directly from master, so in order to test they need to temporary disable to allow action to finish.
Actually on this, maybe a Dockerfile makes more sense for Linux... |
Why "future" proofing ;) the future is here now! :) This kit runs Ubuntu, all NPU stuff is available via APT. IQ9 with dual-NPU. Arduino Ventuno-Q is around the corner as well. Also Ubuntu. Single NPU. IQ10 kits should start showing up soon as well. IQ10 with quad-NPU. All fully supported with Tensor and Row split implementations. So yeah, it'd be worthwhile packaging Ubuntu arm64 builds now. |
You see this issue with doing out of tree is for integrations like llmman, we have a release on a per commit basis for ROCM, CUDA, opencl, vulkan, etc. (it's a long list) at the per-commit level... So you can have the exact same version of llama.cpp/ggml across all the GPU types... And "llama-server" is the key binary other than the libs... Doing one out of tree is messy and prone to drift... |
I'm suggesting (or ACKing your suggestion) to do in-tree. |
FWIW I do keep an eye on the geniex ecosystem (heck I'm a contributor there) and I am also open to Qualcomm AI Engine Direct (qairt) backend in llmman as an alternate to llama.cpp . I actually don't have a qualcomm machine. Although strangely enough a detailed background with Qualcomm Automotive boards which are quite similar :) |
Alternative to llama.cpp? Nooo -- llama.cpp is the BEST! |
Oh I prefer llama.cpp you don't have to sell me on that :) But if people have special model types that need to run on an alternate runtime such as qairt in: https://github.com/llmmanorg/llmman that's fine too :) Heck this is a special runtime about to be merged soon: llama.cpp don't have interest in becoming an omni/multi-modal type engine in terms of outputs at least: |
|
Here's where I am now: This build https://github.com/tfenster/llama.cpp/actions/runs/35978701970 created a working llama-server.exe and after copying in the suggested geniex files, I could run it. The issue now is that somehow the NPU is not reporting memory. So if I just run it directly, it ignores the NPU With
Any idea on what might cause that memory issue and how to fix it? |
|
Without reading the code, this seems like a bug: I suspect it's looking at the wrong memory, if it's the hexagon backend, it should look at NPU memory (NPU being the device in this case), I suspect it's looking at the wrong memory (just a hypothesis) |
|
Hm, listing the devices also shows no memory for the NPU. Which in a way is true as it only has shared memory, but the same is true for the GPU, which does show memory |
|
The lack of memory reporting for NPU is "correct" because it is declared as an accelerator device type compared to a GPU (with dedicated VRAM). I've tried proposing a Memory Aware API so that devices can explicitly declare if they have memory information to report, while being accurate to the device type they are, but it's currently stuck: #22949 |
If we open a PR to get this from 0 to the correct value we might be lucky and everything might just work. I don't have a device though 😅 |
Ah. Yes. Sorry been meaning to follow up on this. Sorry for the delay. |
The correct value is all available RAM. There is no fixed limit. |
|
Windows task manager claims it is 16G, compared to 32G overall on the device (see screenshot) |

Overview
This PR implements the missing Hexagon NPU build
Additional information
fixes #26877
Requirements