From f8e28fb940d725afb26a37c1964fbd1fcdb50fe8 Mon Sep 17 00:00:00 2001 From: Yin-Chia Yeh Date: Thu, 16 May 2019 11:46:54 -0700 Subject: [PATCH] Camera: fix acquire fence FD double close This patch fixes several issues: * Change the timing the acquire fence FD is closed: we used to close the FD when the buffer is returned from HAL. This patch changes that to after the request is sent to HAL (or failed to send to HAL) * Cleanup inflight buffer map if the request fails to be sent to HAL * With the CL, the acquire fence FDs are now closed by - HalInterface::processBatchCaptureRequests if the HIDL processCaptureRequests call succeeds and HAL is running in binderized mode (for passthrough mode the FD is owned by HAL if the HIDL call succeeds) - Camera3Device::cleanupFailedRequests otherwise Test: Camera CTS tests Bug: 132594861 Change-Id: I5f67ae9e7b8008738bd9a24246d754a6a3669b0c --- .../device3/Camera3Device.cpp | 84 ++++++++++++++----- .../libcameraservice/device3/Camera3Device.h | 16 +++- 2 files changed, 73 insertions(+), 27 deletions(-) diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index e18415bba8..306f9b65b7 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -4381,11 +4381,12 @@ status_t Camera3Device::HalInterface::configureStreams(const camera_metadata_t * status_t Camera3Device::HalInterface::wrapAsHidlRequest(camera3_capture_request_t* request, /*out*/device::V3_2::CaptureRequest* captureRequest, - /*out*/std::vector* handlesCreated) { + /*out*/std::vector* handlesCreated, + /*out*/std::vector>* inflightBuffers) { ATRACE_CALL(); - if (captureRequest == nullptr || handlesCreated == nullptr) { - ALOGE("%s: captureRequest (%p) and handlesCreated (%p) must not be null", - __FUNCTION__, captureRequest, handlesCreated); + if (captureRequest == nullptr || handlesCreated == nullptr || inflightBuffers == nullptr) { + ALOGE("%s: captureRequest (%p), handlesCreated (%p), and inflightBuffers(%p) " + "must not be null", __FUNCTION__, captureRequest, handlesCreated, inflightBuffers); return BAD_VALUE; } @@ -4415,8 +4416,8 @@ status_t Camera3Device::HalInterface::wrapAsHidlRequest(camera3_capture_request_ captureRequest->inputBuffer.releaseFence = nullptr; pushInflightBufferLocked(captureRequest->frameNumber, streamId, - request->input_buffer->buffer, - request->input_buffer->acquire_fence); + request->input_buffer->buffer); + inflightBuffers->push_back(std::make_pair(captureRequest->frameNumber, streamId)); } else { captureRequest->inputBuffer.streamId = -1; captureRequest->inputBuffer.bufferId = BUFFER_ID_NO_BUFFER; @@ -4455,14 +4456,31 @@ status_t Camera3Device::HalInterface::wrapAsHidlRequest(camera3_capture_request_ // Output buffers are empty when using HAL buffer manager if (!mUseHalBufManager) { - pushInflightBufferLocked(captureRequest->frameNumber, streamId, - src->buffer, src->acquire_fence); + pushInflightBufferLocked(captureRequest->frameNumber, streamId, src->buffer); + inflightBuffers->push_back(std::make_pair(captureRequest->frameNumber, streamId)); } } } return OK; } +void Camera3Device::HalInterface::cleanupNativeHandles( + std::vector *handles, bool closeFd) { + if (handles == nullptr) { + return; + } + if (closeFd) { + for (auto& handle : *handles) { + native_handle_close(handle); + } + } + for (auto& handle : *handles) { + native_handle_delete(handle); + } + handles->clear(); + return; +} + status_t Camera3Device::HalInterface::processBatchCaptureRequests( std::vector& requests,/*out*/uint32_t* numRequestProcessed) { ATRACE_NAME("CameraHal::processBatchCaptureRequests"); @@ -4483,17 +4501,20 @@ status_t Camera3Device::HalInterface::processBatchCaptureRequests( captureRequests.resize(batchSize); } std::vector handlesCreated; + std::vector> inflightBuffers; status_t res = OK; for (size_t i = 0; i < batchSize; i++) { if (hidlSession_3_4 != nullptr) { res = wrapAsHidlRequest(requests[i], /*out*/&captureRequests_3_4[i].v3_2, - /*out*/&handlesCreated); + /*out*/&handlesCreated, /*out*/&inflightBuffers); } else { - res = wrapAsHidlRequest(requests[i], - /*out*/&captureRequests[i], /*out*/&handlesCreated); + res = wrapAsHidlRequest(requests[i], /*out*/&captureRequests[i], + /*out*/&handlesCreated, /*out*/&inflightBuffers); } if (res != OK) { + popInflightBuffers(inflightBuffers); + cleanupNativeHandles(&handlesCreated); return res; } } @@ -4590,18 +4611,30 @@ status_t Camera3Device::HalInterface::processBatchCaptureRequests( } if (!err.isOk()) { ALOGE("%s: Transaction error: %s", __FUNCTION__, err.description().c_str()); - return DEAD_OBJECT; + status = common::V1_0::Status::CAMERA_DISCONNECTED; } + if (status == common::V1_0::Status::OK && *numRequestProcessed != batchSize) { ALOGE("%s: processCaptureRequest returns OK but processed %d/%zu requests", __FUNCTION__, *numRequestProcessed, batchSize); status = common::V1_0::Status::INTERNAL_ERROR; } - for (auto& handle : handlesCreated) { - native_handle_delete(handle); + res = CameraProviderManager::mapToStatusT(status); + if (res == OK) { + if (mHidlSession->isRemote()) { + // Only close acquire fence FDs when the HIDL transaction succeeds (so the FDs have been + // sent to camera HAL processes) + cleanupNativeHandles(&handlesCreated, /*closeFd*/true); + } else { + // In passthrough mode the FDs are now owned by HAL + cleanupNativeHandles(&handlesCreated); + } + } else { + popInflightBuffers(inflightBuffers); + cleanupNativeHandles(&handlesCreated); } - return CameraProviderManager::mapToStatusT(status); + return res; } status_t Camera3Device::HalInterface::flush() { @@ -4683,10 +4716,9 @@ void Camera3Device::HalInterface::getInflightRequestBufferKeys( } status_t Camera3Device::HalInterface::pushInflightBufferLocked( - int32_t frameNumber, int32_t streamId, buffer_handle_t *buffer, int acquireFence) { + int32_t frameNumber, int32_t streamId, buffer_handle_t *buffer) { uint64_t key = static_cast(frameNumber) << 32 | static_cast(streamId); - auto pair = std::make_pair(buffer, acquireFence); - mInflightBufferMap[key] = pair; + mInflightBufferMap[key] = buffer; return OK; } @@ -4698,16 +4730,22 @@ status_t Camera3Device::HalInterface::popInflightBuffer( uint64_t key = static_cast(frameNumber) << 32 | static_cast(streamId); auto it = mInflightBufferMap.find(key); if (it == mInflightBufferMap.end()) return NAME_NOT_FOUND; - auto pair = it->second; - *buffer = pair.first; - int acquireFence = pair.second; - if (acquireFence > 0) { - ::close(acquireFence); + if (buffer != nullptr) { + *buffer = it->second; } mInflightBufferMap.erase(it); return OK; } +void Camera3Device::HalInterface::popInflightBuffers( + const std::vector>& buffers) { + for (const auto& pair : buffers) { + int32_t frameNumber = pair.first; + int32_t streamId = pair.second; + popInflightBuffer(frameNumber, streamId, nullptr); + } +} + status_t Camera3Device::HalInterface::pushInflightRequestBuffer( uint64_t bufferId, buffer_handle_t* buf, int32_t streamId) { std::lock_guard lock(mRequestedBuffersLock); diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index fe94f8e86d..1e0040c3e2 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/services/camera/libcameraservice/device3/Camera3Device.h @@ -358,13 +358,21 @@ class Camera3Device : // Do not free input camera3_capture_request_t before output HIDL request status_t wrapAsHidlRequest(camera3_capture_request_t* in, /*out*/hardware::camera::device::V3_2::CaptureRequest* out, - /*out*/std::vector* handlesCreated); + /*out*/std::vector* handlesCreated, + /*out*/std::vector>* inflightBuffers); status_t pushInflightBufferLocked(int32_t frameNumber, int32_t streamId, - buffer_handle_t *buffer, int acquireFence); + buffer_handle_t *buffer); + + // Pop inflight buffers based on pairs of (frameNumber,streamId) + void popInflightBuffers(const std::vector>& buffers); + // Cache of buffer handles keyed off (frameNumber << 32 | streamId) - // value is a pair of (buffer_handle_t*, acquire_fence FD) - std::unordered_map> mInflightBufferMap; + std::unordered_map mInflightBufferMap; + + // Delete and optionally close native handles and clear the input vector afterward + static void cleanupNativeHandles( + std::vector *handles, bool closeFd = false); struct BufferHasher { size_t operator()(const buffer_handle_t& buf) const {