-
Notifications
You must be signed in to change notification settings - Fork 94
Add Arch Linux support for xdna-driver #866
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
base: main
Are you sure you want to change the base?
Changes from all commits
24164d6
126c1a5
dd70faa
5aa2e92
d6fcf49
2951bb8
63480a4
16f0a4a
4d9eb85
88d8759
e3650e4
dfef9f3
bfd2b64
4f80d49
ea156e0
8cf1ba4
a666c05
b721228
3deb899
fb81b66
95a9dac
8d9c15d
d76a44b
71dd5c6
cde12fc
5855900
8e1a43b
81179fa
1e4e1c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -103,7 +103,7 @@ cd <root-of-source-tree> | |||||||||||||||||||||||||||||||||||||||
| exit | ||||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| ### Steps to create release build DEB package: | ||||||||||||||||||||||||||||||||||||||||
| ### Steps to create release build DEB package (Ubuntu/Debian): | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| ``` bash | ||||||||||||||||||||||||||||||||||||||||
| cd <root-of-source-tree>/build | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -121,7 +121,53 @@ cd ../../build | |||||||||||||||||||||||||||||||||||||||
| # To adapt according to your OS & version | ||||||||||||||||||||||||||||||||||||||||
| sudo apt reinstall ./Release/xrt_plugin.2.19.0_ubuntu22.04-x86_64-amdxdna.deb | ||||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||||
| You will find `xrt_plugin\*-amdxdna.deb` in Release/ folder. This package includes: | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| ### Steps to create release build packages (Arch Linux): | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| ``` bash | ||||||||||||||||||||||||||||||||||||||||
| cd <root-of-source-tree> | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Install dependencies (requires sudo) | ||||||||||||||||||||||||||||||||||||||||
| sudo ./tools/amdxdna_deps.sh | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Get submodules | ||||||||||||||||||||||||||||||||||||||||
| git submodule update --init --recursive | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Build XRT | ||||||||||||||||||||||||||||||||||||||||
| cd xrt/build | ||||||||||||||||||||||||||||||||||||||||
| ./build.sh -npu -opt | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Build and install XRT packages using pacman | ||||||||||||||||||||||||||||||||||||||||
| # PKGBUILDs are in xrt/build/arch/ | ||||||||||||||||||||||||||||||||||||||||
| cd arch | ||||||||||||||||||||||||||||||||||||||||
| makepkg -p PKGBUILD-xrt-base | ||||||||||||||||||||||||||||||||||||||||
| sudo pacman -U xrt-base-*.pkg.tar.zst | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| makepkg -p PKGBUILD-xrt-npu | ||||||||||||||||||||||||||||||||||||||||
| sudo pacman -U xrt-npu-*.pkg.tar.zst | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Build XDNA driver | ||||||||||||||||||||||||||||||||||||||||
| cd ../../../build | ||||||||||||||||||||||||||||||||||||||||
| ./build.sh -release | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Build and install XDNA plugin package | ||||||||||||||||||||||||||||||||||||||||
| makepkg -p PKGBUILD-xrt-plugin | ||||||||||||||||||||||||||||||||||||||||
| sudo pacman -U xrt-plugin-amdxdna-*.pkg.tar.zst | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Configure memory limits (required for NPU access) | ||||||||||||||||||||||||||||||||||||||||
| sudo bash -c 'echo "* soft memlock unlimited" >> /etc/security/limits.conf' | ||||||||||||||||||||||||||||||||||||||||
| sudo bash -c 'echo "* hard memlock unlimited" >> /etc/security/limits.conf' | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # Log out and log back in (or reboot) for memory limit changes to take effect | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+158
to
+161
|
||||||||||||||||||||||||||||||||||||||||
| sudo bash -c 'echo "* soft memlock unlimited" >> /etc/security/limits.conf' | |
| sudo bash -c 'echo "* hard memlock unlimited" >> /etc/security/limits.conf' | |
| # Log out and log back in (or reboot) for memory limit changes to take effect | |
| # ⚠️ **Warning:** Setting unlimited memlock for all users (`*`) is very permissive and can cause security or stability issues on multi-user or memory-constrained systems. | |
| # It is recommended to restrict this setting to a dedicated group (e.g., `xdna`) and only add users who need NPU access. | |
| # Create a group for XDNA users (if it doesn't exist) | |
| sudo groupadd -f xdna | |
| # Add your user to the group (replace $USER if needed) | |
| sudo usermod -aG xdna $USER | |
| # Set memlock limits for the xdna group | |
| sudo bash -c 'echo "@xdna soft memlock unlimited" >> /etc/security/limits.conf' | |
| sudo bash -c 'echo "@xdna hard memlock unlimited" >> /etc/security/limits.conf' | |
| # Log out and log back in (or reboot) for group and memory limit changes to take effect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On many system, it's not recommanded to modify /etc/security/limits.conf directly since it's a file that's managed by the packaging system (so modifying it directly implies headaches when updating packages). In many cases (debian, arch, ...) you'll find a /etc/security/limits.d folder where you can save a file, like 99-amdxdna.conf with the same command and they'll survive package upgrade.
I don't know and can't take a decision here if this driver should create a very unrestrictive access to memlock, like it does for permissions to /dev/accel/accel0 file, it's a choice that distribution should make, not us.
Similarly, adding a group and setting permissions for this group is also a distribution specific choice. This PR decided not to mess with system's group, so I don't think this suggestion is good. Also, messing with group is a manual operation since you can't/don't know which user to append to this group. It's very fragile and debug prone, since if you only do the group fiddling for the current user, the driver will suddenly not work for all other users of the system.
If we were to follow this advice, then the same group should be used for /dev/accel/accel0 file too, since it doesn't make sense to have both of them.
So to conclude, the note in the README is, IMHO, the best we can do in the driver and let the distribution package and set permission the way they want.
As for this PR, I'm not sure it's worth adding the 2 lines in PKGBUILD. On one side, if we do, it makes the driver works out-of-the-box, on the other hand, it gives all the users the ability to crash the system. I've tried to put numerical value on my system, but while it works for benchmarks, as soon as I launch a NPU model with Lemonade, it fails because it's too small. So it's definitively a mess and if the users don't put the lines by themselves, they will never know what/where to change so it doesn't crash in lemonade. What do you think @kashif?
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note mentions that XRT packages are in xrt/build/arch/, but the instructions in lines 136-147 show building XRT in xrt/build and then moving to a subdirectory arch. It would be clearer to explicitly state that the PKGBUILDs are generated during the XRT build process, or if they're provided by XRT, to mention that upfront.
| **Note for Arch Linux users**: The build system generates `.tar.gz` packages which are repackaged into proper Arch packages (`.pkg.tar.zst`) using the provided PKGBUILDs: | |
| **Note for Arch Linux users**: The build system generates `.tar.gz` packages which are repackaged into proper Arch packages (`.pkg.tar.zst`) using PKGBUILDs. | |
| The PKGBUILD files for XRT (`PKGBUILD-xrt-base`, `PKGBUILD-xrt-npu`) and the XDNA driver (`PKGBUILD-xrt-plugin`) are provided by the XRT repository and are available in the following locations after building: |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states that the package will be in the "Release/" folder, but for Arch Linux builds, the actual .pkg.tar.zst packages are created in the respective build directories (xrt/build/arch/ and build/). Only the intermediate .tar.gz file is in Release/. Consider clarifying: "You will find the intermediate xrt_plugin*.tar.gz in Release/ folder (which is repackaged into xrt-plugin-amdxdna-*.pkg.tar.zst using the PKGBUILD)."
| You will find `xrt_plugin*-amdxdna.deb` (Ubuntu/Debian) or `xrt_plugin*.tar.gz` (Arch Linux) in Release/ folder. This package includes: | |
| You will find the intermediate `xrt_plugin*.tar.gz` (Arch Linux) or `xrt_plugin*-amdxdna.deb` (Ubuntu/Debian) in the Release/ folder. For Arch Linux, this `.tar.gz` is repackaged into the final installable package `xrt-plugin-amdxdna-*.pkg.tar.zst` using the provided PKGBUILD, which can be found in the respective build directories (`xrt/build/arch/` or `build/`). These packages include: |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||||||||||||||||||||||
| # Maintainer: Your Name <your@email.com> | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| # Maintainer: Your Name <your@email.com> | |
| # Maintainer: Jane Doe <jane.doe@example.com> |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maintainer information is a placeholder. Replace "Your Name <your@email.com>" with actual maintainer details before distribution. This is important for package maintainability and user support.
| # Maintainer: Your Name <your@email.com> | |
| # Maintainer: Jane Doe <jane.doe@example.com> |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable assignment : ${XDNA_BUILD_DIR:="Release"} uses the colon null command which is a valid bash idiom for setting defaults. However, this pattern is unconventional in PKGBUILDs. The standard approach would be to use XDNA_BUILD_DIR=${XDNA_BUILD_DIR:-Release} which is clearer and more maintainable.
| : ${XDNA_BUILD_DIR:="Release"} | |
| XDNA_BUILD_DIR=${XDNA_BUILD_DIR:-Release} |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path $startdir/${XDNA_BUILD_DIR}/package/postinst uses unquoted variable expansion which could fail if XDNA_BUILD_DIR contains spaces or special characters. While "Release" is the default and unlikely to have spaces, it's good practice to quote variable expansions: "$startdir/${XDNA_BUILD_DIR}/package/postinst"
| tar -xzf "$tarball" -C "$pkgdir" | |
| # Copy the install scripts for reference (they'll be called from .install file) | |
| install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/postinst" \ | |
| "$pkgdir/opt/xilinx/xrt/share/amdxdna/package/postinst" | |
| install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/prerm" \ |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The install commands lack proper error handling. If the postinst or prerm files don't exist in the expected location, the install command will fail silently or cause the build to fail. Consider adding existence checks before these install commands or using || true if the files are optional.
| tar -xzf "$tarball" -C "$pkgdir" | |
| # Copy the install scripts for reference (they'll be called from .install file) | |
| mkdir -p "$pkgdir/opt/xilinx/xrt/share/amdxdna/package" | |
| if [ -f "$startdir/${XDNA_BUILD_DIR}/package/postinst" ]; then | |
| install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/postinst" \ | |
| "$pkgdir/opt/xilinx/xrt/share/amdxdna/package/postinst" | |
| fi | |
| if [ -f "$startdir/${XDNA_BUILD_DIR}/package/prerm" ]; then | |
| install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/prerm" \ | |
| "$pkgdir/opt/xilinx/xrt/share/amdxdna/package/prerm" | |
| fi |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,75 @@ | ||||||||||||||||||||
| post_install() { | ||||||||||||||||||||
| echo "Installing AMD XDNA driver via DKMS..." | ||||||||||||||||||||
|
|
||||||||||||||||||||
| install_datadir=/opt/xilinx/xrt/share/amdxdna | ||||||||||||||||||||
| udev_rules_d=/etc/udev/rules.d | ||||||||||||||||||||
| amdxdna_rules_file=99-amdxdna.rules | ||||||||||||||||||||
| dracut_conf_d=/etc/dracut.conf.d | ||||||||||||||||||||
| dracut_conf_file=amdxdna.dracut.conf | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # On systems with dracut, exclude driver from initramfs | ||||||||||||||||||||
| if [ -d ${dracut_conf_d} ]; then | ||||||||||||||||||||
| if [ ! -f ${dracut_conf_d}/${dracut_conf_file} ]; then | ||||||||||||||||||||
| touch ${dracut_conf_d}/${dracut_conf_file} | ||||||||||||||||||||
| fi | ||||||||||||||||||||
| grep -q '^omit_drivers\+=\" amdxdna \"' "${dracut_conf_d}/${dracut_conf_file}" || echo 'omit_drivers+=" amdxdna "' >> "${dracut_conf_d}/${dracut_conf_file}" | ||||||||||||||||||||
| fi | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Install DKMS module | ||||||||||||||||||||
| echo "Installing amdxdna Linux kernel module via DKMS..." | ||||||||||||||||||||
| if [ ! -x "$install_datadir/dkms_driver.sh" ]; then | ||||||||||||||||||||
| echo "Error: $install_datadir/dkms_driver.sh not found or not executable." >&2 | ||||||||||||||||||||
| return 1 | ||||||||||||||||||||
|
||||||||||||||||||||
| return 1 | |
| exit 1 |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The udev rule sets MODE="0666" which grants read and write access to all users. While this may be intentional for device access, it could pose a security risk by allowing any user to access the NPU device. Consider using GROUP-based permissions with a dedicated group (e.g., "render" or "video") and MODE="0660" instead.
| grep -q '^KERNEL=="accel\*",DRIVERS=="amdxdna",MODE="0666"$' ${udev_rules_d}/${amdxdna_rules_file} || \ | |
| echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0666"' >> ${udev_rules_d}/${amdxdna_rules_file} | |
| grep -q '^KERNEL=="accel\*",DRIVERS=="amdxdna",GROUP="video",MODE="0660"$' ${udev_rules_d}/${amdxdna_rules_file} || \ | |
| echo 'KERNEL=="accel*",DRIVERS=="amdxdna",GROUP="video",MODE="0660"' >> ${udev_rules_d}/${amdxdna_rules_file} |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rmmod amdxdna > /dev/null 2>&1 || true pattern silently ignores all errors, including cases where the module is in use and cannot be unloaded. This could lead to the subsequent modprobe loading the module while stale state exists. Consider checking if the module is loaded first with lsmod | grep -q amdxdna and handling the unload more explicitly, similar to the pre_remove function at line 57.
| rmmod amdxdna > /dev/null 2>&1 || true | |
| if lsmod | grep -q "amdxdna"; then | |
| rmmod amdxdna | |
| fi |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message "Warning: Failed to load amdxdna module. Check dmesg for details." could be more helpful by including the actual error code or suggesting specific troubleshooting steps. Consider capturing the modprobe exit status and including it in the message, or providing common failure reasons (e.g., kernel version mismatch, missing dependencies).
| modprobe amdxdna || echo "Warning: Failed to load amdxdna module. Check dmesg for details." | |
| if ! modprobe amdxdna; then | |
| exit_code=$? | |
| echo "Warning: Failed to load amdxdna module (exit code: $exit_code)." | |
| echo "Check 'dmesg' for details. Common reasons for failure:" | |
| echo " - Kernel version mismatch" | |
| echo " - Missing dependencies (run 'depmod -a' and try again)" | |
| echo " - Secure Boot enabled (may block unsigned modules)" | |
| fi |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rmmod amdxdna command has no error handling. If the module is in use or fails to unload for any reason, the script will fail silently or abort. Consider adding error handling with a descriptive message: rmmod amdxdna || echo "Warning: Failed to unload amdxdna module. It may be in use." This is especially important during package removal when users need to understand why the removal failed.
| rmmod amdxdna | |
| rmmod amdxdna || echo "Warning: Failed to unload amdxdna module. It may be in use." |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DKMS removal script is called without error handling or existence check, unlike the installation which checks for the script's existence (line 20-23). If the script doesn't exist or fails, the removal will abort with an unclear error. Consider adding: if [ -x "$install_datadir/dkms_driver.sh" ]; then "$install_datadir/dkms_driver.sh" --remove || echo "Warning: Failed to remove DKMS module"; fi
| $install_datadir/dkms_driver.sh --remove | |
| if [ -x "$install_datadir/dkms_driver.sh" ]; then | |
| "$install_datadir/dkms_driver.sh" --remove || echo "Warning: Failed to remove DKMS module" | |
| fi |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing quotes around variable expansion. On line 72, $install_datadir/dkms_driver.sh should be quoted as "$install_datadir/dkms_driver.sh" to handle paths with spaces safely and to be consistent with line 20 and 24 where the same variable is properly quoted.
| $install_datadir/dkms_driver.sh --remove | |
| "$install_datadir/dkms_driver.sh" --remove |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -41,6 +41,13 @@ try_compile() { | |||||||||
| tmpdir=$(mktemp -d /tmp/conftest-XXXXXX) | ||||||||||
| conftest_c="$tmpdir/conftest.c" | ||||||||||
| conftest_mk="$tmpdir/Makefile" | ||||||||||
| USE_LLVM="" | ||||||||||
| if [ -e /proc/config.gz ]; then | ||||||||||
| USE_LLVM=$(zgrep -q "CONFIG_CC_IS_CLANG=y" /proc/config.gz 2>/dev/null && echo -n "LLVM=1"); | ||||||||||
| elif [ -e /boot/config-$(KERNEL_VER) ]; then | ||||||||||
| USE_LLVM=$(grep -q "CONFIG_CC_IS_CLANG=y" /boot/config-$(KERNEL_VER) 2>/dev/null && echo -n "LLVM=1"); | ||||||||||
|
Comment on lines
+47
to
+48
|
||||||||||
| elif [ -e /boot/config-$(KERNEL_VER) ]; then | |
| USE_LLVM=$(grep -q "CONFIG_CC_IS_CLANG=y" /boot/config-$(KERNEL_VER) 2>/dev/null && echo -n "LLVM=1"); | |
| elif [ -e /boot/config-$(uname -r) ]; then | |
| USE_LLVM=$(grep -q "CONFIG_CC_IS_CLANG=y" /boot/config-$(uname -r) 2>/dev/null && echo -n "LLVM=1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, it's right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
bash -cwithechoand>>redirection is unnecessarily complex and potentially error-prone. A more robust approach would be to usetee -aor create a dedicated script. Additionally, directly appending to/etc/security/limits.confwithout checking for existing entries could create duplicates on repeated runs. Consider:echo "* soft memlock unlimited" | sudo tee -a /etc/security/limits.confor add a check to prevent duplicates.