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
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 @@
mIsConstrainedHighSpeedConfiguration(false),
mHal3Device(NULL),
mStatus(STATUS_UNINITIALIZED),
+ mStatusWaiters(0),
mUsePartialResult(false),
mNumPartialResults(1),
mNextResultFrameNumber(0),
@@ -191,7 +192,8 @@
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 @@
mHal3Device = NULL;
}
- mStatus = STATUS_UNINITIALIZED;
+ internalUpdateStatusLocked(STATUS_UNINITIALIZED);
}
ALOGV("%s: X", __FUNCTION__);
@@ -1166,6 +1168,13 @@
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 @@
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 @@
}
} while (!stateSeen);
+ mStatusWaiters--;
+
return res;
}
@@ -1461,9 +1489,7 @@
}
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 @@
// 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 @@
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 @@
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 @@
STATUS_CONFIGURED,
STATUS_ACTIVE
} mStatus;
+
+ // Only clear mRecentStatusUpdates, mStatusWaiters from waitUntilStateThenRelock
Vector<Status> mRecentStatusUpdates;
+ int mStatusWaiters;
+
Condition mStatusChanged;
// Tracking cause of fatal errors when in STATUS_ERROR
@@ -276,6 +280,13 @@
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.