Skip to content

[FIX] Improve D4XX streaming resilience and I2C failure handling #368

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.

Sign up for GitHub

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

Closed
ymodlin wants to merge 4 commits into from
Closed

[FIX] Improve D4XX streaming resilience and I2C failure handling #368

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e73bd5
[FIX] Improve D4XX streaming resilience and I2C failure handling
ymodlin Feb 19, 2026
47995ff
Bump version to 1.0.2.11
ymodlin Feb 19, 2026
b17850a
[FIX] Reset sensor pipe configuration on release
ymodlin Feb 19, 2026
b991538
Bump version to 1.0.2.12
ymodlin Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 128 additions & 35 deletions kernel/realsense/d4xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ enum ds5_mux_pad {
/* I2C retry configuration */
#define DS5_I2C_RETRY_COUNT 5
#define DS5_I2C_RETRY_DELAY_US 5000
#define DS5_I2C_DEAD_THRESHOLD 50 /* consecutive errors before declaring dead */

/* DFU definition section */
#define DFU_MAGIC_NUMBER "/0x01/0x02/0x03/0x04"
Expand Down Expand Up @@ -481,6 +482,9 @@ struct ds5 {
struct i2c_client *dser_i2c;
const struct dser_interface *dser_ops;
#endif
/* I2C health tracking */
atomic_t i2c_consec_errors;
bool camera_dead;
};

struct ds5_counters {
Expand Down Expand Up @@ -545,6 +549,9 @@ static int ds5_write(struct ds5 *state, u16 reg, u16 val)
int retry;
u8 value[2];

if (state->camera_dead)
return -ENODEV;

value[1] = val >> 8;
value[0] = val & 0x00FF;

Expand All @@ -557,20 +564,31 @@ static int ds5_write(struct ds5 *state, u16 reg, u16 val)
if (ret == 0)
break;
if (retry < DS5_I2C_RETRY_COUNT - 1) {
dev_warn(&state->client->dev,
dev_dbg(&state->client->dev,
"%s(): i2c write retry %d, 0x%04x = 0x%x, err %d\n",
__func__, retry + 1, reg, val, ret);
usleep_range(DS5_I2C_RETRY_DELAY_US,
DS5_I2C_RETRY_DELAY_US + 500);
}
}
if (ret < 0)
dev_err(&state->client->dev,
if (ret < 0) {
dev_warn_ratelimited(&state->client->dev,
"%s(): i2c write failed after %d retries, 0x%04x = 0x%x, err %d\n",
__func__, DS5_I2C_RETRY_COUNT, reg, val, ret);
else if (state->dfu_dev.dfu_state_flag == DS5_DFU_IDLE)
dev_dbg(&state->client->dev, "%s(): i2c write 0x%04x: 0x%x\n",
__func__, reg, val);
if (atomic_inc_return(&state->i2c_consec_errors) >=
DS5_I2C_DEAD_THRESHOLD) {
state->camera_dead = true;
dev_err(&state->client->dev,

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The circuit breaker threshold message is duplicated across all four I2C functions (ds5_write, ds5_raw_write, ds5_read, ds5_raw_read) with identical wording. If multiple I2C functions hit the threshold simultaneously or sequentially, the same "camera unreachable after 50 consecutive I2C errors" message will be logged multiple times. Since the goal is to reduce log noise (as mentioned in the PR description about 3,430+ identical messages), consider using a flag to ensure this critical message is only logged once, or at least using dev_err_ratelimited instead of dev_err.

Suggested change
dev_err(&state->client->dev,
dev_err_ratelimited(&state->client->dev,

Copilot uses AI. Check for mistakes.
"camera unreachable after %d consecutive I2C errors\n",
DS5_I2C_DEAD_THRESHOLD);
}
Comment on lines +578 to +584

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: the camera_dead flag is set without proper synchronization. While i2c_consec_errors is atomic, the camera_dead boolean is not protected by any lock. Multiple threads could simultaneously trigger the threshold and all call atomic_inc_return, leading to multiple threads setting camera_dead to true and logging the error message. More critically, one thread could read camera_dead as false at line 552 while another thread is setting it to true at line 580, potentially causing inconsistent behavior. Consider using atomic_cmpxchg or protecting both the counter check and the flag assignment with the state mutex.

Copilot uses AI. Check for mistakes.
} else {
atomic_set(&state->i2c_consec_errors, 0);
if (state->dfu_dev.dfu_state_flag == DS5_DFU_IDLE)
dev_dbg(&state->client->dev,
"%s(): i2c write 0x%04x: 0x%x\n",
__func__, reg, val);
}

return ret;
}
Expand All @@ -581,26 +599,39 @@ static int ds5_raw_write(struct ds5 *state, u16 reg,
int ret;
int retry;

if (state->camera_dead)
return -ENODEV;

for (retry = 0; retry < DS5_I2C_RETRY_COUNT; retry++) {
ret = regmap_raw_write(state->regmap, reg, val, val_len);
if (ret == 0)
break;
if (retry < DS5_I2C_RETRY_COUNT - 1) {
dev_warn(&state->client->dev,
dev_dbg(&state->client->dev,
"%s(): i2c raw write retry %d, 0x%04x size(%d), err %d\n",
__func__, retry + 1, reg, (int)val_len, ret);
usleep_range(DS5_I2C_RETRY_DELAY_US,
DS5_I2C_RETRY_DELAY_US + 500);
}
}
if (ret < 0)
dev_err(&state->client->dev,
if (ret < 0) {
dev_warn_ratelimited(&state->client->dev,
"%s(): i2c raw write failed after %d retries, 0x%04x size(%d), err %d\n",
__func__, DS5_I2C_RETRY_COUNT, reg, (int)val_len, ret);
else if (state->dfu_dev.dfu_state_flag == DS5_DFU_IDLE)
dev_dbg(&state->client->dev,
"%s(): i2c raw write 0x%04x: %d bytes\n",
__func__, reg, (int)val_len);
if (atomic_inc_return(&state->i2c_consec_errors) >=
DS5_I2C_DEAD_THRESHOLD) {
state->camera_dead = true;
dev_err(&state->client->dev,
"camera unreachable after %d consecutive I2C errors\n",
DS5_I2C_DEAD_THRESHOLD);
}
} else {
atomic_set(&state->i2c_consec_errors, 0);
if (state->dfu_dev.dfu_state_flag == DS5_DFU_IDLE)
dev_dbg(&state->client->dev,
"%s(): i2c raw write 0x%04x: %d bytes\n",
__func__, reg, (int)val_len);
}

return ret;
}
Expand All @@ -610,25 +641,39 @@ static int ds5_read(struct ds5 *state, u16 reg, u16 *val)
int ret;
int retry;

if (state->camera_dead)
return -ENODEV;

for (retry = 0; retry < DS5_I2C_RETRY_COUNT; retry++) {
ret = regmap_raw_read(state->regmap, reg, val, 2);
if (ret == 0)
break;
if (retry < DS5_I2C_RETRY_COUNT - 1) {
dev_warn(&state->client->dev,
dev_dbg(&state->client->dev,
"%s(): i2c read retry %d, 0x%04x, err %d\n",
__func__, retry + 1, reg, ret);
usleep_range(DS5_I2C_RETRY_DELAY_US,
DS5_I2C_RETRY_DELAY_US + 500);
}
}
if (ret < 0)
dev_err(&state->client->dev,
if (ret < 0) {
dev_warn_ratelimited(&state->client->dev,
"%s(): i2c read failed after %d retries, 0x%04x, err %d\n",
__func__, DS5_I2C_RETRY_COUNT, reg, ret);
else if (state->dfu_dev.dfu_state_flag == DS5_DFU_IDLE)
dev_dbg(&state->client->dev, "%s(): i2c read 0x%04x: 0x%x\n",
__func__, reg, *val);
if (atomic_inc_return(&state->i2c_consec_errors) >=
DS5_I2C_DEAD_THRESHOLD) {
state->camera_dead = true;
dev_err(&state->client->dev,
"camera unreachable after %d consecutive I2C errors\n",
DS5_I2C_DEAD_THRESHOLD);
}
} else {
atomic_set(&state->i2c_consec_errors, 0);
if (state->dfu_dev.dfu_state_flag == DS5_DFU_IDLE)
dev_dbg(&state->client->dev,
"%s(): i2c read 0x%04x: 0x%x\n",
__func__, reg, *val);
}

return ret;
}
Expand All @@ -638,22 +683,35 @@ static int ds5_raw_read(struct ds5 *state, u16 reg, void *val, size_t val_len)
int ret;
int retry;

if (state->camera_dead)
return -ENODEV;

for (retry = 0; retry < DS5_I2C_RETRY_COUNT; retry++) {
ret = regmap_raw_read(state->regmap, reg, val, val_len);
if (ret == 0)
break;
if (retry < DS5_I2C_RETRY_COUNT - 1) {
dev_warn(&state->client->dev,
dev_dbg(&state->client->dev,
"%s(): i2c raw read retry %d, 0x%04x size(%d), err %d\n",
__func__, retry + 1, reg, (int)val_len, ret);
usleep_range(DS5_I2C_RETRY_DELAY_US,
DS5_I2C_RETRY_DELAY_US + 500);
}
}
if (ret < 0)
dev_err(&state->client->dev,
if (ret < 0) {
dev_warn_ratelimited(&state->client->dev,
"%s(): i2c raw read failed after %d retries, 0x%04x size(%d), err %d\n",
__func__, DS5_I2C_RETRY_COUNT, reg, (int)val_len, ret);
if (atomic_inc_return(&state->i2c_consec_errors) >=
DS5_I2C_DEAD_THRESHOLD) {
state->camera_dead = true;
dev_err(&state->client->dev,
"camera unreachable after %d consecutive I2C errors\n",
DS5_I2C_DEAD_THRESHOLD);
}
} else {
atomic_set(&state->i2c_consec_errors, 0);
}

return ret;
}
Expand Down Expand Up @@ -2384,12 +2442,43 @@ static int ds5_hw_reset_with_recovery(struct ds5 *state)
sensor->pipe_configured = false;
sensor->pipe_id = -1;
}

/* Ensure all sensor streaming state is fully reset so
* ds5_configure() does a fresh setup on next stream-on */
for (i = 0; i < ARRAY_SIZE(sensors); i++)
sensors[i]->streaming = false;

mutex_unlock(&serdes_lock__);

dev_info(&state->client->dev,
"%s(): Re-initializing SERDES link\n", __func__);
state->dser_ops->reset_oneshot(state->dser_dev);
msleep(300);

/*
* Verify GMSL link is re-established by polling I2C.
* reset_oneshot itself sleeps 100ms internally; we add
* settling time and verify the link is actually alive
* rather than blindly sleeping a fixed 300ms.
*/
msleep(200);
for (i = 0; i < 10; i++) {
ret = ds5_read(state, DS5_FW_VERSION, &status);

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GMSL link verification uses status variable to read firmware version, but status is declared as u16 at line 2309 for use with hardware reset status checks. Reusing it here for firmware version reading is semantically confusing. While technically correct since both are u16 values, consider using a separate variable like fw_version for the GMSL link verification to improve code clarity and prevent future maintenance issues.

Copilot uses AI. Check for mistakes.
if (ret == 0)
break;
dev_dbg(&state->client->dev,
"%s(): GMSL link not ready, retry %d\n",
__func__, i);
msleep(100);
}
Comment on lines +2464 to +2472

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GMSL link verification polling (lines 2464-2472) will increment the consecutive error counter if I2C reads fail, potentially triggering the circuit breaker and setting camera_dead during legitimate recovery. Since ds5_read returns -ENODEV when camera_dead is true (line 644-645), once the threshold is hit during this polling loop, all subsequent reads will fail with -ENODEV, causing the recovery to fail at line 2473-2477 even if the hardware is actually recovering. This creates a catch-22 where recovery attempts can poison themselves. Consider temporarily suppressing the circuit breaker logic during hardware reset recovery, or resetting the error counter before attempting GMSL link verification.

Copilot uses AI. Check for mistakes.
if (ret < 0) {
dev_err(&state->client->dev,
"%s(): GMSL link failed to recover after reset_oneshot (%d)\n",
__func__, ret);
return -EIO;
}
dev_info(&state->client->dev,
"%s(): GMSL link verified after %d ms\n",

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GMSL link verification loop increments i from 0 to 9, but the final success message reports timing as 200 + i * 100 milliseconds. If the link succeeds on the first attempt (i=0, ret=0 at line 2466), the message claims "GMSL link verified after 200 ms" when actually 200ms was the initial sleep before any polling attempts. This is misleading. Consider either adjusting the message to account for this (e.g., using 200 + (i > 0 ? i * 100 : 0)) or restructuring the timing calculation to be more accurate.

Suggested change
"%s(): GMSL link verified after %d ms\n",
"%s(): GMSL link verified after %d ms (including initial 200 ms settling delay)\n",

Copilot uses AI. Check for mistakes.
__func__, 200 + i * 100);
Comment on lines 2455 to +2481

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The circuit breaker has no recovery mechanism. Once camera_dead is set to true, it remains permanently true for the lifetime of the driver instance. Even if hardware reset recovery (ds5_hw_reset_with_recovery) succeeds and verifies the GMSL link is working (lines 2464-2481), the camera_dead flag is never cleared. This means the camera will remain unusable until the driver is unloaded and reloaded. Consider resetting both camera_dead and i2c_consec_errors after successful hardware reset recovery to allow the driver to recover from transient failures.

Copilot uses AI. Check for mistakes.
}
#endif

Expand Down Expand Up @@ -4681,6 +4770,7 @@ static int ds5_mux_s_stream(struct v4l2_subdev *sd, int on)
if (state->dser_ops->release_pipe(state->dser_dev, sensor->pipe_id) < 0)
dev_warn(&state->client->dev, "release pipe failed\n");
sensor->pipe_id = -1;
sensor->pipe_configured = false;
mutex_unlock(&serdes_lock__);
}
#endif
Expand Down Expand Up @@ -5560,20 +5650,21 @@ static int ds5_dfu_device_release(struct inode *inode, struct file *file)
struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(
state->client->adapter);
#endif
int ret = 0, retry = 10;
int ret = 0, retry = 3;
mutex_lock(&state->lock);
state->dfu_dev.device_open_count--;
if (state->dfu_dev.dfu_state_flag != DS5_DFU_RECOVERY)
state->dfu_dev.dfu_state_flag = DS5_DFU_IDLE;
/* We disable this section as it has no effect when device in operational
mode and has not enough effect when device in recovery mode */
// if (state->dfu_dev.dfu_state_flag == DS5_DFU_DONE
// && state->dfu_dev.init_v4l_f)
// ds5_v4l_init(state->client, state);
// state->dfu_dev.init_v4l_f = 0;
if (state->dfu_dev.dfu_msg)
devm_kfree(&state->client->dev, state->dfu_dev.dfu_msg);
state->dfu_dev.dfu_msg = NULL;

/* Skip I2C verification if camera is unreachable */
if (state->camera_dead) {
mutex_unlock(&state->lock);
return 0;
}

#ifdef CONFIG_TEGRA_CAMERA_PLATFORM
/* get i2c controller and restore bus clock rate */
while (parent && i2c_parent_is_i2c_adapter(parent))
Expand All @@ -5594,16 +5685,16 @@ static int ds5_dfu_device_release(struct inode *inode, struct file *file)
ret = ds5_read(state, DS5_FW_VERSION, &state->fw_version);
if (ret)
msleep_range(10);
} while (retry-- && ret != 0 );
} while (retry-- && ret != 0);
if (ret) {
dev_warn(&state->client->dev,
dev_warn_ratelimited(&state->client->dev,
"%s(): no communication with d4xx\n", __func__);
mutex_unlock(&state->lock);
return ret;
return 0; /* release must succeed to avoid FD leak */
}
ret = ds5_read(state, DS5_FW_BUILD, &state->fw_build);

Copilot AI Feb 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function now unconditionally returns 0 even after the final ds5_read for FW_BUILD (line 5694) regardless of whether it succeeds or fails. While this prevents FD leaks (which is good), silently ignoring errors from reading FW_BUILD means the firmware version information could be incomplete or stale. If consistency is important, either document why FW_BUILD read failures are acceptable, or at least log a debug message when line 5694 fails so there's a record in the logs.

Suggested change
ret = ds5_read(state, DS5_FW_BUILD, &state->fw_build);
ret = ds5_read(state, DS5_FW_BUILD, &state->fw_build);
if (ret)
dev_dbg(&state->client->dev,
"%s(): failed to read FW_BUILD, firmware build info may be stale (err=%d)\n",
__func__, ret);

Copilot uses AI. Check for mistakes.
mutex_unlock(&state->lock);
return ret;
return 0;
};

static const struct file_operations ds5_device_file_ops = {
Expand Down Expand Up @@ -5829,6 +5920,8 @@ static int ds5_probe(struct i2c_client *c, const struct i2c_device_id *id)
return -ENOMEM;

mutex_init(&state->lock);
atomic_set(&state->i2c_consec_errors, 0);
state->camera_dead = false;

state->client = c;
dev_warn(&c->dev, "Probing driver for D4xx\n");
Expand Down Expand Up @@ -6073,4 +6166,4 @@ MODULE_AUTHOR("Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>,\n\
Shikun Ding <shikun.ding@intel.com>,\n\
Dmitry Perchanov <dmitry.perchanov@intel.com>");
MODULE_LICENSE("GPL v2");
MODULE_VERSION("1.0.2.10");
MODULE_VERSION("1.0.2.12");