Add Hailo-8 firmware for Raspberry Pi AI Kit/HAT on RPi 5 #3680
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce support for the Hailo8 firmware in the buildroot configuration for Raspberry Pi 5. This includes adding a configuration source for the firmware, defining a new package, and creating a Makefile that manages the firmware's build and installation processes. A SHA256 hash for the firmware binary is also included to ensure its integrity. Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
buildroot-external/package/hailo8-firmware/Config.in (1)
3-6: Consider enhancing the help text documentationWhile the current help text provides basic information, consider adding:
- Firmware version or source information
- Specific kernel version requirements
- Expected installation path of the firmware file
- License information for the firmware
Example enhancement:
help Firmware for Hailo-8 PCIe device found e.g. on Raspberry Pi AI Kit and Raspberry Pi AI HAT+. Requires the hailo kernel driver - (included in Raspberry Pi's downstream kernel). + (included in Raspberry Pi's downstream kernel >= X.Y). + + The firmware file (hailo8_fw.bin) will be installed to + /lib/firmware/hailo and is distributed under the license XXX. + Version: X.Y.Z (sourced from RPi kernel git history)buildroot-external/package/hailo8-firmware/hailo8-firmware.mk (2)
12-14: Consider adding integrity verification during extractionWhile the copy operation is correct, consider adding SHA256 verification of the firmware file during extraction for additional security.
define HAILO8_FIRMWARE_EXTRACT_CMDS + echo "$(sha256sum $(HAILO8_FIRMWARE_DL_DIR)/$(HAILO8_FIRMWARE_SOURCE))" | \ + grep -q "^$(shell cat $(HAILO8_FIRMWARE_PKGDIR)/hailo8-firmware.hash)" cp $(HAILO8_FIRMWARE_DL_DIR)/$(HAILO8_FIRMWARE_SOURCE) $(@D) endef
20-23: LGTM! Installation follows firmware best practicesThe installation commands correctly:
- Create a vendor-specific firmware directory
- Set appropriate read-only permissions (0644)
- Use the standard
/lib/firmwarepath hierarchyThis should resolve the firmware persistence issue mentioned in #3663.
Consider adding a post-install verification step in the OS startup scripts to confirm firmware presence and permissions, helping diagnose any potential installation issues.
buildroot-external/configs/rpi5_64_defconfig (1)
Line range hint
1-146: Architectural consideration: Ensure firmware persistence with EROFS.The configuration uses EROFS with compression and deduplication enabled. This setup, combined with the following relevant settings, should properly handle firmware persistence:
BR2_TARGET_ROOTFS_EROFS=yBR2_TARGET_ROOTFS_EROFS_LZ4HC_LEVEL=12BR2_TARGET_ROOTFS_EROFS_DEDUPE=yTo ensure the firmware persists across reboots (addressing issue #3663), verify that:
- The firmware is installed to the correct path in the EROFS image
- The firmware is included in the initial ramdisk if needed before root filesystem mount
- The firmware path matches the driver's expected location
Would you like me to generate a script to verify these aspects in the build configuration?
Also applies to: 146-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
buildroot-external/Config.in(1 hunks)buildroot-external/configs/rpi5_64_defconfig(1 hunks)buildroot-external/package/hailo8-firmware/Config.in(1 hunks)buildroot-external/package/hailo8-firmware/hailo8-firmware.hash(1 hunks)buildroot-external/package/hailo8-firmware/hailo8-firmware.mk(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- buildroot-external/package/hailo8-firmware/hailo8-firmware.hash
🧰 Additional context used
📓 Path-based instructions (1)
buildroot-external/Config.in (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (8)
buildroot-external/package/hailo8-firmware/Config.in (2)
1-2: LGTM! Configuration follows buildroot conventions
The package configuration name and type are correctly defined following buildroot naming conventions.
1-6: Verify firmware installation path and persistence
Given issue #3663 where the firmware file wasn't found during boot and manual uploads didn't persist, we should verify the package installation configuration.
✅ Verification successful
Let me analyze the firmware installation configuration from the output. I can see that:
- The package has a proper .mk file defining installation paths:
define HAILO8_FIRMWARE_INSTALL_TARGET_CMDS
$(INSTALL) -d $(TARGET_DIR)/lib/firmware/hailo
$(INSTALL) -m 0644 $(@D)/hailo8_fw.bin $(TARGET_DIR)/lib/firmware/hailo/
endef-
The firmware is installed to
/lib/firmware/hailo/which is a standard firmware location in Linux systems. -
No cleanup scripts were found that would affect firmware persistence (the search for cleanup scripts returned no results).
-
The installation is done through Buildroot's standard package installation mechanism which ensures files are properly installed into the target filesystem.
-
The firmware will be included in the root filesystem and persist across reboots since it's installed to a standard system location.
Firmware installation path and persistence verified
The firmware will be installed to the correct system path /lib/firmware/hailo/ and will persist across reboots since:
- It uses standard firmware installation paths
- Installation is handled by Buildroot's package system
- No cleanup scripts were found that would remove it
- The target directory is part of the root filesystem
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the firmware installation configuration
# Check if there's a corresponding .mk file defining the installation path
echo "Checking for package makefile..."
find . -name "hailo8-firmware.mk"
# Check if other firmware packages use similar paths
echo -e "\nChecking firmware installation paths in other packages..."
rg -A 3 "$(FIRMWARE_DIR)" "buildroot-external/package/"
# Check for firmware cleanup scripts that might affect persistence
echo -e "\nChecking for firmware cleanup scripts..."
rg -l "/lib/firmware.*clean" "buildroot-external/"
Length of output: 350559
buildroot-external/package/hailo8-firmware/hailo8-firmware.mk (4)
16-18: LGTM! Filename matches driver expectations
The build command correctly renames the firmware to hailo8_fw.bin, which matches the filename expected by the driver as mentioned in issue #3663.
25-25: LGTM! Correct package infrastructure used
The use of generic-package is appropriate for binary-only firmware packages.
9-10: Verify S3 bucket accessibility and stability
The firmware is sourced from an AWS S3 bucket. Please ensure:
- The bucket is publicly accessible
- The URL is stable and won't change
- There's a backup source in case the S3 bucket becomes unavailable
Let's verify the URL accessibility:
#!/bin/bash
# Verify URL accessibility
curl -I "${HAILO8_FIRMWARE_SITE}/${HAILO8_FIRMWARE_SOURCE}"7-8: Version determination approach differs from PR objectives
The PR objectives mention that the firmware version should be determined from the Git history of the kernel sources. However, the version is currently hardcoded to 4.18.0.
Let's verify if this version matches the kernel sources:
buildroot-external/Config.in (1)
5-5: LGTM! The change follows buildroot conventions.
The addition maintains consistent formatting and correct alphabetical ordering.
Let's verify the existence of the referenced configuration file:
✅ Verification successful
✓ Config.in file exists and is properly configured
The referenced configuration file exists and contains the correct package configuration for Hailo-8 firmware. The file includes:
- Proper package naming convention (BR2_PACKAGE_HAILO8_FIRMWARE)
- Clear description and help text
- Correct dependency information for Raspberry Pi AI Kit/HAT
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the Hailo8 firmware configuration file
# Expected: The file should exist and contain the firmware package configuration
# Check if the Config.in file exists
fd -t f "Config.in" package/hailo8-firmware
# If found, display its contents to verify the package configuration
fd -t f "Config.in" package/hailo8-firmware -x cat
Length of output: 316
Script:
#!/bin/bash
# Let's verify from the repository root with correct path
fd -t f "Config.in" buildroot-external/package/hailo8-firmware
# If found, display its contents
fd -t f "Config.in" buildroot-external/package/hailo8-firmware -x cat
# Let's also check if the directory exists
ls -la buildroot-external/package/hailo8-firmware 2>/dev/null || echo "Directory not found"
Length of output: 794
buildroot-external/configs/rpi5_64_defconfig (1)
146-146: LGTM! Verify firmware persistence across reboots.
The addition of BR2_PACKAGE_HAILO8_FIRMWARE=y is correctly positioned and integrated with the existing firmware configuration. The EROFS configuration ensures proper firmware handling.
Let's verify the firmware package configuration:
✅ Verification successful
Firmware package configuration is correct and well-structured
The verification shows that the Hailo-8 firmware package is properly configured:
- Config.in correctly defines the package with appropriate help text
- Makefile properly handles firmware installation to
/lib/firmware/hailo/hailo8_fw.bin - SHA256 hash is present for firmware integrity verification
- Version 4.18.0 firmware is sourced from official Hailo repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Hailo-8 firmware package configuration and dependencies
# Check if the package configuration exists
if ! fd -t f "Config.in$" | xargs grep -l "BR2_PACKAGE_HAILO8_FIRMWARE"; then
echo "Error: Missing Config.in file for Hailo-8 firmware package"
exit 1
fi
# Verify firmware installation path in package makefile
if ! fd -t f "hailo8-firmware.mk$" | xargs grep -l "\/lib\/firmware\/hailo8_fw.bin"; then
echo "Error: Firmware installation path might be incorrect"
exit 1
fi
# Check for firmware hash file
if ! fd -t f "hailo8-firmware.hash$"; then
echo "Error: Missing hash file for firmware verification"
exit 1
fi
Length of output: 324
Script:
#!/bin/bash
# Let's examine the actual content of these files to verify the configuration
# Check Config.in content
echo "=== Config.in content ==="
fd -t f "Config.in$" -x cat {} \; | grep -A 5 "config BR2_PACKAGE_HAILO8_FIRMWARE"
# Check makefile content
echo -e "\n=== Makefile content ==="
fd -t f "hailo8-firmware.mk$" -x cat {}
# Check hash file content
echo -e "\n=== Hash file content ==="
fd -t f "hailo8-firmware.hash$" -x cat {}
Length of output: 1547
Add Hailo-8 firmware binary for Rasperry Pi AI accelerators. The version needs to be determined from the Git history of the kernel sources, as the driver source code is included in the RPi downstream kernel and the version string can't be found in the code directly. Fixes #3663
sairon
force-pushed
the
add-hailo8-firmware
branch
from
74e2f56 to
724ee1d
Compare
|
FTR, I was wrong the version is not defined in the sources, it is in |
|
Is this a pre-cursor to native Frigate support or any other projects that can leverage the Hailo-8? |
|
@drkpxl Frigate already supports it but there are still some issues that need to be addressed for seamless usage in HAOS, see here: blakeblackshear/frigate#15172 |
Add Hailo-8 firmware binary for Rasperry Pi AI accelerators. The version needs to be determined from the Git history of the kernel sources, as the driver source code is included in the RPi downstream kernel and the version string can't be found in the code directly.
Fixes #3663
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores