Skip to content

Add reset generation tracking and cache invalidation for DS5 sensors #381

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

Merged
ymodlin merged 2 commits into from
Feb 25, 2026
Merged

Add reset generation tracking and cache invalidation for DS5 sensors #381

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97d90c3
Add reset generation tracking and cache invalidation for DS5 sensors
Feb 24, 2026
a73f9e8
Align D401 resolution to the POR
Feb 24, 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
70 changes: 53 additions & 17 deletions kernel/realsense/d4xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ struct ds5 {
int is_depth, is_y8, is_rgb, is_imu;
bool metadata_enabled;
int aggregated;

Copilot AI Feb 24, 2026

Copy link

Choose a reason for hiding this comment

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

The reset_gen field should have a comment explaining its purpose, such as "Generation counter for tracking hardware resets; compared against global ds5_reset_gen to detect when device has been reset and cache needs invalidation."

Suggested change
int aggregated;
int aggregated;
/* Generation counter for tracking hardware resets; compared against
* global ds5_reset_gen to detect when device has been reset and any
* cached state needs invalidation.
*/

Copilot uses AI. Check for mistakes.
int reset_gen;
u16 fw_version;
u16 fw_build;
#ifdef CONFIG_VIDEO_D4XX_SERDES
Expand All @@ -492,6 +493,8 @@ struct ds5_counters {
unsigned int n_ctrl;
};

Copilot AI Feb 24, 2026

Copy link

Choose a reason for hiding this comment

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

The ds5_reset_gen global variable should have a comment explaining its purpose and scope. For example: "Global generation counter incremented on each hardware reset. Each device instance tracks this to detect resets and invalidate cached configuration."

Suggested change
/* Global reset generation counter incremented on each hardware reset.
* Each ds5 device instance tracks this to detect resets and invalidate
* any cached configuration that may no longer be valid.
*/

Copilot uses AI. Check for mistakes.
static atomic_t ds5_reset_gen = ATOMIC_INIT(0);

Copilot AI Feb 24, 2026

Copy link

Choose a reason for hiding this comment

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

The global atomic counter ds5_reset_gen is shared across all DS5 device instances. When one device is reset, the counter increments, causing all other device instances to invalidate their caches on their next configure call, even though they weren't reset. This could lead to unnecessary cache invalidations and configuration writes for devices that haven't been reset. Consider making the reset generation counter per-device (non-static) or using a different synchronization mechanism if multiple DS5 devices can be present in the system.

Copilot uses AI. Check for mistakes.

@ymodlin ymodlin Feb 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not relevant


#ifdef CONFIG_VIDEO_D4XX_SERDES
static DEFINE_MUTEX(serdes_lock__);

Expand Down Expand Up @@ -700,23 +703,22 @@ static const u16 ds5_framerate_100[] = {100};

#define D401_COMMON_RES \
DS5_RES(1280, 720, ds5_framerate_15_30)\
DS5_RES(848, 480, ds5_framerate_15_90)\
DS5_RES(640, 480, ds5_framerate_15_90)\
DS5_RES(640, 360, ds5_framerate_15_90)\
DS5_RES(480, 270, ds5_framerate_15_90)\
DS5_RES(424, 240, ds5_framerate_15_90)\
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)\

static const struct ds5_resolution d40x_depth_sizes[] = {
D401_COMMON_RES
DS5_RES(256, 144, ds5_framerate_15_90)
DS5_RES(256, 144, ds5_framerate_90)
};

static const struct ds5_resolution d40x_y8_sizes[] = {
D401_COMMON_RES
};

static const struct ds5_resolution d40x_rgb_sizes[] = {
DS5_RES(1280, 800, ds5_framerate_15_30)
D401_COMMON_RES
};

Expand Down Expand Up @@ -1652,9 +1654,41 @@ static int ds5_setup_pipeline(struct ds5 *state, u8 data_type1, u8 data_type2,
}
#endif

static void ds5_config_cache_clear(struct ds5_sensor *sensor)
{
sensor->cached_dt_value = 0xFFFF;
sensor->cached_md_value = 0xFFFF;
sensor->cached_override_value = 0xFFFF;
sensor->cached_fps_value = 0xFFFF;
sensor->cached_width_value = 0xFFFF;
sensor->cached_height_value = 0xFFFF;
}

static void ds5_invalidate_state_cache(struct ds5 *state)
{
struct ds5_sensor *sensors[] = {
&state->depth.sensor,
&state->ir.sensor,
&state->rgb.sensor,
&state->imu.sensor,
};
int i;

for (i = 0; i < ARRAY_SIZE(sensors); i++) {
struct ds5_sensor *sensor = sensors[i];

ds5_config_cache_clear(sensor);
sensor->pipe_id = PIPE_NOT_CONFIGURED;
sensor->pipe_data_type1 = 0;
sensor->pipe_data_type2 = 0;
sensor->pipe_vc_id = 0;
}
}

