diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index 90d21a21e5..f29431c560 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/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> 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 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 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); diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index e13e45fbe2..8b0f68fa10 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/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