Tracked-on RSDSO 21082: [D457 GSML][Regression] No frame received after several iterations streaming start/stop with pipeline. - #356
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the RealSense d4xx kernel driver (and related deployment artifacts) to improve robustness during rapid stream start/stop cycles on GMSL SERDES links, addressing a regression where frames stop arriving after repeated iterations.
Changes:
- Add I2C retry logic around regmap reads/writes and tighten/start/stop polling behavior in
kernel/realsense/d4xx.c. - Serialize SERDES pipe allocation/config/cleanup to avoid MAX9295/MAX9296 contention during concurrent operations.
- Adjust deployment script TARGET gating and clean up an out-of-tree patch header content.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| scripts/deploy_kernel.sh | Changes the “skip copy/reboot” gating condition to check TARGET and exits early when unset. |
| nvidia-oot/6.0/0004-Add-GPIO-tunneling-for-the-external-sync-support.patch | Removes .gitignore changes from the patch, updating file-change metadata accordingly. |
| kernel/realsense/d4xx.c | Adds I2C retry and new start/stop sequencing/locking to reduce I2C NACKs and start/stop instability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #endif | ||
|
|
||
| ret = ds5_configure(state); | ||
| #ifdef CONFIG_VIDEO_D4XX_SERDES | ||
| mutex_unlock(&serdes_lock__); |
There was a problem hiding this comment.
The mutex_lock(&serdes_lock__) is unconditional here, but the corresponding mutex_unlock(&serdes_lock__) is guarded by #ifdef CONFIG_VIDEO_D4XX_SERDES. If CONFIG_VIDEO_D4XX_SERDES is not enabled (or if the surrounding #ifdef boundaries differ from what’s assumed), this can lead to an unmatched lock/unlock and potential deadlock. Make the lock/unlock consistently compiled under the same #ifdef (or consistently unconditional), ensuring all paths release the mutex exactly once.
| #endif | |
| ret = ds5_configure(state); | |
| #ifdef CONFIG_VIDEO_D4XX_SERDES | |
| mutex_unlock(&serdes_lock__); | |
| ret = ds5_configure(state); | |
| mutex_unlock(&serdes_lock__); | |
| #else | |
| ret = ds5_configure(state); |
| if (attempt == 0) { | ||
| /* | ||
| * First attempt timed out. Stop the stream, | ||
| * wait for FW to confirm idle, then retry. |
There was a problem hiding this comment.
The comment says “with two retries”, but the loop is attempt < 3 while the retry/stop-start logic only runs for attempt == 0. This is confusing to future readers and makes it unclear how many retries are intended. Either adjust the loop/conditions to match the intended retry count (e.g., a single retry would typically be attempt < 2), or update the comment and control flow so behavior and documentation align.
| * wait for FW to confirm idle, then retry. | |
| * wait for FW to confirm idle, then resend | |
| * the start command for the next attempt. | |
| * The outer loop may perform an additional | |
| * (passive) retry, but we only tear down / | |
| * restart the stream once to avoid churn. |
| /* Fail fast on firmware config errors */ | ||
| if (status & (DS5_STATUS_INVALID_DT | | ||
| DS5_STATUS_INVALID_RES | | ||
| DS5_STATUS_INVALID_FPS)) { | ||
| dev_err(&state->client->dev, | ||
| "start: FW rejected config, status 0x%04x\n", | ||
| status); | ||
| i = DS5_START_MAX_COUNT; | ||
| break; | ||
| } |
There was a problem hiding this comment.
On firmware config rejection, the code sets i = DS5_START_MAX_COUNT and breaks, but then the outer logic can still enter the timeout “retrying” path and attempt stop/start again. Retrying a start after FW explicitly rejected the configuration is unlikely to recover and can add extra bus traffic during an error state. Consider returning a hard error immediately (e.g., -EINVAL) when invalid config flags are seen, and avoid entering the retry block in that case. Also, the ds5_write(... STOP ...) return value is ignored here; if the stop command fails, the subsequent “wait for idle” and re-start can behave incorrectly—capture ret and bail out on failure.
| dev_warn(&state->client->dev, | ||
| "start timeout (status 0x%04x, stream 0x%04x), retrying\n", | ||
| status, streaming); | ||
| ds5_write(state, DS5_START_STOP_STREAM, | ||
| DS5_STREAM_STOP | stream_id); |
There was a problem hiding this comment.
On firmware config rejection, the code sets i = DS5_START_MAX_COUNT and breaks, but then the outer logic can still enter the timeout “retrying” path and attempt stop/start again. Retrying a start after FW explicitly rejected the configuration is unlikely to recover and can add extra bus traffic during an error state. Consider returning a hard error immediately (e.g., -EINVAL) when invalid config flags are seen, and avoid entering the retry block in that case. Also, the ds5_write(... STOP ...) return value is ignored here; if the stop command fails, the subsequent “wait for idle” and re-start can behave incorrectly—capture ret and bail out on failure.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…uild helper Tested 2026-04-28 with the post-realsenseai#11 driver: setting force_clk0; on the dser node makes 0x08A0 = 0x24 (FORCE_CLK0_HS bit 5 set) which wedges MAX96712 forward I2C the moment v4l2-ctl --stream-mmap fires; the chip becomes unreachable on its primary I2C bus until a DC power cycle. Same failure class as the previously documented "FORCE_CLK 0x81 harmful" finding from the pre-PR#404 era; the offending bit is 5 (FORCE_CLK0_HS), not just specific full-byte values. Replace the experimental force_clk0; line with a multi-line comment that warns future readers off the property. The avermedia overlay in this tree sets force_clk0; — that pattern does NOT carry over to LI-JAG-ADP-GMSL2-8CH. Also adds scripts/lijag_rebuild_dtbo.sh so iterating on this DT overlay no longer requires running build_stage1_oot.sh (which rebuilds max9296.ko it doesn't need): preprocess + dtc + cp into /boot/, then reboot. A research pass on realsenseai PRs (realsenseai#356, realsenseai#399, realsenseai#404, realsenseai#405, realsenseai#426) confirmed our 6.2 patch series already incorporates all upstream MAX96712-touching fixes. The "NVCSI sees zero frames" symptom we still have is therefore not addressed by any merged upstream PR.
No description provided.