static int ds5_configure(struct ds5 *state)
{
struct ds5_sensor *sensor;
int current_reset_gen;
u16 fmt, md_fmt, vc_id;
#ifdef CONFIG_VIDEO_D4XX_SERDES
u16 data_type1, data_type2;
Expand All @@ -1668,6 +1702,12 @@ static int ds5_configure(struct ds5 *state)
u16 height_value = 0;
int ret;

current_reset_gen = atomic_read(&ds5_reset_gen);

@ymodlin ymodlin Feb 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

reset_gen is shared between the instances ?

if (state->reset_gen != current_reset_gen) {
ds5_invalidate_state_cache(state);
state->reset_gen = current_reset_gen;
}

if (state->is_depth) {
sensor = &state->depth.sensor;
dt_addr = DS5_DEPTH_STREAM_DT;
Expand Down Expand Up @@ -1775,6 +1815,9 @@ static int ds5_configure(struct ds5 *state)
sensor->config.format->data_type == GMSL_CSI_DT_YUV422_8)
dt_value = 0x32;

dev_dbg(&state->client->dev, "sensor %p: dt_value=0x%x, cached_dt_value=0x%x, cached_fps_value=%u, framerate=%u\n",
sensor, dt_value, sensor->cached_dt_value, sensor->cached_fps_value, sensor->config.framerate);
Comment on lines +1818 to +1819

Copilot AI Feb 24, 2026

Copy link

Choose a reason for hiding this comment

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

The debug log includes sensor pointer address which may not be very useful for debugging. Consider logging the sensor name (sensor->sd.name) instead to make the debug output more meaningful.

Suggested change
dev_dbg(&state->client->dev, "sensor %p: dt_value=0x%x, cached_dt_value=0x%x, cached_fps_value=%u, framerate=%u\n",
sensor, dt_value, sensor->cached_dt_value, sensor->cached_fps_value, sensor->config.framerate);
dev_dbg(&state->client->dev, "sensor %s: dt_value=0x%x, cached_dt_value=0x%x, cached_fps_value=%u, framerate=%u\n",
sensor->sd.name, dt_value, sensor->cached_dt_value, sensor->cached_fps_value, sensor->config.framerate);

Copilot uses AI. Check for mistakes.

@ymodlin ymodlin Feb 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree


if (sensor->cached_dt_value != dt_value) {
ret = ds5_write(state, dt_addr, dt_value);
if (ret < 0)
Expand Down Expand Up @@ -2183,16 +2226,6 @@ static int ds5_set_calibration_data(struct ds5 *state,
#define DS5_HW_RESET_STATUS_READY 0xDEAD
#define DS5_HW_RESET_DFU_MAGIC_LSW 0x0201 /* Lower 16 bits of 0x04030201 */

static void ds5_config_cache_clear(struct ds5_sensor *sensor)
{
sensor->cached_dt_value = 0xFFFF;
sensor->cached_md_value = 0xFFFF;
sensor->cached_override_value = 0xFFFF;
sensor->cached_fps_value = 0xFFFF;
sensor->cached_width_value = 0xFFFF;
sensor->cached_height_value = 0xFFFF;
}

/*
* ds5_hw_reset_with_recovery - Perform hardware reset with GMSL recovery
* @state: Driver state structure
Expand Down Expand Up @@ -2255,6 +2288,8 @@ static int ds5_hw_reset_with_recovery(struct ds5 *state)
__func__, ret);
return ret;
}
atomic_inc(&ds5_reset_gen);
state->reset_gen = atomic_read(&ds5_reset_gen);
Comment on lines +2291 to +2292

Copilot AI Feb 24, 2026

Copy link

Choose a reason for hiding this comment

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

The cache clearing for SERDES (lines 2255-2273) happens before the reset generation counter is incremented (line 2292). This creates a window where the cache is cleared but the reset generation hasn't been updated yet. If another thread calls ds5_configure during this window, it may not detect the reset and could attempt to use or update the already-cleared cache values. Consider incrementing the reset generation counter before clearing the cache to ensure proper synchronization.

Copilot uses AI. Check for mistakes.

dev_info(&state->client->dev, "%s(): HW reset command sent, waiting for device...\n",
__func__);
Expand Down Expand Up @@ -5739,6 +5774,7 @@ static int ds5_probe(struct i2c_client *c, const struct i2c_device_id *id)
mutex_init(&state->lock);

state->client = c;
state->reset_gen = atomic_read(&ds5_reset_gen);
dev_warn(&c->dev, "Probing driver for D4xx\n");
#ifdef CONFIG_OF
ret = of_property_read_u32(c->dev.of_node, "override_reg", &override_addr);
Expand Down