gugelfrei
/
android_frameworks_av
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Add mInterfaceLock back to Camera3Device methods called by RequestThread.

Browse Source 
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 <jchowdhary@google.com>
gugelfrei
Jayant Chowdhary 4 years ago
parent 265e047963
commit 646c31b058
2 changed files with 70 additions and 72 deletions
Show Stats Download Patch File Download Diff File

141
services/camera/libcameraservice/device3/Camera3Device.cpp
Unescape View File

@ -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);

1
services/camera/libcameraservice/device3/Camera3Device.h
Unescape View File

@ -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

Write Preview
Loading…
Cancel
Save