From 183f056393423b344e73f388f21d30379a38e519 Mon Sep 17 00:00:00 2001 From: Ruben Brunk Date: Wed, 12 Aug 2015 12:55:02 -0700 Subject: Fix deadlock conditions in Camera3Device. Potential deadlock conditions this addresses, include: - Not waking up waiting threads for several situations where the status had been updated. - Not waking up all waiting thread when status had been updated (only one thread was awoken due to use of signal). - Threads clear status transitions before other waiting threads have a chance to examine them. Bug: 22448586 Change-Id: I53ba669d333a83d2bfa1ca3170d34acc6d8fe6e3 --- .../libcameraservice/device3/Camera3Device.cpp | 63 +++++++++++++++------- .../libcameraservice/device3/Camera3Device.h | 11 ++++ 2 files changed, 54 insertions(+), 20 deletions(-) (limited to 'services/camera') diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index c91517c..f1ccf0b 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -60,6 +60,7 @@ Camera3Device::Camera3Device(int id): mIsConstrainedHighSpeedConfiguration(false), mHal3Device(NULL), mStatus(STATUS_UNINITIALIZED), + mStatusWaiters(0), mUsePartialResult(false), mNumPartialResults(1), mNextResultFrameNumber(0), @@ -191,7 +192,8 @@ status_t Camera3Device::initialize(CameraModule *module) mDeviceVersion = device->common.version; mDeviceInfo = info.static_camera_characteristics; mHal3Device = device; - mStatus = STATUS_UNCONFIGURED; + + internalUpdateStatusLocked(STATUS_UNCONFIGURED); mNextStreamId = 0; mDummyStreamId = NO_STREAM; mNeedConfig = true; @@ -296,7 +298,7 @@ status_t Camera3Device::disconnect() { mHal3Device = NULL; } - mStatus = STATUS_UNINITIALIZED; + internalUpdateStatusLocked(STATUS_UNINITIALIZED); } ALOGV("%s: X", __FUNCTION__); @@ -1166,6 +1168,13 @@ status_t Camera3Device::waitUntilDrainedLocked() { return res; } + +void Camera3Device::internalUpdateStatusLocked(Status status) { + mStatus = status; + mRecentStatusUpdates.add(mStatus); + mStatusChanged.broadcast(); +} + // Pause to reconfigure status_t Camera3Device::internalPauseAndWaitLocked() { mRequestThread->setPaused(true); @@ -1196,23 +1205,40 @@ status_t Camera3Device::internalResumeLocked() { return OK; } -status_t Camera3Device::waitUntilStateThenRelock(bool active, - nsecs_t timeout) { +status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout) { status_t res = OK; - if (active == (mStatus == STATUS_ACTIVE)) { - // Desired state already reached - return res; + + size_t startIndex = 0; + if (mStatusWaiters == 0) { + // Clear the list of recent statuses if there are no existing threads waiting on updates to + // this status list + mRecentStatusUpdates.clear(); + } else { + // If other threads are waiting on updates to this status list, set the position of the + // first element that this list will check rather than clearing the list. + startIndex = mRecentStatusUpdates.size(); } + mStatusWaiters++; + bool stateSeen = false; do { - mRecentStatusUpdates.clear(); + if (active == (mStatus == STATUS_ACTIVE)) { + // Desired state is current + break; + } res = mStatusChanged.waitRelative(mLock, timeout); if (res != OK) break; - // Check state change history during wait - for (size_t i = 0; i < mRecentStatusUpdates.size(); i++) { + // This is impossible, but if not, could result in subtle deadlocks and invalid state + // transitions. + LOG_ALWAYS_FATAL_IF(startIndex > mRecentStatusUpdates.size(), + "%s: Skipping status updates in Camera3Device, may result in deadlock.", + __FUNCTION__); + + // Encountered desired state since we began waiting + for (size_t i = startIndex; i < mRecentStatusUpdates.size(); i++) { if (active == (mRecentStatusUpdates[i] == STATUS_ACTIVE) ) { stateSeen = true; break; @@ -1220,6 +1246,8 @@ status_t Camera3Device::waitUntilStateThenRelock(bool active, } } while (!stateSeen); + mStatusWaiters--; + return res; } @@ -1461,9 +1489,7 @@ void Camera3Device::notifyStatus(bool idle) { } ALOGV("%s: Camera %d: Now %s", __FUNCTION__, mId, idle ? "idle" : "active"); - mStatus = idle ? STATUS_CONFIGURED : STATUS_ACTIVE; - mRecentStatusUpdates.add(mStatus); - mStatusChanged.signal(); + internalUpdateStatusLocked(idle ? STATUS_CONFIGURED : STATUS_ACTIVE); // Skip notifying listener if we're doing some user-transparent // state changes @@ -1672,7 +1698,7 @@ status_t Camera3Device::configureStreamsLocked() { // Return state to that at start of call, so that future configures // properly clean things up - mStatus = STATUS_UNCONFIGURED; + internalUpdateStatusLocked(STATUS_UNCONFIGURED); mNeedConfig = true; ALOGV("%s: Camera %d: Stream configuration failed", __FUNCTION__, mId); @@ -1719,11 +1745,8 @@ status_t Camera3Device::configureStreamsLocked() { mNeedConfig = false; - if (mDummyStreamId == NO_STREAM) { - mStatus = STATUS_CONFIGURED; - } else { - mStatus = STATUS_UNCONFIGURED; - } + internalUpdateStatusLocked((mDummyStreamId == NO_STREAM) ? + STATUS_CONFIGURED : STATUS_UNCONFIGURED); ALOGV("%s: Camera %d: Stream configuration complete", __FUNCTION__, mId); @@ -1831,7 +1854,7 @@ void Camera3Device::setErrorStateLockedV(const char *fmt, va_list args) { mErrorCause = errorCause; mRequestThread->setPaused(true); - mStatus = STATUS_ERROR; + internalUpdateStatusLocked(STATUS_ERROR); // Notify upstream about a device error if (mListener != NULL) { diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index eea34af..0ea9b8b 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/services/camera/libcameraservice/device3/Camera3Device.h @@ -205,7 +205,11 @@ class Camera3Device : STATUS_CONFIGURED, STATUS_ACTIVE } mStatus; + + // Only clear mRecentStatusUpdates, mStatusWaiters from waitUntilStateThenRelock Vector mRecentStatusUpdates; + int mStatusWaiters; + Condition mStatusChanged; // Tracking cause of fatal errors when in STATUS_ERROR @@ -276,6 +280,13 @@ class Camera3Device : virtual CameraMetadata getLatestRequestLocked(); /** + * Update the current device status and wake all waiting threads. + * + * Must be called with mLock held. + */ + void internalUpdateStatusLocked(Status status); + + /** * Pause processing and flush everything, but don't tell the clients. * This is for reconfiguring outputs transparently when according to the * CameraDeviceBase interface we shouldn't need to. -- cgit v1.1