From 646c31b058e25dd8ee44ff4ec5a5c3005c8c829c Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Thu, 30 Jan 2020 13:09:59 -0800 Subject: [PATCH] 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) Change-Id: I384f62bc9936d9691dfe9c13ff743e3d07f2ed55 Signed-off-by: Jayant Chowdhary --- .../device3/Camera3Device.cpp | 141 +++++++++--------- .../libcameraservice/device3/Camera3Device.h | 1 - 2 files changed, 70 insertions(+), 72 deletions(-) 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