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

intel-npu-driver: init at 1.13.0 #382756

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pseudocc
Copy link

@pseudocc pseudocc commented Feb 17, 2025

Packaging the Intel NPU firmware (for Linux kernel module: intel_vpu).

[nix-shell:~/.cache/nixpkgs-review/pr-382756-4]$ tree results/intel-npu-driver-x86_64-linux/
results/intel-npu-driver-x86_64-linux/
├── bin
│   ├── npu-kmd-test
│   └── npu-umd-test
└── lib
    ├── libze_intel_vpu.so -> libze_intel_vpu.so.1
    ├── libze_intel_vpu.so.1 -> libze_intel_vpu.so.1.13.0
    └── libze_intel_vpu.so.1.13.0

3 directories, 5 files

[nix-shell:~/.cache/nixpkgs-review/pr-382756-4]$ tree results/intel-npu-firmware-x86_64-linux
results/intel-npu-firmware-x86_64-linux
└── lib
    └── firmware
        └── intel
            └── vpu
                ├── mtl_vpu_v0.0.bin -> vpu_37xx_v0.0.bin
                ├── vpu_37xx_v0.0.bin
                └── vpu_40xx_v0.0.bin

5 directories, 3 files

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: hardware 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Feb 17, 2025
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Feb 17, 2025
@pseudocc
Copy link
Author

I created a flake for it, and verified the firmware worked well.
intel/linux-npu-driver#67
pseudocc/nix-config@27ee1af

Comment on lines +201 to +202
($device eq "0x7d1d" || $device eq "0xad1d" ||
$device eq "0x643e" || $device eq "0xb03e");
Copy link
Author

Choose a reason for hiding this comment

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

PCI ID are copied from the intel_vpu linux driver module.

Copy link
Member

@osbm osbm left a comment

Choose a reason for hiding this comment

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

I only have trivial change requests, package seems sufficiently up to standard otherwise.

@jwludzik
Copy link

The NPU firmware is strictly connected with NPU compiler and driver. This is a problem, because whenever you want to use newer driver, ex. upcoming v1.14.0 release, then you should use the same firmware from the release as well. If the firmware is not matched with compiler, then we do not guarantee that all works correctly. Best would be to have a common package - firmware, driver and compiler. That is why the NPU firmware never reached the linux-firmware repository

Copy link
Member

Choose a reason for hiding this comment

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

I question this entire module. It just does to little to be added.

Copy link
Author

Choose a reason for hiding this comment

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

I was just trying to get rid of the firmware loading error at the first place, while according to @jwludzik, other packages are also required to get NPU really work, I think it's good to keep the module to install all related packages together.

@@ -196,6 +196,11 @@ sub pciCheck {
($device eq "0x4229" || $device eq "0x4230" ||
$device eq "0x4222" || $device eq "0x4227");

push @attrs, "hardware.cpu.intel.enableNpuFirmware = true;" if
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend

Suggested change
push @attrs, "hardware.cpu.intel.enableNpuFirmware = true;" if
push @attrs, "hardware.firmware = [ pkgs.intel-npu-firmware ];" if

@pseudocc
Copy link
Author

Thank you guys for reviewing this, I am changing this to WIP for packaging related driver and compiler as well.

@pseudocc pseudocc marked this pull request as draft February 20, 2025 02:48
@pseudocc pseudocc changed the title intel-npu-firmware: init at 1.13.0 intel-npu-driver: init at 1.13.0 Feb 20, 2025
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Feb 24, 2025
@pseudocc pseudocc force-pushed the intel-npu-firmware branch 7 times, most recently from 4d7bb9c to 2d2b4c4 Compare February 26, 2025 06:21
@pseudocc pseudocc marked this pull request as ready for review February 26, 2025 07:22
inherit (intel-npu-driver) version src;
pname = "intel-npu-firmware";

installPhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to compile everything again? why not just copy it to a separate output?

Copy link
Author

Choose a reason for hiding this comment

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

I thought inheriting version and src will not cause building intel-npu-driver, let me re-organize this.

passthru = {
inherit standalone;

level-zero = stdenv.mkDerivation {
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 the justification that we cannot use our system level-zero?

Copy link
Author

Choose a reason for hiding this comment

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

somehow missed it, will remove this whole part.

Comment on lines +37 to +18
mkInstallPhase = lib.concatMapStringsSep "\n" (
component: "cmake --install . --component ${component} --prefix $out"
);
Copy link
Member

Choose a reason for hiding this comment

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

We cannot use installTarget?

Copy link
Author

Choose a reason for hiding this comment

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

I failed to find the make targets to do identical stuffs than this, I guess we just use this --component option here.

Only support the standalone build.
@pseudocc pseudocc force-pushed the intel-npu-firmware branch from 2d2b4c4 to 74fcd25 Compare March 4, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: hardware 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants