Add mInterfaceLock back to Camera3Device methods called by RequestThread.
Methods like reconfigureCamera called by the RequestThread internally
call methods which might expect to have mInterfaceLock locked
(eg: configureStreamsLocked).
Also, function calls which expect to be serialized w.r.t
reconfigureCamera can cause reported camera status to be inconsistent
w.r.t the expected status.
For example the following situation might occur
through CameraTest.java#testVideoSnapshot :
For certain video sizes that would fail to configure the camera
startRecordingL
camera reconfiguration is triggered -> reconfigureCamera() starts
waitUntilDrained -> gets interleaved with reconfigureCamera()
reconfigureCamera() completes
Here the status of the camera device is active instead of CONFIGURED.
updateRecording
createStream() called expects the camera to not be in an ACTIVE
state since updateProcessorStream cannot handle stream
configuration failure gracefully when the device is streaming.
However since waitUntilDrained did not complete after
reconfigureCamera the camera status is STATUS_ACTIVE.
As a result, we should hold mInterfaceMutex while
calling these methods. To avoid deadlocks (b/143513518), we can have the thread
calling disconnect, relinquish mInterfaceMutex right before it starts
waiting on RequestThread to exit. It can re-acquire it when
RequestThread finishes.
Bug: 147313521
Test: atest CameraTest.java#testVideoSnapshot on pixel2 passes
Test: camera CTS
Test: GCA (sanity)
Merged-In: I384f62bc9936d9691dfe9c13ff743e3d07f2ed55
Change-Id: I384f62bc9936d9691dfe9c13ff743e3d07f2ed55
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>
(cherry picked from commit 646c31b058e25dd8ee44ff4ec5a5c3005c8c829c)
diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp
index bda35f3..dfe5eb0 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.cpp
+++ b/services/camera/libcameraservice/device3/Camera3Device.cpp
@@ -339,100 +339,103 @@
status_t Camera3Device::disconnectImpl() {
ATRACE_CALL();
- Mutex::Autolock il(mInterfaceLock);
-
ALOGI("%s: E", __FUNCTION__);
status_t res = OK;
std::vector<wp<Camera3StreamInterface>> streams;
- nsecs_t maxExpectedDuration = getExpectedInFlightDuration();
{
- Mutex::Autolock l(mLock);
- if (mStatus == STATUS_UNINITIALIZED) return res;
+ Mutex::Autolock il(mInterfaceLock);
+ nsecs_t maxExpectedDuration = getExpectedInFlightDuration();
+ {
+ Mutex::Autolock l(mLock);
+ if (mStatus == STATUS_UNINITIALIZED) return res;
- if (mStatus == STATUS_ACTIVE ||
- (mStatus == STATUS_ERROR && mRequestThread != NULL)) {
- res = mRequestThread->clearRepeatingRequests();
- if (res != OK) {
- SET_ERR_L("Can't stop streaming");
- // Continue to close device even in case of error
- } else {
- res = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration);
+ if (mStatus == STATUS_ACTIVE ||
+ (mStatus == STATUS_ERROR && mRequestThread != NULL)) {
+ res = mRequestThread->clearRepeatingRequests();
if (res != OK) {
- SET_ERR_L("Timeout waiting for HAL to drain (% " PRIi64 " ns)",
- maxExpectedDuration);
+ SET_ERR_L("Can't stop streaming");
// Continue to close device even in case of error
+ } else {
+ res = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration);
+ if (res != OK) {
+ SET_ERR_L("Timeout waiting for HAL to drain (% " PRIi64 " ns)",
+ maxExpectedDuration);
+ // Continue to close device even in case of error
+ }
}
}
- }
- if (mStatus == STATUS_ERROR) {
- CLOGE("Shutting down in an error state");
- }
+ if (mStatus == STATUS_ERROR) {
+ CLOGE("Shutting down in an error state");
+ }
- if (mStatusTracker != NULL) {
- mStatusTracker->requestExit();
- }
+ if (mStatusTracker != NULL) {
+ mStatusTracker->requestExit();
+ }
- if (mRequestThread != NULL) {
- mRequestThread->requestExit();
- }
+ if (mRequestThread != NULL) {
+ mRequestThread->requestExit();
+ }
- streams.reserve(mOutputStreams.size() + (mInputStream != nullptr ? 1 : 0));
- for (size_t i = 0; i < mOutputStreams.size(); i++) {
- streams.push_back(mOutputStreams[i]);
- }
- if (mInputStream != nullptr) {
- streams.push_back(mInputStream);
+ streams.reserve(mOutputStreams.size() + (mInputStream != nullptr ? 1 : 0));
+ for (size_t i = 0; i < mOutputStreams.size(); i++) {
+ streams.push_back(mOutputStreams[i]);
+ }
+ if (mInputStream != nullptr) {
+ streams.push_back(mInputStream);
+ }
}
}
-
- // Joining done without holding mLock, otherwise deadlocks may ensue
- // as the threads try to access parent state
+ // Joining done without holding mLock and mInterfaceLock, otherwise deadlocks may ensue
+ // as the threads try to access parent state (b/143513518)
if (mRequestThread != NULL && mStatus != STATUS_ERROR) {
// HAL may be in a bad state, so waiting for request thread
// (which may be stuck in the HAL processCaptureRequest call)
// could be dangerous.
+ // give up mInterfaceLock here and then lock it again. Could this lead
+ // to other deadlocks
mRequestThread->join();
}
-
- if (mStatusTracker != NULL) {
- mStatusTracker->join();
- }
-
- HalInterface* interface;
{
- Mutex::Autolock l(mLock);
- mRequestThread.clear();
- Mutex::Autolock stLock(mTrackerLock);
- mStatusTracker.clear();
- interface = mInterface.get();
- }
+ Mutex::Autolock il(mInterfaceLock);
+ if (mStatusTracker != NULL) {
+ mStatusTracker->join();
+ }
- // Call close without internal mutex held, as the HAL close may need to
- // wait on assorted callbacks,etc, to complete before it can return.
- interface->close();
+ HalInterface* interface;
+ {
+ Mutex::Autolock l(mLock);
+ mRequestThread.clear();
+ Mutex::Autolock stLock(mTrackerLock);
+ mStatusTracker.clear();
+ interface = mInterface.get();
+ }
- flushInflightRequests();
+ // Call close without internal mutex held, as the HAL close may need to
+ // wait on assorted callbacks,etc, to complete before it can return.
+ interface->close();
- {
- Mutex::Autolock l(mLock);
- mInterface->clear();
- mOutputStreams.clear();
- mInputStream.clear();
- mDeletedStreams.clear();
- mBufferManager.clear();
- internalUpdateStatusLocked(STATUS_UNINITIALIZED);
- }
+ flushInflightRequests();
- for (auto& weakStream : streams) {
- sp<Camera3StreamInterface> stream = weakStream.promote();
- if (stream != nullptr) {
- ALOGE("%s: Stream %d leaked! strong reference (%d)!",
- __FUNCTION__, stream->getId(), stream->getStrongCount() - 1);
+ {
+ Mutex::Autolock l(mLock);
+ mInterface->clear();
+ mOutputStreams.clear();
+ mInputStream.clear();
+ mDeletedStreams.clear();
+ mBufferManager.clear();
+ internalUpdateStatusLocked(STATUS_UNINITIALIZED);
+ }
+
+ for (auto& weakStream : streams) {
+ sp<Camera3StreamInterface> stream = weakStream.promote();
+ if (stream != nullptr) {
+ ALOGE("%s: Stream %d leaked! strong reference (%d)!",
+ __FUNCTION__, stream->getId(), stream->getStrongCount() - 1);
+ }
}
}
-
ALOGI("%s: X", __FUNCTION__);
return res;
}
@@ -2165,9 +2168,7 @@
}
void Camera3Device::pauseStateNotify(bool enable) {
- // We must not hold mInterfaceLock here since this function is called from
- // RequestThread::threadLoop and holding mInterfaceLock could lead to
- // deadlocks (http://b/143513518)
+ Mutex::Autolock il(mInterfaceLock);
Mutex::Autolock l(mLock);
mPauseStateNotify = enable;
@@ -2744,9 +2745,7 @@
ATRACE_CALL();
bool ret = false;
- // We must not hold mInterfaceLock here since this function is called from
- // RequestThread::threadLoop and holding mInterfaceLock could lead to
- // deadlocks (http://b/143513518)
+ Mutex::Autolock il(mInterfaceLock);
nsecs_t maxExpectedDuration = getExpectedInFlightDuration();
Mutex::Autolock l(mLock);