Fix/hw reset gmsl recovery - #384
Merged
Merged
Conversation
Fix two HW reset issues: 1. [D457 GMSL] No device found after HW Reset - caused by chip-wide deserializer reset_oneshot disrupting sibling camera links 2. [D401 GMSL] Fail to stream after HW reset during active streaming - caused by stale sensor->streaming flags blocking subsequent stream starts Changes: - Stop active streams before sending HW reset command to ensure clean state - Clear sensor->streaming flags and release SERDES pipes eagerly during reset (uncomment and enhance the previously disabled invalidation block) - Replace chip-wide reset_oneshot with tiered SERDES recovery: Phase 1: serializer-only re-init (per-camera, non-disruptive) Phase 2: full deserializer reset only when all siblings are also dead, with I2C health check gate to protect actively streaming cameras - Replace global ds5_reset_gen counter with per-deserializer counters to prevent cross-camera state invalidation across independent GMSL links - Relocate serdes_inited[] and MAX_DEV_NUM earlier for visibility Fixes: RSDSO-21151, RSDSO-21257, RSDSO-21254
Each physical D4XX camera has 4 driver instances (Depth, RGB, IR, IMU) sharing the same MAX9295 serializer (ser_dev). The initial HW reset recovery implementation treated all instances on the same deserializer as siblings, which caused incorrect behavior: 1. Same-camera instances (same ser_dev) were treated as 'siblings with active streams' in Phase 2, blocking deserializer reset even when no true sibling camera was streaming. 2. The streaming flag check examined all 4 sensor types per instance, but each instance only manages ONE sensor type (via is_depth/is_rgb/ is_y8/is_imu flags). 3. Steps 1 and 2 (stop streams, invalidate state) only operated on the current instance, leaving peer instances of the same physical camera with stale streaming flags and SERDES pipe state after HW reset. Fix by: - Distinguishing same-camera peers (same ser_dev) from true siblings (different ser_dev, same dser_dev) in SERDES recovery Phase 2 - Using ds5_get_active_sensor() to check only the managed sensor type per instance instead of all 4 sensor structs - Iterating serdes_inited[] to stop streams and invalidate state on all peer instances of the same physical camera in Steps 1 and 2 - Using __maybe_unused for loop variable only needed in SERDES path JIRA: RSDSO-21151, RSDSO-21257, RSDSO-21254
…loop Two bugs found via post-HW-reset log analysis: Bug A: Only the first stream instance (Depth) per physical camera was registered in serdes_inited[]. RGB, Y8, and IMU instances returned -ENOTSUPP from ds5_board_setup() and were never registered. This made them invisible to peer iteration in ds5_hw_reset_with_recovery() steps 1 and 2, so their streaming flags and SERDES pipe state were never cleared after HW reset. Fix: Register ALL instances in serdes_inited[], even when they share the same serializer. The -ENOTSUPP still prevents duplicate SERDES setup but the instance is now registered before returning. Add a serdes_primary flag to struct ds5 so that only the primary instance tears down the serializer/deserializer on ds5_remove(). Bug B: After HW reset, all FW streams return to idle. When the VI capture engine detects frame loss and initiates error recovery, it calls ds5_mux_s_stream(off) to stop the stream. The pre-condition check at the top of ds5_mux_s_stream() sees the stream is already stopped (status 0x0000) and returns -EBUSY, which causes VI to fail the restart and enter an infinite retry loop. Fix: Treat stream-already-in-target-state as a no-op (return 0) instead of -EBUSY, allowing VI error recovery to proceed. Observed in log: d4xx 9-001b (RGB) stuck in endless 'stream 1 in 0 state already, return busy' loop after d4xx 9-001a (Depth) triggered HW reset. JIRA: RSDSO-21151, RSDSO-21257, RSDSO-21254
There was a problem hiding this comment.
Pull request overview
Updates the D4XX kernel driver’s GMSL/SERDES reset behavior to improve recovery after a hardware reset, especially in multi-camera / multi-instance (Depth/RGB/IR/IMU) deployments.
Changes:
- Introduces per-deserializer reset generation tracking (instead of a single global counter) to avoid cross-deserializer state invalidation.
- Adds a tiered SERDES recovery flow after HW reset (serializer-only first; full deserializer reset only when it won’t disrupt streaming siblings).
- Improves multi-instance bookkeeping (
serdes_inited[],serdes_primary) and changesds5_mux_s_stream()to treat “already in requested state” as a no-op (avoid EBUSY loops).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The 4th probe instance (IMU at 9-001d) fails to communicate with the D457 camera at the second I2C check in ds5_probe(), even though the first 3 instances succeed. Root cause analysis of the dmesg logs shows the first I2C check (with retries) passes, but the second check (single ds5_read, no outer retry) fails with -121 (EREMOTEIO / NAK). The D457 firmware has a brief I2C-unresponsive window during post-HW- reset initialization that the D401 does not exhibit. By the time the 4th instance reaches the second I2C check (~50ms after HW reset completion), the camera is momentarily unreachable. Three bugs fixed: 1. Second I2C check had no retry loop: replaced single ds5_read() with a retry loop (10 iterations, 50ms delay between retries) matching the pattern used by the first communication check. 2. Spurious pipe 0 release at probe: sensor pipe_ids default to 0 (kzalloc) instead of PIPE_NOT_CONFIGURED (-1). When hw_reset_with_recovery runs during probe (before ds5_sensor_init), step 2 sees pipe_id=0 >= 0 and releases pipe 0 from the MAX9296. Fix: initialize all four sensor pipe_ids to PIPE_NOT_CONFIGURED before calling hw_reset_with_recovery. 3. No stabilization delay after probe HW reset: added msleep(100) after hw_reset_with_recovery returns, giving the D457 firmware time to complete post-reset initialization before subsequent instances attempt I2C communication. Tested against logs: v1.0.2.16 (0/4 instances, old full SERDES re-init killed GMSL link), v1.0.2.18 pre-fix (3/4 instances, IMU fails at 2nd I2C check).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced Mar 2, 2026
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced Mar 2, 2026
This comment was marked as outdated.
This comment was marked as outdated.
Nikolai-L
force-pushed
the
fix/hw-reset-gmsl-recovery
branch
from
March 2, 2026 15:34
fd8f06c to
4254890
Compare
RSDSO-21151 After HW reset, the driver returned success to userspace ~54ms before the firmware's HWMC subsystem was ready for commands. RS Viewer immediately queried GVD/HWMC, hitting I2C EREMOTEIO (-121) failures and WIP/ERR status codes for seconds, eventually crashing. Root causes and fixes: 1) ds5_hw_reset_with_recovery: Add Step 9 HWMC readiness check. After device type is confirmed (Step 8), send a lightweight GVD command and poll HWMC_STATUS until completion before returning to userspace. This blocks the caller until the FW command processor is fully operational, preventing the race window. 2) ds5_gvd: Fix retry loop condition. The old condition 'ret && retries-- && status != 0' only retried on I2C failure. If I2C succeeded but status was WIP (2), ret==0 caused immediate loop exit. Changed to '(ret || status == WIP) && retries--' to retry on both I2C errors and WIP. Increased retries from 3x10ms to 20x50ms for transient post-reset conditions. 3) ds5_gvd: Fix return value. Previously returned raw HWMC status (positive int, e.g. 2 for WIP) on failure. V4L2 g_volatile_ctrl interprets positive returns as success, so userspace received garbage GVD data. Now returns -EIO on failure. 4) ds5_get_hwmc_status: Add I2C retry tolerance. The loop exited immediately on I2C failure (!ret was false). Changed condition to continue retrying on both I2C failures and WIP status, with a dev_dbg trace for I2C errors. Signed-off-by: GitHub Copilot <copilot@github.com>
…er HW reset RSDSO-21257 During probe, the first driver instance resets the camera via ds5_hw_reset_with_recovery(). Step 8 polls DS5_DEVICE_TYPE until it returns a non-zero value, but stores the result in a local variable only. All four probe instances then call ds5_fixed_configuration() which reads DS5_DEVICE_TYPE independently -- and at that point the register still returns 0 because the firmware hasn't finished re-populating it. When dev_type is 0, the switch falls to 'default' which selected D46X format tables (only 2 depth resolutions: 1280x960, 640x480). D401 needs 7 resolutions and D457 needs 8, including 1280x720 (HD Depth). This caused RS Viewer to be unable to select HD Depth mode: switching to HD silently reverted to IR or jumped to another resolution, with no error messages. This was not seen in 1.0.2.18 because probe-time HW reset was introduced in 1.0.2.19 (commit 4e8e62c). Fixes: 1) Cache device type from Step 8: when the poll succeeds, store the confirmed value in a static ds5_cached_device_type variable (following the ds5_probe_reset_once pattern for cross-instance sharing). 2) Use cached value in ds5_fixed_configuration(): if the register read returns 0 but the cache is valid, use the cache. Logs a dev_info when using the cached value. 3) Use cached value in ds5_adjust_sync_mode_control(): same pattern, prevents the 'Unknown device type 0, disabling sync mode' warning. 4) Change default depth format fallback from D46X (2 resolutions) to D43X (8 resolutions). D43X is the most capable table and a safer fallback if device type is truly unknown. 5) Promote Step 8 success message from dev_dbg to dev_info for visibility in production dmesg. Signed-off-by: GitHub Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Nikolai-L
force-pushed
the
fix/hw-reset-gmsl-recovery
branch
from
March 10, 2026 15:47
9f5a811 to
68083ec
Compare
sareluzi
previously requested changes
Mar 15, 2026
ejgoldik
reviewed
Mar 15, 2026
ejgoldik
reviewed
Mar 15, 2026
ejgoldik
reviewed
Mar 15, 2026
ejgoldik
requested changes
Mar 15, 2026
ejgoldik
left a comment
ejgoldik
left a comment
Contributor
There was a problem hiding this comment.
A few very small comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Nikolai-L
force-pushed
the
fix/hw-reset-gmsl-recovery
branch
from
March 15, 2026 14:34
9d30c3a to
2313f86
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Nikolai-L
force-pushed
the
fix/hw-reset-gmsl-recovery
branch
from
March 15, 2026 14:58
2313f86 to
f065b0a
Compare
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Fixes robustness of the hardware-reset / GMSL (SerDes) recovery path in the D4XX kernel driver and updates associated documentation and helper tooling. kernel: Improve GMSL HW-reset recovery and sibling invalidation in d4xx.c. Refactor: Encapsulate shared state (struct ds5_dev, struct dser_control) and switch to lazy reset-generation invalidation; add multi-process fine-tuning to improve concurrent stream/recovery behavior. Docs: Add follow-up notes and guidance in hw-reset-followup-plan.md. Tooling: Update build script (build_all.sh) and add generate_compile_commands.sh. Repo config: Add editor settings and CI helper files (copilot-instructions.md, settings.json, c_cpp_properties.json). Misc: Update CLAUDE.md and .gitignore.
Nikolai-L
force-pushed
the
fix/hw-reset-gmsl-recovery
branch
from
March 15, 2026 15:26
32795fe to
44dd50d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ejgoldik
approved these changes
Mar 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. New data structures:
ds5_devanddser_controlPreviously, per-camera and per-deserializer state (
reset_gen,cached_device_type,last_reset_jiffies) was stored in scattered global variables. Now it's grouped into two proper structs:dser_control— per-deserializer: ownreset_gen,dser_devpointerds5_dev— per-camera: ownreset_gen,cached_device_type,last_reset_jiffies,dser_control*,ds5_primary*, and streaming flagsEach
ds5instance gets a->ds5_devpointer to its camera's shared state, and eachds5_devlinks to itsdser_control.2. Dual reset-generation tracking
state->reset_genis split intoreset_ref_ds5(camera-level) andreset_ref_dser(deserializer-level).ds5_mux_s_stream()now checks both: a mismatch on either triggers invalidation. This lets camera reset and deserializer reset be independent events, whereas before they were conflated into a single counter.3.
ds5_setup_and_link()— deduplicated registrationThe duplicated
serdes_inited[]registration logic in both#ifdef CONFIG_OFand#elsebranches ofds5_board_setup()is extracted into oneds5_setup_and_link()function. Also handles deserializer linking in the same pass.4. Eliminated
serdes_inited[]/MAX_DEV_NUM(24)Replaced by
ds5_inited[MAX_DS5_NUM](8 entries) anddser_inited[MAX_DSER_NUM](4 entries). The old 24-entry flat array mixed camera instances and counted every V4L2 subdevice; the new arrays count physical cameras and deserializers.5. Simplified peer iteration
All loops that previously scanned 24
serdes_inited[]entries with multi-condition filtering (same dser? different ser? skip self?) are replaced by shorter loops overds5_inited[]. Primary instance lookup is now O(1) viastate->ds5_dev->ds5_primary.6. Lazy invalidation via
atomic_increplaces explicit peer loopsds5_hw_reset_serdes_recovery()Phase 2: the 20-line loop that explicitly invalidated all cameras on the deserializer is replaced byatomic_inc(dser_get_reset_gen(state)). Peers detect the bump lazily inds5_configure().ds5_hw_reset_with_recovery()Step 2: the explicit peer invalidation loop is replaced byatomic_inc(ds5_get_reset_gen(state)).7.
ds5_dev_type()helperThe cached-device-type fallback pattern (check register value, substitute cached if 0) was duplicated in
ds5_fixed_configuration()andds5_adjust_sync_mode_control(). Now a single inline function.8. Timing change: generation bump moved before HW reset command
Old: explicit invalidation → HW reset →
atomic_inc. New:atomic_inc→ HW reset. This is slightly safer — eliminates a window where a peer could re-configure between explicit invalidation and the eventualatomic_inc.Advantages (weighted)
# | Advantage | Weight -- | -- | -- 1 | Structural clarity — per-camera/per-deserializer state is encapsulated, not scattered in globals | High 2 | ~120 fewer lines with no loss of functionality intent | High 3 | O(1) primary lookup — direct pointer vs. linear search through 24 entries | Medium 4 | Deduplicated registration — one ds5_setup_and_link() vs. two copies | Medium 5 | Dual reset-gen — camera and deserializer resets are independent signals | Medium 6 | Smaller iteration arrays — 8+4 entries vs. 24, less wasted scanning | Low 7 | ds5_dev_type() helper — eliminates repeated 7-line fallback blocks | Low