| Message ID | 20241108233415.25782-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | dcb90f13cf79d909d04dacf2cfbd256a4744347f |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Nov 08, 2024 at 11:34:15PM +0000, Kieran Bingham wrote: > If the CameraManager fails to initialise at startup during > CameraManager::Private::init(), then the run() function will prepare the > thread for operations, but the thread will immediately close without > executing any cleanup. > > This can leave instantiated objects such as the EventNotifier registered > by the udev enumerator constructed in a thread which no longer exists. > The destructor of those objects can then fire an assertion that they are > being executed from an incorrect thread while performing their cleanup. > > Ensure that the failure path does not result in reporting thread > ownership assertions by performing cleanup correctly on the > CameraManager thread before it closes. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > This is the result of debugging an awkward error on an unrelated failure > path that caused the CameraManager to fail to construct: > > """ > camera_sensor_legacy.cpp:850 'ar0144 2-0010': The analogue crop rectangle has been defaulted to the active area size > [1:49:26.079788375] [922] WARN RkISP1Agc agc.cpp:46 'AeMeteringMode' parameter not found in tuning file > [1:49:26.079829125] [922] WARN RkISP1Agc agc.cpp:69 No metering modes read from tuning file; defaulting to matrix > [1:49:26.080311125] [921] ERROR Camera camera_manager.cpp:350 Failed to start camera manager: No such file or directory > Failed to start camera manager: No such file or directory > [1:49:26.080547875] [921] ERROR Object object.cpp:252 EventNotifier can't be enabled from another thread > [1:49:26.080582375] [921] FATAL default object.cpp:253 assertion "false" failed in assertThreadBound() > Backtrace: > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera6Object17assertThreadBoundEPKc+0xe4) [0xffffa35fd884] > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifier10setEnabledEb+0x24) [0xffffa3601b34] > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifierD2Ev+0x50) [0xffffa3601bf0] > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifierD0Ev+0x18) [0xffffa3601cb8] > /usr/lib/libcamera.so.0.3(_ZN9libcamera20DeviceEnumeratorUdevD2Ev+0x38) [0xffffa3798698] > /usr/lib/libcamera.so.0.3(_ZN9libcamera20DeviceEnumeratorUdevD0Ev+0x18) [0xffffa3798738] > /usr/lib/libcamera.so.0.3(+0x997cc) [0xffffa36d97cc] > /usr/lib/libcamera.so.0.3(_ZN9libcamera13CameraManagerD1Ev+0x134) [0xffffa36d6f04] > /usr/lib/libcamera.so.0.3(_ZN9libcamera13CameraManagerD0Ev+0x18) [0xffffa36d6f78] > cam(+0x1ba60) [0xaaaad8d9ba60] > cam(+0x8b64) [0xaaaad8d88b64] > /usr/lib/libc.so.6(+0x284b4) [0xffffa30984b4] > /usr/lib/libc.so.6(__libc_start_main+0x9c) [0xffffa309858c] > cam(+0x8fb0) [0xaaaad8d88fb0] > Aborted (core dumped) > """ > > Whilst the error "No such file or directory" reported above was out of > tree code causing the camera manager to fail - libcamera should not then > fail with an assertion after that while shutting down and calling > destructors. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera_manager.cpp | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 6280b2b3e485..a8b36bcea2d4 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -83,8 +83,10 @@ void CameraManager::Private::run() > mutex_.unlock(); > cv_.notify_one(); > > - if (ret < 0) > + if (ret < 0) { > + cleanup(); The cleanup() function can deal with a partial initialization, so this seems fine to me. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > return; > + } > > /* Now start processing events and messages. */ > exec();
Hi Kieran On Fri, Nov 08, 2024 at 11:34:15PM +0000, Kieran Bingham wrote: > If the CameraManager fails to initialise at startup during > CameraManager::Private::init(), then the run() function will prepare the > thread for operations, but the thread will immediately close without > executing any cleanup. > > This can leave instantiated objects such as the EventNotifier registered > by the udev enumerator constructed in a thread which no longer exists. > The destructor of those objects can then fire an assertion that they are > being executed from an incorrect thread while performing their cleanup. > > Ensure that the failure path does not result in reporting thread > ownership assertions by performing cleanup correctly on the > CameraManager thread before it closes. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Seems sane to me! Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > This is the result of debugging an awkward error on an unrelated failure > path that caused the CameraManager to fail to construct: > > """ > camera_sensor_legacy.cpp:850 'ar0144 2-0010': The analogue crop rectangle has been defaulted to the active area size > [1:49:26.079788375] [922] WARN RkISP1Agc agc.cpp:46 'AeMeteringMode' parameter not found in tuning file > [1:49:26.079829125] [922] WARN RkISP1Agc agc.cpp:69 No metering modes read from tuning file; defaulting to matrix > [1:49:26.080311125] [921] ERROR Camera camera_manager.cpp:350 Failed to start camera manager: No such file or directory > Failed to start camera manager: No such file or directory > [1:49:26.080547875] [921] ERROR Object object.cpp:252 EventNotifier can't be enabled from another thread > [1:49:26.080582375] [921] FATAL default object.cpp:253 assertion "false" failed in assertThreadBound() > Backtrace: > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera6Object17assertThreadBoundEPKc+0xe4) [0xffffa35fd884] > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifier10setEnabledEb+0x24) [0xffffa3601b34] > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifierD2Ev+0x50) [0xffffa3601bf0] > /usr/lib/libcamera-base.so.0.3(_ZN9libcamera13EventNotifierD0Ev+0x18) [0xffffa3601cb8] > /usr/lib/libcamera.so.0.3(_ZN9libcamera20DeviceEnumeratorUdevD2Ev+0x38) [0xffffa3798698] > /usr/lib/libcamera.so.0.3(_ZN9libcamera20DeviceEnumeratorUdevD0Ev+0x18) [0xffffa3798738] > /usr/lib/libcamera.so.0.3(+0x997cc) [0xffffa36d97cc] > /usr/lib/libcamera.so.0.3(_ZN9libcamera13CameraManagerD1Ev+0x134) [0xffffa36d6f04] > /usr/lib/libcamera.so.0.3(_ZN9libcamera13CameraManagerD0Ev+0x18) [0xffffa36d6f78] > cam(+0x1ba60) [0xaaaad8d9ba60] > cam(+0x8b64) [0xaaaad8d88b64] > /usr/lib/libc.so.6(+0x284b4) [0xffffa30984b4] > /usr/lib/libc.so.6(__libc_start_main+0x9c) [0xffffa309858c] > cam(+0x8fb0) [0xaaaad8d88fb0] > Aborted (core dumped) > """ > > Whilst the error "No such file or directory" reported above was out of > tree code causing the camera manager to fail - libcamera should not then > fail with an assertion after that while shutting down and calling > destructors. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/camera_manager.cpp | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index 6280b2b3e485..a8b36bcea2d4 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -83,8 +83,10 @@ void CameraManager::Private::run() > mutex_.unlock(); > cv_.notify_one(); > > - if (ret < 0) > + if (ret < 0) { > + cleanup(); > return; > + } > > /* Now start processing events and messages. */ > exec(); > -- > 2.47.0 >
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index 6280b2b3e485..a8b36bcea2d4 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -83,8 +83,10 @@ void CameraManager::Private::run() mutex_.unlock(); cv_.notify_one(); - if (ret < 0) + if (ret < 0) { + cleanup(); return; + } /* Now start processing events and messages. */ exec();