Enhance DS5 I2C communication and add d4xx.c copying for JetPack - #360
Conversation
15fc1d8 to
a8430fa
Compare
…opying for JetPack versions
a8430fa to
f70b02b
Compare
This reverts commit 7cc6490.
There was a problem hiding this comment.
Pull request overview
This pull request enhances I2C communication reliability for D4XX camera drivers on NVIDIA Jetson platforms by adding retry logic for stream control operations and implementing a register write caching mechanism to reduce I2C traffic. It also updates the build system to correctly copy the d4xx.c driver file based on JetPack version (6.x vs 5.x/4.6.1), reflecting different kernel directory structures.
Changes:
- Added global mutex-protected SERDES pipe management with caching to prevent concurrent access and reduce redundant I2C operations
- Refactored stream start/stop logic with improved retry mechanism and state verification
- Updated build script to handle JetPack version-specific directory structures for d4xx.c deployment
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| kernel/realsense/d4xx.c | Core driver changes: added pipe management caching, register write caching, refactored stream control with retry logic, and hardware reset cleanup for SERDES pipes |
| build_all.sh | Added conditional logic to copy d4xx.c to appropriate directory based on JetPack version (nvidia-oot for 6.x, kernel/nvidia for others) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dev_warn(&state->client->dev, | ||
| "stop streaming timeout, stream_status: 0x%04x\n", | ||
| streaming); | ||
| "stop streaming timeout, stream %d status: 0x%04x\n", stream_id, streaming); |
There was a problem hiding this comment.
Stale variable in error message: The 'streaming' variable used in this error message may contain a stale value. The loop at lines 5033-5044 only reads 'status', not 'streaming', so the value printed here could be from an earlier read or even uninitialized in some code paths. Consider reading stream_status_base into 'streaming' just before this message or removing the streaming value from the message.
| "stop streaming timeout, stream %d status: 0x%04x\n", stream_id, streaming); | |
| "stop streaming timeout, stream %d status: 0x%04x\n", stream_id, status); |
| mutex_unlock(&serdes_lock__); | ||
| if (sensor->pipe_id < 0) { | ||
| dev_err(&state->client->dev, "No free pipe in max9296\n"); | ||
| ret = -(ENOSR); |
There was a problem hiding this comment.
Unnecessary parentheses around error constant: The error code assignment uses -(ENOSR) which is unconventional. The standard kernel style is -ENOSR without parentheses around the constant. While functionally equivalent, this deviates from standard Linux kernel coding conventions.
| ret = -(ENOSR); | |
| ret = -ENOSR; |
| if (state->is_depth && fmt != 0) | ||
| ret = ds5_write(state, dt_addr, 0x31); | ||
| dt_value = 0x31; | ||
| else if (state->is_y8 && fmt != 0 && | ||
| sensor->config.format->data_type == GMSL_CSI_DT_YUV422_8) { | ||
| if (sensor->config.format->mbus_code == MEDIA_BUS_FMT_VYUY8_1X16) | ||
| { | ||
| /* This is the custom Y8I format - | ||
| * telling FW to enable "etMipiDataType_UserDefined3_R8L8" | ||
| */ | ||
| ret = ds5_write(state, dt_addr, GMSL_CSI_DT_CUSTOM_Y8I_16); | ||
| } else if (sensor->config.format->mbus_code == MEDIA_BUS_FMT_YUYV8_1X16) { | ||
| /* This is the custom RGB through IR format - | ||
| * telling FW to enable "etMipiDataType_UserDefined0_IR_RGB" | ||
| */ | ||
| ret = ds5_write(state, dt_addr, GMSL_CSI_DT_CUSTOM_IR_RGB_16); | ||
| } else { | ||
| dev_err(sensor->sd.dev, "%s(): Illegal mbus_code %u for IR sensor\n", | ||
| __func__, sensor->config.format->mbus_code); | ||
| return -EINVAL; | ||
| } | ||
| } else { | ||
| sensor->config.format->data_type == GMSL_CSI_DT_YUV422_8) | ||
| ret = ds5_write(state, dt_addr, 0x32); | ||
| else | ||
| ret = ds5_write(state, dt_addr, fmt); | ||
| } | ||
| if (ret < 0) | ||
| return ret; | ||
|
|
||
| ret = ds5_write(state, md_addr, (vc_id << 8) | md_fmt); | ||
| if (ret < 0) | ||
| return ret; | ||
| if (sensor->cached_dt_value != dt_value) { | ||
| ret = ds5_write(state, dt_addr, dt_value); | ||
| if (ret < 0) | ||
| return ret; | ||
| sensor->cached_dt_value = dt_value; | ||
| } |
There was a problem hiding this comment.
Critical logic error: Lines 2017-2023 write directly to dt_addr and return on error, but dt_value remains 0 (its initial value). Then lines 2025-2030 unconditionally write dt_value (which is 0) to dt_addr again, overwriting the previous value. This will cause incorrect data type values to be written for the Y8 sensor case.
The code should set dt_value for all branches (depth, y8, and else cases) and only write once using the caching mechanism. Lines 2017-2023 should be removed and replaced with appropriate dt_value assignments.
| #ifdef CONFIG_VIDEO_D4XX_SERDES | ||
| mutex_unlock(&serdes_lock__); |
There was a problem hiding this comment.
Orphaned mutex_unlock: There is a mutex_unlock call here without a corresponding mutex_lock. The pipe allocation logic was moved into ds5_configure() where the mutex is properly locked and unlocked (lines 1964-1966 and 1978-1981), so this unlock call should be removed.
| /* notify fw */ | ||
| ret = ds5_write(state, DS5_START_STOP_STREAM, | ||
| DS5_STREAM_STOP | stream_id); | ||
| "start streaming failed after retry %d, status 0x%04x stream 0x%04x %d\n", |
There was a problem hiding this comment.
Misleading error message parameters: The error message format shows "status 0x%04x stream 0x%04x %d" but the actual parameters are (status, stream_id, streaming). This is confusing because:
- stream_id is printed where it says "stream" (which makes it look like a status value)
- streaming value is printed at the end without a format descriptor
The message should either use "stream_id 0x%04x streaming 0x%04x" or better yet "status 0x%04x stream_id 0x%04x streaming_state 0x%04x" to clarify what each value represents.
| "start streaming failed after retry %d, status 0x%04x stream 0x%04x %d\n", | |
| "start streaming failed after retry %d, status 0x%04x stream_id 0x%04x streaming_state 0x%04x\n", |
| continue; | ||
| } | ||
| if (streaming != DS5_STREAM_STREAMING) { | ||
| ds5_write(state, DS5_START_STOP_STREAM, DS5_STREAM_START | stream_id); |
There was a problem hiding this comment.
Unchecked return value: The ds5_write call on this line does not check for errors. If the write fails, the code will continue retrying indefinitely without detecting the I2C failure. Consider checking the return value and handling errors appropriately, possibly by breaking out of the loop or logging the error.
| ds5_write(state, DS5_START_STOP_STREAM, DS5_STREAM_START | stream_id); | |
| ret = ds5_write(state, DS5_START_STOP_STREAM, | |
| DS5_STREAM_START | stream_id); | |
| if (ret < 0) { | |
| dev_err(&state->client->dev, | |
| "start: failed to resend stream start command: %d\n", | |
| ret); | |
| break; | |
| } |
Improve I2C communication reliability with retry logic and ensure d4xx.c is copied to the correct directory based on JetPack version.