Skip to content
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.

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

camerad: prevent redundant request re-enqueue after frame ID jump #34665

Closed

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Feb 22, 2025

resolve #34663

When a frame id jump occurs, the code clears the request queue and re-enqueues requests via enqueue_req_multi. However, the subsequent check for dropped request ids (request_id > request_id_last + 1) could redundantly trigger another enqueue_req_multi call, this risks overwriting buf_idx, request_id, and sync objects, potentially leading to invalid states, such as waiting on obsolete sync objects or mismanaging buffer slots.

Additionally, this behavior can result in duplicate request IDs being sent to CAM_REQ_MGR_SCHED_REQ. this could overwrite or merge the state of the original request with the new one, causing state corruption. In some cases, if memory is allocated per request and tied to the ID, the original request’s memory may become unreachable, resulting in a memory leak.

The following errors were observed in the cam_log.txt:

system/camerad/cameras/spectra.cc: camera 0 realign
system/camerad/cameras/spectra.cc: camera 0 dropped requests 150 119
system/camerad/cameras/spectra.cc: CAM_SYNC error: id 6 - errno 11 - ret 0 - ioctl_result -110
system/camerad/cameras/spectra.cc: failed to wait for IFE sync: -110 6

1.953526] system/camerad/cameras/spectra.cc:1379 - handle_camera_event: camera 2 realign
[1.953625] system/camerad/cameras/spectra.cc:1379 - handle_camera_event: camera 2 realign
[1.953763] system/camerad/cameras/spectra.cc:257 - clear_req_queue: flushed all req: 100
[1.954976] system/camerad/cameras/spectra.cc:1389 - handle_camera_event: camera 2 dropped requests 19 2
[1.955117] system/camerad/cameras/spectra.cc:1389 - handle_camera_event: camera 2 dropped requests 19 2
[2.029354] MAIN 0 kernel - CAM_ERR: CAM-ICP: cam_icp_mgr_process_msg_frame_process: 1525 failed with error : 10
[2.029481] MAIN 0 kernel - CAM_ERR: CAM-ICP: cam_icp_mgr_process_msg_frame_process: 1525 failed with error : 10
[2.029579] MAIN 0 kernel - CAM_WARN: CAM-CRM: cam_req_mgr_process_add_req: 1708 Unexpected state 2 for slot 1 map 1
[2.029642] MAIN 0 kernel - CAM_WARN: CAM-CRM: cam_req_mgr_process_add_req: 1708 Unexpected state 2 for slot 1 map 1
[2.030021] MAIN 0 kernel - CAM_ERR: CAM-ISP: __cam_isp_ctx_rdi_only_sof_in_bubble_applied: 1638 No wait request
[2.038408] MAIN 0 kernel - CAM_WARN: CAM-CRM: cam_req_mgr_process_add_req: 1708 Unexpected state 2 for slot 0 map 1
[2.038469] MAIN 0 kernel - CAM_WARN: CAM-CRM: cam_req_mgr_process_add_req: 1708 Unexpected state 2 for slot 0 map 1
[2.038530] MAIN 0 kernel - CAM_WARN: CAM-CRM: cam_req_mgr_process_add_req: 1708 Unexpected state 2 for slot 1 map 1

@deanlee deanlee force-pushed the camerad_improve_handle_dropped_requests branch 2 times, most recently from 8cbe130 to 41a09da Compare February 22, 2025 11:21
@deanlee deanlee marked this pull request as ready for review February 22, 2025 12:36
@deanlee deanlee changed the title camerad: improve dropped request handling camerad: prevent redundant request re-enqueue after frame ID jump Feb 22, 2025
@deanlee deanlee force-pushed the camerad_improve_handle_dropped_requests branch from 41a09da to ef51b51 Compare February 22, 2025 12:59
@adeebshihadeh
Copy link
Contributor

@deanlee this doesn't fix SPECTRA_STRESS_TEST=1 LOGPRINT=debug ./camerad. Instead of this, I propose we rewrite the enqueue + clear stuff to be simpler and centralize it. It's currently not easy to reason about.

@deanlee deanlee marked this pull request as draft February 27, 2025 23:50
@deanlee
Copy link
Contributor Author

deanlee commented Feb 27, 2025

@adeebshihadeh How about merging #34684 before refactoring the enqueue and clear logic? It could streamline the refactor and make it easier to follow.

@adeebshihadeh
Copy link
Contributor

So that actually makes a functional change that's subtle. I prefer we do this all in one PR.

@deanlee
Copy link
Contributor Author

deanlee commented Feb 28, 2025

@deanlee this doesn't fix SPECTRA_STRESS_TEST=1 LOGPRINT=debug ./camerad. Instead of this, I propose we rewrite the enqueue + clear stuff to be simpler and centralize it. It's currently not easy to reason about.

My plan is to address this in two steps:

  1. Refactor the enqueue logic to improve clarity and separate responsibilities. camerad: refactor enqueue_buffer #34726
  2. Revise the clear logic for robustness and centralize its handling.

@deanlee deanlee closed this Feb 28, 2025
@deanlee deanlee deleted the camerad_improve_handle_dropped_requests branch February 28, 2025 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

camerad: BPS config failure
2 participants