Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements hardware reset recovery for the first probe instance and refactors the sensor invalidation logic to improve device reliability after resets. The changes introduce a probe-time hardware reset mechanism and modify how sensor state is invalidated when a reset is detected.
Changes:
- Add hardware reset recovery during the first device probe instance using atomic flag
- Refactor sensor invalidation from all-sensors to active-sensor-only approach
- Add device type register polling after HW reset to ensure firmware initialization completes
- Update D401 framerate arrays to include lower framerates (5fps, 10fps)
Comments suppressed due to low confidence (6)
kernel/realsense/d4xx.c:1688
- In ds5_invalidate_sensor(), the SERDES pipe is released only when CONFIG_VIDEO_D4XX_SERDES is defined, state->dser_dev is set, and sensor->pipe_id >= 0. However, if the pipe release operation fails (e.g., the SERDES device is no longer accessible), the error is silently ignored. This could lead to resource leaks in the SERDES deserializer. Consider logging the return value from release_pipe() similar to how it's done at line 1780 in ds5_configure(), or at minimum checking for errors to help with debugging.
if (sensor->pipe_id >= 0 && state->dser_dev) {
mutex_lock(&serdes_lock__);
state->dser_ops->release_pipe(state->dser_dev, sensor->pipe_id);
mutex_unlock(&serdes_lock__);
kernel/realsense/d4xx.c:2291
- The commented-out code block at lines 2273-2291 has inconsistent brace placement that doesn't follow the Linux kernel coding style. The opening brace for the if statement at line 2274 and the for loop braces at line 2283 are on separate lines, which deviates from the kernel style where opening braces typically go on the same line for control structures. If this code is ever uncommented, it should be reformatted to match kernel conventions.
/* TBD: Open for single instance architecture and remove ds5_invalidate_sensor multi instance solution
struct ds5_sensor *sensors[] = {
&state->depth.sensor,
&state->ir.sensor,
&state->rgb.sensor,
&state->imu.sensor,
};
int i;
if (state->dser_dev)
{
mutex_lock(&serdes_lock__);
for (i = 0; i < ARRAY_SIZE(sensors); i++)
{
struct ds5_sensor *sensor = sensors[i];
ds5_config_cache_clear(sensor);
if (sensor->pipe_id >= 0)
{
int release_ret = max9296_release_pipe(state->dser_dev, sensor->pipe_id);
dev_warn(&state->client->dev, "release pipe %d (%d)\n",
sensor->pipe_id, release_ret);
sensor->pipe_id = PIPE_NOT_CONFIGURED;
}
}
mutex_unlock(&serdes_lock__);
}
kernel/realsense/d4xx.c:2286
- The commented-out code at line 2284 hardcodes a call to max9296_release_pipe() instead of using state->dser_ops->release_pipe() through the deserializer abstraction interface. This breaks the abstraction pattern used throughout the driver where the specific deserializer operations are accessed via the dser_ops function pointer. The active code at line 1687 correctly uses the abstraction. If this commented code is ever reactivated, this hardcoded call should be fixed to use the interface.
/* TBD: Open for single instance architecture and remove ds5_invalidate_sensor multi instance solution
struct ds5_sensor *sensors[] = {
&state->depth.sensor,
&state->ir.sensor,
&state->rgb.sensor,
&state->imu.sensor,
};
int i;
if (state->dser_dev)
{
mutex_lock(&serdes_lock__);
for (i = 0; i < ARRAY_SIZE(sensors); i++)
{
struct ds5_sensor *sensor = sensors[i];
ds5_config_cache_clear(sensor);
if (sensor->pipe_id >= 0)
{
int release_ret = max9296_release_pipe(state->dser_dev, sensor->pipe_id);
dev_warn(&state->client->dev, "release pipe %d (%d)\n",
sensor->pipe_id, release_ret);
kernel/realsense/d4xx.c:2263
- The multi-line comment at lines 2262-2263 does not follow the standard Linux kernel multi-line comment style. The comment should start with /* on its own line, followed by * for subsequent lines, and end with / on its own line. The current format has the opening / on the same line as the first sentence, which is inconsistent with kernel coding standards.
/* Invalidate cached sensor configs and release SERDES pipes before reset, since reset will clear device state but not driver state.
This ensures we don't try to reuse stale configs or pipes after reset. */
kernel/realsense/d4xx.c:5928
- The error handling at line 5928 goes to e_chardev label, but the chardev is only initialized when state->is_depth is true (line 5915). If the probe instance that runs the HW reset recovery is not a depth sensor (e.g., state->is_depth == 0), then the e_chardev cleanup path at line 5974-5976 will check if state->dfu_dev.ds5_class is set, which may not be the case. While the code handles this with a null check, it would be clearer to use a different error label (e.g., e_regulator) for consistency with the depth check logic.
goto e_chardev;
kernel/realsense/d4xx.c:2401
- The variable 'retry' at line 2401 is reusing the 'retry' variable declared at line 2255 in the function scope. This creates a shadowing issue where the inner scope variable shadows the outer one. While the code block creates a new scope with braces, this pattern can be confusing and error-prone. Consider using a different variable name (e.g., 'dev_type_retry') for clarity and to avoid potential maintenance issues.
for (retry = 0; retry < DS5_HW_RESET_MAX_RETRIES; retry++) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct ds5_sensor *active = ds5_get_active_sensor(state); | ||
|
|
||
| if (active) | ||
| ds5_invalidate_sensor(state, active); |
There was a problem hiding this comment.
The new sensor invalidation logic only invalidates the currently active sensor determined by ds5_get_active_sensor() at lines 1716-1719. However, a hardware reset affects the entire device, not just the active sensor. If multiple sensors were previously configured and active, only the current one will be invalidated after reset. This could lead to stale cached configurations in inactive sensors. The commented-out code at lines 2264-2292 suggests this was previously handled by invalidating all sensors. Consider whether the current approach correctly handles multi-sensor scenarios or if selective invalidation is intentional for the multi-instance architecture mentioned in the TBD comment.
| struct ds5_sensor *active = ds5_get_active_sensor(state); | |
| if (active) | |
| ds5_invalidate_sensor(state, active); | |
| /* | |
| * A hardware reset affects the entire device, so invalidate | |
| * all sensor contexts associated with this ds5 instance to | |
| * avoid stale cached configurations. | |
| */ | |
| ds5_invalidate_sensor(state, &state->depth.sensor); | |
| ds5_invalidate_sensor(state, &state->rgb.sensor); | |
| ds5_invalidate_sensor(state, &state->ir.sensor); | |
| ds5_invalidate_sensor(state, &state->imu.sensor); |
| @@ -2289,7 +2306,6 @@ static int ds5_hw_reset_with_recovery(struct ds5 *state) | |||
| return ret; | |||
| } | |||
| atomic_inc(&ds5_reset_gen); | |||
There was a problem hiding this comment.
The removal of state->reset_gen = atomic_read(&ds5_reset_gen); after incrementing the reset generation counter creates a potential issue. Without this line, the state's local reset_gen will be out of sync with the global ds5_reset_gen immediately after the hardware reset. This means that in ds5_configure() (line 1715), the condition state->reset_gen != current_reset_gen will be true on the next configure call, which will trigger ds5_invalidate_sensor() unnecessarily. This defeats the purpose of the reset_gen mechanism for this particular state instance.
| atomic_inc(&ds5_reset_gen); | |
| atomic_inc(&ds5_reset_gen); | |
| state->reset_gen = atomic_read(&ds5_reset_gen); |
| DS5_RES(1280, 720, ds5_framerate_to_30)\ | ||
| DS5_RES(848, 480, ds5_framerate_to_60)\ | ||
| DS5_RES(640, 480, ds5_framerate_to_60)\ | ||
| DS5_RES(640, 360, ds5_framerate_to_60)\ | ||
| DS5_RES(480, 270, ds5_framerate_to_60)\ | ||
| DS5_RES(424, 240, ds5_framerate_to_60)\ |
There was a problem hiding this comment.
The framerate arrays are being changed from ds5_framerate_15_30 (which contains {15, 30}) to ds5_framerate_to_30 (which contains {5, 10, 15, 30}), and from ds5_framerate_15_60 (which contains {15, 30, 60}) to ds5_framerate_to_60 (which contains {5, 15, 30, 60}). This adds support for lower frame rates (5 fps and 10 fps) that were not previously available for D401. This is a behavioral change that affects supported frame rates for D401 devices. Verify that the firmware on D401 devices actually supports these lower frame rates, as this change expands the advertised capabilities.
| DS5_RES(1280, 720, ds5_framerate_to_30)\ | |
| DS5_RES(848, 480, ds5_framerate_to_60)\ | |
| DS5_RES(640, 480, ds5_framerate_to_60)\ | |
| DS5_RES(640, 360, ds5_framerate_to_60)\ | |
| DS5_RES(480, 270, ds5_framerate_to_60)\ | |
| DS5_RES(424, 240, ds5_framerate_to_60)\ | |
| DS5_RES(1280, 720, ds5_framerate_15_30)\ | |
| DS5_RES(848, 480, ds5_framerate_15_60)\ | |
| DS5_RES(640, 480, ds5_framerate_15_60)\ | |
| DS5_RES(640, 360, ds5_framerate_15_60)\ | |
| DS5_RES(480, 270, ds5_framerate_15_60)\ | |
| DS5_RES(424, 240, ds5_framerate_15_60)\ |
| if (retry >= DS5_HW_RESET_MAX_RETRIES) | ||
| dev_warn(&state->client->dev, | ||
| "%s(): Device type register not ready (0x%x) after %d ms, formats may be wrong\n", | ||
| __func__, dev_type, DS5_HW_RESET_TIMEOUT_MS); |
There was a problem hiding this comment.
The new device type polling logic (lines 2390-2417) adds robustness by waiting for the DS5_DEVICE_TYPE register to be populated after reset. However, if the polling times out (retry >= DS5_HW_RESET_MAX_RETRIES), only a warning is logged and execution continues. This could lead to ds5_fixed_configuration() reading dev_type as 0, which would incorrectly select D46X format arrays (see line 4987-4988 in the default case). This timeout should be treated as a hard error rather than just a warning, as continuing with incorrect format arrays can cause functional issues.
| if (retry >= DS5_HW_RESET_MAX_RETRIES) | |
| dev_warn(&state->client->dev, | |
| "%s(): Device type register not ready (0x%x) after %d ms, formats may be wrong\n", | |
| __func__, dev_type, DS5_HW_RESET_TIMEOUT_MS); | |
| if (retry >= DS5_HW_RESET_MAX_RETRIES) { | |
| dev_err(&state->client->dev, | |
| "%s(): Device type register not ready (0x%x) after %d ms, aborting initialization\n", | |
| __func__, dev_type, DS5_HW_RESET_TIMEOUT_MS); | |
| return -ETIMEDOUT; | |
| } |
| if (atomic_cmpxchg(&ds5_probe_reset_once, 0, 1) == 0) { | ||
| dev_info(&c->dev, "%s(): first probe instance, running HW reset recovery\n", | ||
| __func__); | ||
| ret = ds5_hw_reset_with_recovery(state); |
There was a problem hiding this comment.
The hardware reset with recovery is called at line 5924 during probe, but it occurs after state->reset_gen has already been initialized at line 5822 to the current value of ds5_reset_gen. Since ds5_hw_reset_with_recovery() increments ds5_reset_gen (line 2308) but no longer updates state->reset_gen, this state instance will be out of sync. When ds5_configure() is later called during device initialization, it will detect the mismatch and incorrectly invalidate the sensor. This creates an inconsistency between the probe-time reset intention and the runtime state management.
| if (atomic_cmpxchg(&ds5_probe_reset_once, 0, 1) == 0) { | ||
| dev_info(&c->dev, "%s(): first probe instance, running HW reset recovery\n", | ||
| __func__); | ||
| ret = ds5_hw_reset_with_recovery(state); | ||
| if (ret < 0) { | ||
| dev_err(&c->dev, "%s(): probe HW reset recovery failed: %d\n", | ||
| __func__, ret); | ||
| goto e_chardev; | ||
| } | ||
| } |
There was a problem hiding this comment.
The hardware reset recovery is executed only for the first probe instance (determined by atomic_cmpxchg). However, if the first probe fails after the reset but before completing initialization (e.g., due to errors at lines 5932-5937 or 5955-5957), the atomic flag remains set to 1. This prevents subsequent probe attempts from performing the hardware reset recovery, potentially leaving the device in an inconsistent state. Consider resetting the ds5_probe_reset_once flag on probe failure, or using a per-device flag instead of a global one.
| @@ -2270,9 +2289,7 @@ static int ds5_hw_reset_with_recovery(struct ds5 *state) | |||
| } | |||
| mutex_unlock(&serdes_lock__); | |||
| } | |||
|
|
|||
| dev_info(&state->client->dev, "%s(): Initiating HW reset with recovery\n", | |||
| __func__); | |||
| */ | |||
There was a problem hiding this comment.
The commented-out code block (lines 2264-2292) appears to be an older implementation that invalidated all sensors during hardware reset. The new implementation (ds5_invalidate_sensor in ds5_configure) only invalidates the active sensor. However, this commented code is left with a "TBD" note suggesting it should be reopened for single instance architecture. This creates ambiguity about the intended design. If the commented code represents the correct approach for certain scenarios, it should be properly documented or removed. The TBD comment suggests there's uncertainty about whether the current solution is optimal.
| if (state->is_depth) | ||
| return &state->depth.sensor; | ||
| if (state->is_rgb) | ||
| return &state->rgb.sensor; | ||
| if (state->is_y8) | ||
| return &state->ir.sensor; | ||
| if (state->is_imu) | ||
| return &state->imu.sensor; | ||
| return NULL; |
There was a problem hiding this comment.
The ds5_get_active_sensor() function returns the first sensor that has its corresponding state flag (is_depth, is_rgb, is_y8, is_imu) set to true. However, there's no validation that only one of these flags is set at a time. If multiple flags are set (which could happen due to a bug or race condition), this function will return the first matching sensor based on the order of checks, which may not be the intended behavior. Consider adding validation or documentation about the mutual exclusivity of these flags.
| if (state->is_depth) | |
| return &state->depth.sensor; | |
| if (state->is_rgb) | |
| return &state->rgb.sensor; | |
| if (state->is_y8) | |
| return &state->ir.sensor; | |
| if (state->is_imu) | |
| return &state->imu.sensor; | |
| return NULL; | |
| int active_cnt = 0; | |
| struct ds5_sensor *active = NULL; | |
| if (state->is_depth) { | |
| active_cnt++; | |
| if (!active) | |
| active = &state->depth.sensor; | |
| } | |
| if (state->is_rgb) { | |
| active_cnt++; | |
| if (!active) | |
| active = &state->rgb.sensor; | |
| } | |
| if (state->is_y8) { | |
| active_cnt++; | |
| if (!active) | |
| active = &state->ir.sensor; | |
| } | |
| if (state->is_imu) { | |
| active_cnt++; | |
| if (!active) | |
| active = &state->imu.sensor; | |
| } | |
| if (active_cnt > 1) | |
| dev_warn(&state->client->dev, | |
| "multiple active sensor flags set (depth=%d, rgb=%d, y8=%d, imu=%d); using first active sensor\n", | |
| state->is_depth, state->is_rgb, | |
| state->is_y8, state->is_imu); | |
| return active; |
|
|
||
| for (retry = 0; retry < DS5_HW_RESET_MAX_RETRIES; retry++) { | ||
| ret = ds5_read(state, DS5_DEVICE_TYPE, &dev_type); | ||
| if (ret == 0 && dev_type != 0) { |
There was a problem hiding this comment.
The device type polling logic checks for 'dev_type != 0' at line 2403, but doesn't validate whether the read value is one of the known valid device types (DS5_DEVICE_TYPE_D40X through DS5_DEVICE_TYPE_D46X, which are values 4-8 based on lines 92-96). A dev_type value that is non-zero but invalid (e.g., 0xFF, 0x1234) would pass this check but still cause ds5_fixed_configuration() to fall into the default case. Consider validating against the known device type values or at least checking if dev_type is within a reasonable range.
| if (ret == 0 && dev_type != 0) { | |
| if (ret == 0 && | |
| dev_type >= DS5_DEVICE_TYPE_D40X && | |
| dev_type <= DS5_DEVICE_TYPE_D46X) { |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement hardware reset recovery for the first probe instance and enhance sensor invalidation logic to ensure proper initialization after a reset. This change improves device reliability and state management.
Fixing 5 FPS issue: https://rsjira.realsenseai.com/browse/RSDSO-21189