From 30ab5ed62fecb96078fb8ece767a66ef794bcb12 Mon Sep 17 00:00:00 2001 From: Yin-Chia Yeh Date: Fri, 12 Oct 2018 15:57:04 -0700 Subject: [PATCH] Camera: prevent requestStreamBuffers break IDLE state Test: CTS Bug: 109829698 Change-Id: Ib9092e4e6b7d6cee2fbd96a3209e3b760f9fdaf0 --- .../device3/Camera3Device.cpp | 177 +++++++++++++++--- .../libcameraservice/device3/Camera3Device.h | 71 ++++++- 2 files changed, 226 insertions(+), 22 deletions(-) diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index f829b65eea..fa4e036b15 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -224,6 +224,17 @@ status_t Camera3Device::initializeCommonLocked() { ANDROID_INFO_SUPPORTED_BUFFER_MANAGEMENT_VERSION_HIDL_DEVICE_3_5); } + if (mUseHalBufManager) { + res = mRequestBufferSM.initialize(mStatusTracker); + if (res != OK) { + SET_ERR_L("Unable to start request buffer state machine: %s (%d)", + strerror(-res), res); + mInterface->close(); + mStatusTracker.clear(); + return res; + } + } + /** Start up request queue thread */ mRequestThread = new RequestThread( this, mStatusTracker, mInterface, sessionParamKeys, mUseHalBufManager); @@ -951,6 +962,13 @@ hardware::Return Camera3Device::requestStreamBuffers( return hardware::Void(); } + if (bufReqs.size() > mOutputStreams.size()) { + ALOGE("%s: too many buffer requests (%zu > # of output streams %zu)", + __FUNCTION__, bufReqs.size(), mOutputStreams.size()); + _hidl_cb(BufferRequestStatus::FAILED_ILLEGAL_ARGUMENTS, bufRets); + return hardware::Void(); + } + // Check for repeated streamId for (const auto& bufReq : bufReqs) { if (streamIds.indexOf(bufReq.streamId) != NAME_NOT_FOUND) { @@ -962,19 +980,10 @@ hardware::Return Camera3Device::requestStreamBuffers( streamIds.add(bufReq.streamId); } - // TODO: check we are not configuring streams. If so return FAILED_CONFIGURING - // Probably need to hook CameraDeviceClient::beginConfigure and figure something - // out for API1 client... maybe grab mLock and check mNeedConfig but then we will - // need to wait until mLock is released... - // _hidl_cb(BufferRequestStatus::FAILED_CONFIGURING, bufRets); - // return hardware::Void(); - - // TODO: here we start accessing mOutputStreams, might need mLock, but that - // might block incoming API calls. Not sure how bad is it. - if (bufReqs.size() > mOutputStreams.size()) { - ALOGE("%s: too many buffer requests (%zu > # of output streams %zu)", - __FUNCTION__, bufReqs.size(), mOutputStreams.size()); - _hidl_cb(BufferRequestStatus::FAILED_ILLEGAL_ARGUMENTS, bufRets); + if (!mRequestBufferSM.startRequestBuffer()) { + ALOGE("%s: request buffer disallowed while camera service is configuring", + __FUNCTION__); + _hidl_cb(BufferRequestStatus::FAILED_CONFIGURING, bufRets); return hardware::Void(); } @@ -991,6 +1000,7 @@ hardware::Return Camera3Device::requestStreamBuffers( ALOGE("%s: Output stream id %d not found!", __FUNCTION__, streamId); hardware::hidl_vec emptyBufRets; _hidl_cb(BufferRequestStatus::FAILED_ILLEGAL_ARGUMENTS, emptyBufRets); + mRequestBufferSM.endRequestBuffer(); return hardware::Void(); } @@ -1077,12 +1087,12 @@ hardware::Return Camera3Device::requestStreamBuffers( returnOutputBuffers(streamBuffers.data(), numAllocatedBuffers, 0); } } - // End of mOutputStreams access _hidl_cb(allReqsSucceeds ? BufferRequestStatus::OK : oneReqSucceeds ? BufferRequestStatus::FAILED_PARTIAL : BufferRequestStatus::FAILED_UNKNOWN, bufRets); + mRequestBufferSM.endRequestBuffer(); return hardware::Void(); } @@ -2132,6 +2142,15 @@ status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout) { mStatusWaiters++; + // Notify HAL to start draining. We need to notify the HalInterface layer + // even when the device is already IDLE, so HalInterface can reject incoming + // requestStreamBuffers call. + if (!active && mUseHalBufManager) { + auto streamIds = mOutputStreams.getStreamIds(); + mRequestThread->signalPipelineDrain(streamIds); + mRequestBufferSM.onWaitUntilIdle(); + } + bool stateSeen = false; do { if (active == (mStatus == STATUS_ACTIVE)) { @@ -2139,12 +2158,6 @@ status_t Camera3Device::waitUntilStateThenRelock(bool active, nsecs_t timeout) { break; } - // Notify HAL to start draining - if (!active && mUseHalBufManager) { - auto streamIds = mOutputStreams.getStreamIds(); - mRequestThread->signalPipelineDrain(streamIds); - } - res = mStatusChanged.waitRelative(mLock, timeout); if (res != OK) break; @@ -2889,6 +2902,10 @@ status_t Camera3Device::configureStreamsLocked(int operatingMode, return rc; } + if (mDummyStreamId == NO_STREAM) { + mRequestBufferSM.onStreamsConfigured(); + } + return OK; } @@ -3059,6 +3076,7 @@ void Camera3Device::removeInFlightMapEntryLocked(int idx) { // Indicate idle inFlightMap to the status tracker if (mInFlightMap.size() == 0) { + mRequestBufferSM.onInflightMapEmpty(); // Hold a separate dedicated tracker lock to prevent race with disconnect and also // avoid a deadlock during reprocess requests. Mutex::Autolock l(mTrackerLock); @@ -5097,6 +5115,13 @@ bool Camera3Device::RequestThread::threadLoop() { nsecs_t tRequestEnd = systemTime(SYSTEM_TIME_MONOTONIC); mRequestLatency.add(tRequestStart, tRequestEnd); + if (submitRequestSuccess) { + sp parent = mParent.promote(); + if (parent != nullptr) { + parent->mRequestBufferSM.onRequestSubmitted(); + } + } + if (useFlushLock) { mFlushLock.unlock(); } @@ -5430,7 +5455,8 @@ void Camera3Device::RequestThread::signalPipelineDrain(const std::vector& s Mutex::Autolock pl(mPauseLock); if (mPaused) { - return mInterface->signalPipelineDrain(streamIds); + mInterface->signalPipelineDrain(streamIds); + return; } // If request thread is still busy, wait until paused then notify HAL mNotifyPipelineDrain = true; @@ -5619,6 +5645,10 @@ sp mNotifyPipelineDrain = false; mStreamIdsToBeDrained.clear(); } + sp parent = mParent.promote(); + if (parent != nullptr) { + parent->mRequestBufferSM.onRequestThreadPaused(); + } } // Stop waiting for now and let thread management happen return NULL; @@ -5708,6 +5738,10 @@ bool Camera3Device::RequestThread::waitIfPaused() { mNotifyPipelineDrain = false; mStreamIdsToBeDrained.clear(); } + sp parent = mParent.promote(); + if (parent != nullptr) { + parent->mRequestBufferSM.onRequestThreadPaused(); + } } res = mDoPauseSignal.waitRelative(mPauseLock, kRequestTimeout); @@ -6163,4 +6197,105 @@ bool Camera3Device::PreparerThread::threadLoop() { return true; } +status_t Camera3Device::RequestBufferStateMachine::initialize( + sp statusTracker) { + if (statusTracker == nullptr) { + ALOGE("%s: statusTracker is null", __FUNCTION__); + return BAD_VALUE; + } + + std::lock_guard lock(mLock); + mStatusTracker = statusTracker; + mRequestBufferStatusId = statusTracker->addComponent(); + return OK; +} + +bool Camera3Device::RequestBufferStateMachine::startRequestBuffer() { + std::lock_guard lock(mLock); + if (mStatus == RB_STATUS_READY) { + mRequestBufferOngoing = true; + return true; + } + return false; +} + +void Camera3Device::RequestBufferStateMachine::endRequestBuffer() { + std::lock_guard lock(mLock); + if (!mRequestBufferOngoing) { + ALOGE("%s called without a successful startRequestBuffer call first!", __FUNCTION__); + return; + } + mRequestBufferOngoing = false; + if (mStatus == RB_STATUS_PENDING_STOP) { + checkSwitchToStopLocked(); + } +} + +void Camera3Device::RequestBufferStateMachine::onStreamsConfigured() { + std::lock_guard lock(mLock); + RequestBufferState oldStatus = mStatus; + mStatus = RB_STATUS_READY; + if (oldStatus != RB_STATUS_READY) { + notifyTrackerLocked(/*active*/true); + } + return; +} + +void Camera3Device::RequestBufferStateMachine::onRequestSubmitted() { + std::lock_guard lock(mLock); + mRequestThreadPaused = false; + mInflightMapEmpty = false; + if (mStatus == RB_STATUS_STOPPED) { + mStatus = RB_STATUS_READY; + notifyTrackerLocked(/*active*/true); + } + return; +} + +void Camera3Device::RequestBufferStateMachine::onRequestThreadPaused() { + std::lock_guard lock(mLock); + mRequestThreadPaused = true; + if (mStatus == RB_STATUS_PENDING_STOP) { + checkSwitchToStopLocked(); + } + return; +} + +void Camera3Device::RequestBufferStateMachine::onInflightMapEmpty() { + std::lock_guard lock(mLock); + mInflightMapEmpty = true; + if (mStatus == RB_STATUS_PENDING_STOP) { + checkSwitchToStopLocked(); + } + return; +} + +void Camera3Device::RequestBufferStateMachine::onWaitUntilIdle() { + std::lock_guard lock(mLock); + if (!checkSwitchToStopLocked()) { + mStatus = RB_STATUS_PENDING_STOP; + } + return; +} + +void Camera3Device::RequestBufferStateMachine::notifyTrackerLocked(bool active) { + sp statusTracker = mStatusTracker.promote(); + if (statusTracker != nullptr) { + if (active) { + statusTracker->markComponentActive(mRequestBufferStatusId); + } else { + statusTracker->markComponentIdle(mRequestBufferStatusId, Fence::NO_FENCE); + } + } +} + +bool Camera3Device::RequestBufferStateMachine::checkSwitchToStopLocked() { + if (mInflightMapEmpty && mRequestThreadPaused && !mRequestBufferOngoing) { + mStatus = RB_STATUS_STOPPED; + notifyTrackerLocked(/*active*/false); + return true; + } + return false; +} + }; // namespace android diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index 5c0f570ad8..4f5be6bd7a 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/services/camera/libcameraservice/device3/Camera3Device.h @@ -1249,12 +1249,81 @@ class Camera3Device : // b/79972865 Mutex mTrackerLock; - // Whether HAL request buffers through requestStreamBuffer API + // Whether HAL request buffers through requestStreamBuffers API bool mUseHalBufManager = false; // Lock to ensure requestStreamBuffers() callbacks are serialized std::mutex mRequestBufferInterfaceLock; + // The state machine to control when requestStreamBuffers should allow + // HAL to request buffers. + enum RequestBufferState { + /** + * This is the initial state. + * requestStreamBuffers call will return FAILED_CONFIGURING in this state. + * Will switch to RB_STATUS_READY after a successful configureStreams or + * processCaptureRequest call. + */ + RB_STATUS_STOPPED, + + /** + * requestStreamBuffers call will proceed in this state. + * When device is asked to stay idle via waitUntilStateThenRelock() call: + * - Switch to RB_STATUS_STOPPED if there is no inflight requests and + * request thread is paused. + * - Switch to RB_STATUS_PENDING_STOP otherwise + */ + RB_STATUS_READY, + + /** + * requestStreamBuffers call will proceed in this state. + * Switch to RB_STATUS_STOPPED when all inflight requests are fulfilled + * and request thread is paused + */ + RB_STATUS_PENDING_STOP, + }; + + class RequestBufferStateMachine { + public: + status_t initialize(sp statusTracker); + + // Return if the state machine currently allows for requestBuffers + // If the state allows for it, mRequestBufferOngoing will be set to true + // and caller must call endRequestBuffer() later to unset the flag + bool startRequestBuffer(); + void endRequestBuffer(); + + // Events triggered by application API call + void onStreamsConfigured(); + void onWaitUntilIdle(); + + // Events usually triggered by hwBinder processCaptureResult callback thread + // But can also be triggered on request thread for failed request, or on + // hwbinder notify callback thread for shutter/error callbacks + void onInflightMapEmpty(); + + // Events triggered by RequestThread + void onRequestSubmitted(); + void onRequestThreadPaused(); + + private: + void notifyTrackerLocked(bool active); + + // Switch to STOPPED state and return true if all conditions allows for it. + // Otherwise do nothing and return false. + bool checkSwitchToStopLocked(); + + std::mutex mLock; + RequestBufferState mStatus = RB_STATUS_STOPPED; + + bool mRequestThreadPaused = true; + bool mInflightMapEmpty = true; + bool mRequestBufferOngoing = false; + + wp mStatusTracker; + int mRequestBufferStatusId; + } mRequestBufferSM; + }; // class Camera3Device }; // namespace android