Merge changes from topic "cherrypicker-L89600000960032981:N30900001359241375" into udc-dev
* changes:
Skip setting the state for paused binder thread invocations in waitUntilStateThenRelock
Revert "Add mInterfaceLock back to Camera3Device methods called by RequestThread."
diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp
index 1643212..f5f50a5 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.cpp
+++ b/services/camera/libcameraservice/device3/Camera3Device.cpp
@@ -265,113 +265,110 @@
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 il(mInterfaceLock);
- nsecs_t maxExpectedDuration = getExpectedInFlightDuration();
- {
- Mutex::Autolock l(mLock);
- if (mStatus == STATUS_UNINITIALIZED) return res;
+ Mutex::Autolock l(mLock);
+ if (mStatus == STATUS_UNINITIALIZED) return res;
- if (mRequestThread != NULL) {
- if (mStatus == STATUS_ACTIVE || mStatus == STATUS_ERROR) {
- res = mRequestThread->clear();
+ if (mRequestThread != NULL) {
+ if (mStatus == STATUS_ACTIVE || mStatus == STATUS_ERROR) {
+ res = mRequestThread->clear();
+ 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,
+ /*requestThreadInvocation*/ false);
if (res != OK) {
- SET_ERR_L("Can't stop streaming");
+ SET_ERR_L("Timeout waiting for HAL to drain (% " PRIi64 " ns)",
+ maxExpectedDuration);
// 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
- }
}
}
- // Signal to request thread that we're not expecting any
- // more requests. This will be true since once we're in
- // disconnect and we've cleared off the request queue, the
- // request thread can't receive any new requests through
- // binder calls - since disconnect holds
- // mBinderSerialization lock.
- mRequestThread->setRequestClearing();
}
+ // Signal to request thread that we're not expecting any
+ // more requests. This will be true since once we're in
+ // disconnect and we've cleared off the request queue, the
+ // request thread can't receive any new requests through
+ // binder calls - since disconnect holds
+ // mBinderSerialization lock.
+ mRequestThread->setRequestClearing();
+ }
- 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 and mInterfaceLock, otherwise deadlocks may ensue
- // as the threads try to access parent state (b/143513518)
+
+ // Joining done without holding mLock, otherwise deadlocks may ensue
+ // as the threads try to access parent state
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();
+ }
+
+ if (mInjectionMethods->isInjecting()) {
+ mInjectionMethods->stopInjection();
+ }
+
+ HalInterface* interface;
{
- Mutex::Autolock il(mInterfaceLock);
- if (mStatusTracker != NULL) {
- mStatusTracker->join();
- }
+ Mutex::Autolock l(mLock);
+ mRequestThread.clear();
+ Mutex::Autolock stLock(mTrackerLock);
+ mStatusTracker.clear();
+ interface = mInterface.get();
+ }
- if (mInjectionMethods->isInjecting()) {
- mInjectionMethods->stopInjection();
- }
+ // Call close without internal mutex held, as the HAL close may need to
+ // wait on assorted callbacks,etc, to complete before it can return.
+ mCameraServiceWatchdog->WATCH(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.
- mCameraServiceWatchdog->WATCH(interface->close());
+ {
+ Mutex::Autolock l(mLock);
+ mInterface->clear();
+ mOutputStreams.clear();
+ mInputStream.clear();
+ mDeletedStreams.clear();
+ mBufferManager.clear();
+ internalUpdateStatusLocked(STATUS_UNINITIALIZED);
+ }
- flushInflightRequests();
-
- {
- 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);
- }
+ 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__);
@@ -843,7 +840,7 @@
}
if (res == OK) {
- waitUntilStateThenRelock(/*active*/true, kActiveTimeout);
+ waitUntilStateThenRelock(/*active*/true, kActiveTimeout, /*requestThreadInvocation*/false);
if (res != OK) {
SET_ERR_L("Can't transition to active in %f seconds!",
kActiveTimeout/1e9);
@@ -968,7 +965,8 @@
break;
case STATUS_ACTIVE:
ALOGV("%s: Stopping activity to reconfigure streams", __FUNCTION__);
- res = internalPauseAndWaitLocked(maxExpectedDuration);
+ res = internalPauseAndWaitLocked(maxExpectedDuration,
+ /*requestThreadInvocation*/ false);
if (res != OK) {
SET_ERR_L("Can't pause captures to reconfigure streams!");
return res;
@@ -1085,7 +1083,8 @@
break;
case STATUS_ACTIVE:
ALOGV("%s: Stopping activity to reconfigure streams", __FUNCTION__);
- res = internalPauseAndWaitLocked(maxExpectedDuration);
+ res = internalPauseAndWaitLocked(maxExpectedDuration,
+ /*requestThreadInvocation*/ false);
if (res != OK) {
SET_ERR_L("Can't pause captures to reconfigure streams!");
return res;
@@ -1558,7 +1557,8 @@
}
ALOGV("%s: Camera %s: Waiting until idle (%" PRIi64 "ns)", __FUNCTION__, mId.string(),
maxExpectedDuration);
- status_t res = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration);
+ status_t res = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration,
+ /*requestThreadInvocation*/ false);
if (res != OK) {
mStatusTracker->dumpActiveComponents();
SET_ERR_L("Error waiting for HAL to drain: %s (%d)", strerror(-res),
@@ -1569,12 +1569,14 @@
void Camera3Device::internalUpdateStatusLocked(Status status) {
mStatus = status;
- mRecentStatusUpdates.add(mStatus);
+ mStatusIsInternal = mPauseStateNotify ? true : false;
+ mRecentStatusUpdates.add({mStatus, mStatusIsInternal});
mStatusChanged.broadcast();
}
// Pause to reconfigure
-status_t Camera3Device::internalPauseAndWaitLocked(nsecs_t maxExpectedDuration) {
+status_t Camera3Device::internalPauseAndWaitLocked(nsecs_t maxExpectedDuration,
+ bool requestThreadInvocation) {
if (mRequestThread.get() != nullptr) {
mRequestThread->setPaused(true);
} else {
@@ -1583,7 +1585,8 @@
ALOGV("%s: Camera %s: Internal wait until idle (% " PRIi64 " ns)", __FUNCTION__, mId.string(),
maxExpectedDuration);
- status_t res = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration);
+ status_t res = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration,
+ requestThreadInvocation);
if (res != OK) {
mStatusTracker->dumpActiveComponents();
SET_ERR_L("Can't idle device in %f seconds!",
@@ -1601,7 +1604,9 @@
ALOGV("%s: Camera %s: Internal wait until active (% " PRIi64 " ns)", __FUNCTION__, mId.string(),
kActiveTimeout);
- res = waitUntilStateThenRelock(/*active*/ true, kActiveTimeout);
+ // internalResumeLocked is always called from a binder thread.
+ res = waitUntilStateThenRelock(/*active*/ true, kActiveTimeout,
+ /*requestThreadInvocation*/ false);
if (res != OK) {
SET_ERR_L("Can't transition to active in %f seconds!",
kActiveTimeout/1e9);
@@ -1610,7 +1615,8 @@
return OK;
}
-status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout) {
+status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout,
+ bool requestThreadInvocation) {
status_t res = OK;
size_t startIndex = 0;
@@ -1639,7 +1645,8 @@
bool stateSeen = false;
nsecs_t startTime = systemTime();
do {
- if (active == (mStatus == STATUS_ACTIVE)) {
+ if (active == (mStatus == STATUS_ACTIVE) &&
+ (requestThreadInvocation || !mStatusIsInternal)) {
// Desired state is current
break;
}
@@ -1661,9 +1668,14 @@
"%s: Skipping status updates in Camera3Device, may result in deadlock.",
__FUNCTION__);
- // Encountered desired state since we began waiting
+ // Encountered desired state since we began waiting. Internal invocations coming from
+ // request threads (such as reconfigureCamera) should be woken up immediately, whereas
+ // invocations from binder threads (such as createInputStream) should only be woken up if
+ // they are not paused. This avoids intermediate pause signals from reconfigureCamera as it
+ // changes the status to active right after.
for (size_t i = startIndex; i < mRecentStatusUpdates.size(); i++) {
- if (active == (mRecentStatusUpdates[i] == STATUS_ACTIVE) ) {
+ if (active == (mRecentStatusUpdates[i].status == STATUS_ACTIVE) &&
+ (requestThreadInvocation || !mRecentStatusUpdates[i].isInternal)) {
stateSeen = true;
break;
}
@@ -2310,7 +2322,9 @@
nsecs_t startTime = systemTime();
- Mutex::Autolock il(mInterfaceLock);
+ // We must not hold mInterfaceLock here since this function is called from
+ // RequestThread::threadLoop and holding mInterfaceLock could lead to
+ // deadlocks (http://b/143513518)
nsecs_t maxExpectedDuration = getExpectedInFlightDuration();
Mutex::Autolock l(mLock);
@@ -2325,8 +2339,18 @@
if (mStatus == STATUS_ACTIVE) {
markClientActive = true;
mPauseStateNotify = true;
+ mStatusTracker->markComponentIdle(clientStatusId, Fence::NO_FENCE);
- rc = internalPauseAndWaitLocked(maxExpectedDuration);
+ // This is essentially the same as calling rc = internalPauseAndWaitLocked(..), except that
+ // we don't want to call setPaused(true) to avoid it interfering with setPaused() called
+ // from createInputStream/createStream.
+ rc = waitUntilStateThenRelock(/*active*/ false, maxExpectedDuration,
+ /*requestThreadInvocation*/ true);
+ if (rc != OK) {
+ mStatusTracker->dumpActiveComponents();
+ SET_ERR_L("Can't idle device in %f seconds!",
+ maxExpectedDuration/1e9);
+ }
}
if (rc == NO_ERROR) {
@@ -3567,13 +3591,8 @@
if (res == OK) {
sp<Camera3Device> parent = mParent.promote();
if (parent != nullptr) {
- sp<StatusTracker> statusTracker = mStatusTracker.promote();
- if (statusTracker != nullptr) {
- statusTracker->markComponentIdle(mStatusId, Fence::NO_FENCE);
- }
mReconfigured |= parent->reconfigureCamera(mLatestSessionParams, mStatusId);
}
- setPaused(false);
if (mNextRequests[0].captureRequest->mInputStream != nullptr) {
mNextRequests[0].captureRequest->mInputStream->restoreConfiguredState();
diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h
index 526728c..b1dd135 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.h
+++ b/services/camera/libcameraservice/device3/Camera3Device.h
@@ -366,6 +366,7 @@
// 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
@@ -571,8 +572,15 @@
STATUS_ACTIVE
} mStatus;
+ struct StatusInfo {
+ Status status;
+ bool isInternal; // status triggered by internal reconfigureCamera.
+ };
+
+ bool mStatusIsInternal;
+
// Only clear mRecentStatusUpdates, mStatusWaiters from waitUntilStateThenRelock
- Vector<Status> mRecentStatusUpdates;
+ Vector<StatusInfo> mRecentStatusUpdates;
int mStatusWaiters;
Condition mStatusChanged;
@@ -717,7 +725,8 @@
* CameraDeviceBase interface we shouldn't need to.
* Must be called with mLock and mInterfaceLock both held.
*/
- status_t internalPauseAndWaitLocked(nsecs_t maxExpectedDuration);
+ status_t internalPauseAndWaitLocked(nsecs_t maxExpectedDuration,
+ bool requestThreadInvocation);
/**
* Resume work after internalPauseAndWaitLocked()
@@ -736,7 +745,8 @@
* During the wait mLock is released.
*
*/
- status_t waitUntilStateThenRelock(bool active, nsecs_t timeout);
+ status_t waitUntilStateThenRelock(bool active, nsecs_t timeout,
+ bool requestThreadInvocation);
/**
* Implementation of waitUntilDrained. On success, will transition to IDLE state.
diff --git a/services/camera/libcameraservice/device3/Camera3DeviceInjectionMethods.cpp b/services/camera/libcameraservice/device3/Camera3DeviceInjectionMethods.cpp
index 031c255..ab772d4 100644
--- a/services/camera/libcameraservice/device3/Camera3DeviceInjectionMethods.cpp
+++ b/services/camera/libcameraservice/device3/Camera3DeviceInjectionMethods.cpp
@@ -58,7 +58,8 @@
if (parent->mStatus == STATUS_ACTIVE) {
ALOGV("%s: Let the device be IDLE and the request thread is paused",
__FUNCTION__);
- res = parent->internalPauseAndWaitLocked(maxExpectedDuration);
+ res = parent->internalPauseAndWaitLocked(maxExpectedDuration,
+ /*requestThreadInvocation*/false);
if (res != OK) {
ALOGE("%s: Can't pause captures to inject camera!", __FUNCTION__);
return res;
@@ -117,7 +118,8 @@
if (parent->mStatus == STATUS_ACTIVE) {
ALOGV("%s: Let the device be IDLE and the request thread is paused",
__FUNCTION__);
- res = parent->internalPauseAndWaitLocked(maxExpectedDuration);
+ res = parent->internalPauseAndWaitLocked(maxExpectedDuration,
+ /*requestThreadInvocation*/false);
if (res != OK) {
ALOGE("%s: Can't pause captures to stop injection!", __FUNCTION__);
return res;