Merge "Add mInterfaceLock back to Camera3Device methods called by RequestThread."

services/camera/libcameraservice/device3/Camera3Device.cpp

@@ -364,100 +364,103 @@ status_t Camera3Device::disconnect() {
 
 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;
 }
@@ -1768,9 +1771,7 @@ void Camera3Device::internalUpdateStatusLocked(Status status) {
 }
 
 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;
@@ -2340,9 +2341,7 @@ bool Camera3Device::reconfigureCamera(const CameraMetadata& sessionParams) {
     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);

services/camera/libcameraservice/device3/Camera3Device.h

@@ -278,7 +278,6 @@ class Camera3Device :
 
     // A lock to enforce serialization on the input/configure side
     // of the public interface.
-    // Only locked by public methods inherited from CameraDeviceBase.
     // Not locked by methods guarded by mOutputLock, since they may act
     // concurrently to the input/configure side of the interface.
     // Must be locked before mLock if both will be locked by a method