Conversation
|
The builds fail due to the issue addressed in #401 |
There was a problem hiding this comment.
Pull request overview
Adds support for NVIDIA JetPack 5.1.6 and 6.2.2 across the workspace scripts and kernel patchsets, with CI workflows to validate builds.
Changes:
- Extend setup/sync scripts to recognize JP 5.1.6 and 6.2.2 (and prefer
JP_INPUT_VERSIONsources files). - Add JP-version mapping “pointer” files and introduce new kernel patch directories for 5.1.6 / 6.2.2.
- Add GitHub Actions workflows to build JP 5.1.6 and JP 6.2.2 in CI.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/sync-sources.sh | Prefer sources_${JP_INPUT_VERSION} over sources_${JETPACK_VERSION} when present |
| scripts/setup-common | Add JP 6.2.2 / 5.1.6 support and adjust -h behavior |
| nvidia-oot/6.2.2 | Map JP 6.2.2 OOT patchset to 6.2.1 baseline |
| kernel/kernel-5.10/5.1.6 | Map JP 5.1.6 kernel baseline to 5.1.2 |
| kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch | Kernel driver patchset (ported) used for JP 5.1.6 |
| kernel/nvidia/5.1.6/0002-nvidia-vi5-prevent-high-cpu-usage.patch | VI5 CPU usage fix included for JP 5.1.6 |
| kernel/nvidia/5.1.6/0003-vi-channel-verify-s-g-callbacks.patch | Reuse 5.1.2 patch for JP 5.1.6 |
| kernel/nvidia/5.1.6/0004-Adding-stream-restart-upon-capture-timeout.patch | Add VI stream restart-on-timeout logic for JP 5.1.6 |
| kernel/nvidia/5.1.6/0005-Add-GPIO-tunneling-for-the-external-sync-support.patch | Reuse 5.1.2 patch for JP 5.1.6 |
| kernel/nvidia/5.1.6/0006-Adding-max96712-support-for-D4xx.patch | Reuse 5.1.2 patch for JP 5.1.6 |
| kernel/kernel-jammy-src/6.2.2/0001-kernel-enable-HID_SENSOR-without-USB-impact.patch | Reuse 6.2.1 patch for JP 6.2.2 |
| kernel/kernel-jammy-src/6.2.2/0002-realsense-camera-formats-jammy-master.patch | Reuse 6.2.1 patch for JP 6.2.2 |
| kernel/kernel-jammy-src/6.2.2/0003-realsense-metadata-jammy-master.patch | Reuse 6.2.1 patch for JP 6.2.2 |
| kernel/kernel-jammy-src/6.2.2/0004-realsense-powerlinefrequency-control-fix-jammy.patch | Reuse 6.2.1 patch for JP 6.2.2 |
| kernel/kernel-jammy-src/6.2.2/0005-support-for-dynamic-change-of-i2c-bus-clk-rate.patch | Add dynamic I2C bus clock rate control for JP 6.2.2 |
| .github/workflows/build-jp5.1.6.yml | CI workflow for building JP 5.1.6 |
| .github/workflows/build-jp6.2.2.yml | CI workflow for building JP 6.2.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (4)
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
- Using bitwise-OR to accumulate error codes can corrupt the original errno (e.g., combining negative values) and makes it hard to return the first/most relevant failure. Prefer returning immediately on first error (or preserving the first negative errno) rather than
err |= ....
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
- head/tail/num_buffers are reset outside the spinlock even though they are part of the same protected ring-buffer state used elsewhere. This introduces a race with queueing/dequeueing metadata buffers. Reset these fields under the same spinlock (and remove the extra semicolon on
tail = 0;;).
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/kernel-jammy-src/6.2.2/0005-support-for-dynamic-change-of-i2c-bus-clk-rate.patch:1
- bus_clk_rate is stored as
unsigned longin struct i2c_adapter, but sysfs show uses%ldand store parses into anint(memparse returns an unsigned long), which can truncate/overflow and mis-display values. Also, the sysfs store updatesadap->bus_clk_ratewithout taking the i2c bus lock (unlike i2c_set_adapter_bus_clk_rate), which can race with active transfers. Use sysfs_emit()/%lu, parse into unsigned long, validate range, and update under i2c_lock_bus (or call the new helper) to make the change thread-safe.
From ce6faaedef68204e8b132bf44a0e4d2432dfb7ad Mon Sep 17 00:00:00 2001
kernel/kernel-jammy-src/6.2.2/0005-support-for-dynamic-change-of-i2c-bus-clk-rate.patch:1
- bus_clk_rate is stored as
unsigned longin struct i2c_adapter, but sysfs show uses%ldand store parses into anint(memparse returns an unsigned long), which can truncate/overflow and mis-display values. Also, the sysfs store updatesadap->bus_clk_ratewithout taking the i2c bus lock (unlike i2c_set_adapter_bus_clk_rate), which can race with active transfers. Use sysfs_emit()/%lu, parse into unsigned long, validate range, and update under i2c_lock_bus (or call the new helper) to make the change thread-safe.
From ce6faaedef68204e8b132bf44a0e4d2432dfb7ad Mon Sep 17 00:00:00 2001
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
….2.patch Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
….2.patch Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
a52d725 to
de4b9c2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (7)
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
vb2_buffer_done()is invoked while holdingchan->embedded.spin_lock. Buffer completion can trigger callbacks/locking that is not safe under a spinlock in many vb2 paths. Move thevb2_buffer_done()call outside the spinlocked section (e.g., decide under lock, then complete after unlock).
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
vb2_buffer_done()is called underspin_lockhere as well, which has the same locking-safety concern as integra_metadata_buffer_queue. Consider collecting the buffers to complete into a temporary list/array while holding the lock, then releasing the spinlock and completing them afterwards.
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
- Both
S_FMTandTRY_FMTreturn success without validatingformator normalizing fields (e.g.,dataformatandbuffersize). This can cause user space to believe an unsupported meta format/buffersize is accepted, while the queue later rejects it (or worse, accepts inconsistent state). At minimum, enforceV4L2_META_FMT_D4XXand a fixed/validatedbuffersizehere and return-EINVALfor unsupported requests.
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
intervalis stack-allocated and passed tog_frame_intervalwithout initializing required fields (notablyinterval.pad, which you do set in the correspondings_frame_intervalpath). This can lead to undefined behavior or EINVAL depending on the subdev implementation. Initializeinterval(e.g., zero it and set.padconsistently) and consider mapping-ENOIOCTLCMDto-ENOTTYas is done elsewhere in this file.
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
- The code copies 255 bytes into the metadata buffer but sets the vb2 plane payload to 68 bytes. This inconsistency will cause user space to only see 68 bytes as valid (or suggests the copy size is wrong). Align the payload size with the actual produced metadata size (either copy 68, or set payload to 255, or compute it dynamically).
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
- Returning
-1is non-standard for kernel code and loses errno semantics for callers/diagnostics. Use an appropriate negative errno (e.g.,-ENODEVwhen the CSI subdev can’t be found) so error handling remains consistent.
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
kernel/nvidia/5.1.6/0001-Porting-driver-patches-to-jetpack-5.1.2.patch:1
- Changing
TEGRA_STRIDE_ALIGNMENTfrom 1 to 64 is a global behavior change that will affect buffer layout/stride for all formats and all users of this header, not just D4xx-specific paths. If this is only required for certain modes/devices, consider making the alignment conditional (per-format/per-device) or document why the global change is safe; otherwise it can cause unexpected size/stride differences in existing deployments.
From 1b06cf3323a3d7d1437071dc3789e9487b9b5733 Mon Sep 17 00:00:00 2001
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scripts/config --file $TEGRA_KERNEL_OUT/.config --enable DYNAMIC_DEBUG | ||
| fi | ||
| make O=$TEGRA_KERNEL_OUT -j${NPROC} | ||
| echo WOJTEK: make O=$TEGRA_KERNEL_OUT modules_install INSTALL_MOD_PATH=$KERNEL_MODULES_OUT |
There was a problem hiding this comment.
Looks like debug prints left by mistake
| make O=$TEGRA_KERNEL_OUT -j${NPROC} | ||
| echo WOJTEK: make O=$TEGRA_KERNEL_OUT modules_install INSTALL_MOD_PATH=$KERNEL_MODULES_OUT | ||
| make O=$TEGRA_KERNEL_OUT modules_install INSTALL_MOD_PATH=$KERNEL_MODULES_OUT | ||
| echo WOJTEK: done |
There was a problem hiding this comment.
Looks like debug prints left by mistake
ejgoldik
left a comment
There was a problem hiding this comment.
Just remove the debug prints in build_all (Or make them official-looking if you think they should remain)
Add support for Jetpack versions:
5.1.6
6.2.2