Merge "Camera: use finer lock in external camera OutputThread" into pi-dev

2 files changed, 77 insertions, 34 deletions

diff --git a/camera/device/3.4/default/ExternalCameraDeviceSession.cpp b/camera/device/3.4/default/ExternalCameraDeviceSession.cpp
index 2c03d81f2..bb11d7cd8 100644
--- a/camera/device/3.4/default/ExternalCameraDeviceSession.cpp
+++ b/camera/device/3.4/default/ExternalCameraDeviceSession.cpp
@@ -51,10 +51,12 @@ constexpr int MAX_RETRY = 15; // Allow retry some ioctl failures a few times to
                              // webcam showing temporarily ioctl failures.
 constexpr int IOCTL_RETRY_SLEEP_US = 33000; // 33ms * MAX_RETRY = 5 seconds
 
+// Constants for tryLock during dumpstate
+static constexpr int kDumpLockRetries = 50;
+static constexpr int kDumpLockSleep = 60000;
+
 bool tryLock(Mutex& mutex)
 {
-    static const int kDumpLockRetries = 50;
-    static const int kDumpLockSleep = 60000;
     bool locked = false;
     for (int i = 0; i < kDumpLockRetries; ++i) {
         if (mutex.tryLock() == NO_ERROR) {
@@ -66,6 +68,19 @@ bool tryLock(Mutex& mutex)
     return locked;
 }
 
+bool tryLock(std::mutex& mutex)
+{
+    bool locked = false;
+    for (int i = 0; i < kDumpLockRetries; ++i) {
+        if (mutex.try_lock()) {
+            locked = true;
+            break;
+        }
+        usleep(kDumpLockSleep);
+    }
+    return locked;
+}
+
 } // Anonymous namespace
 
 // Static instances
@@ -163,7 +178,6 @@ void ExternalCameraDeviceSession::dumpState(const native_handle_t* handle) {
     bool streaming = false;
     size_t v4L2BufferCount = 0;
     SupportedV4L2Format streamingFmt;
-    std::unordered_set<uint32_t>  inflightFrames;
     {
         bool sessionLocked = tryLock(mLock);
         if (!sessionLocked) {
@@ -172,12 +186,25 @@ void ExternalCameraDeviceSession::dumpState(const native_handle_t* handle) {
         streaming = mV4l2Streaming;
         streamingFmt = mV4l2StreamingFmt;
         v4L2BufferCount = mV4L2BufferCount;
-        inflightFrames = mInflightFrames;
+
         if (sessionLocked) {
             mLock.unlock();
         }
     }
 
+    std::unordered_set<uint32_t>  inflightFrames;
+    {
+        bool iffLocked = tryLock(mInflightFramesLock);
+        if (!iffLocked) {
+            dprintf(fd,
+                    "!! ExternalCameraDeviceSession mInflightFramesLock may be deadlocked !!\n");
+        }
+        inflightFrames = mInflightFrames;
+        if (iffLocked) {
+            mInflightFramesLock.unlock();
+        }
+    }
+
     dprintf(fd, "External camera %s V4L2 FD %d, cropping type %s, %s\n",
             mCameraId.c_str(), mV4l2Fd.get(),
             (mCroppingType == VERTICAL) ? "vertical" : "horizontal",
@@ -457,6 +484,7 @@ void ExternalCameraDeviceSession::cleanupInflightFences(
 }
 
 int ExternalCameraDeviceSession::waitForV4L2BufferReturnLocked(std::unique_lock<std::mutex>& lk) {
+    ATRACE_CALL();
     std::chrono::seconds timeout = std::chrono::seconds(kBufferWaitTimeoutSec);
     mLock.unlock();
     auto st = mV4L2BufferReturned.wait_for(lk, timeout);
@@ -612,7 +640,10 @@ Status ExternalCameraDeviceSession::processOneCaptureRequest(const CaptureReques
         halBuf.acquireFence = allFences[i];
         halBuf.fenceTimeout = false;
     }
-    mInflightFrames.insert(halReq->frameNumber);
+    {
+        std::lock_guard<std::mutex> lk(mInflightFramesLock);
+        mInflightFrames.insert(halReq->frameNumber);
+    }
     // Send request to OutputThread for the rest of processing
     mOutputThread->submitRequest(halReq);
     mFirstRequest = false;
@@ -670,7 +701,7 @@ Status ExternalCameraDeviceSession::processCaptureRequestError(
 
     // update inflight records
     {
-        Mutex::Autolock _l(mLock);
+        std::lock_guard<std::mutex> lk(mInflightFramesLock);
         mInflightFrames.erase(req->frameNumber);
     }
 
@@ -724,7 +755,7 @@ Status ExternalCameraDeviceSession::processCaptureResult(std::shared_ptr<HalRequ
 
     // update inflight records
     {
-        Mutex::Autolock _l(mLock);
+        std::lock_guard<std::mutex> lk(mInflightFramesLock);
         mInflightFrames.erase(req->frameNumber);
     }
 
@@ -2285,6 +2316,7 @@ sp<V4L2Frame> ExternalCameraDeviceSession::dequeueV4l2FrameLocked(/*out*/nsecs_t
         }
     }
 
+    ATRACE_BEGIN("VIDIOC_DQBUF");
     v4l2_buffer buffer{};
     buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
     buffer.memory = V4L2_MEMORY_MMAP;
@@ -2292,6 +2324,7 @@ sp<V4L2Frame> ExternalCameraDeviceSession::dequeueV4l2FrameLocked(/*out*/nsecs_t
         ALOGE("%s: DQBUF fails: %s", __FUNCTION__, strerror(errno));
         return ret;
     }
+    ATRACE_END();
 
     if (buffer.index >= mV4L2BufferCount) {
         ALOGE("%s: Invalid buffer id: %d", __FUNCTION__, buffer.index);
@@ -2329,21 +2362,18 @@ sp<V4L2Frame> ExternalCameraDeviceSession::dequeueV4l2FrameLocked(/*out*/nsecs_t
 
 void ExternalCameraDeviceSession::enqueueV4l2Frame(const sp<V4L2Frame>& frame) {
     ATRACE_CALL();
-    {
-        // Release mLock before acquiring mV4l2BufferLock to avoid potential
-        // deadlock
-        Mutex::Autolock _l(mLock);
-        frame->unmap();
-        v4l2_buffer buffer{};
-        buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-        buffer.memory = V4L2_MEMORY_MMAP;
-        buffer.index = frame->mBufferIndex;
-        if (TEMP_FAILURE_RETRY(ioctl(mV4l2Fd.get(), VIDIOC_QBUF, &buffer)) < 0) {
-            ALOGE("%s: QBUF index %d fails: %s", __FUNCTION__,
-                    frame->mBufferIndex, strerror(errno));
-            return;
-        }
+    frame->unmap();
+    ATRACE_BEGIN("VIDIOC_QBUF");
+    v4l2_buffer buffer{};
+    buffer.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+    buffer.memory = V4L2_MEMORY_MMAP;
+    buffer.index = frame->mBufferIndex;
+    if (TEMP_FAILURE_RETRY(ioctl(mV4l2Fd.get(), VIDIOC_QBUF, &buffer)) < 0) {
+        ALOGE("%s: QBUF index %d fails: %s", __FUNCTION__,
+                frame->mBufferIndex, strerror(errno));
+        return;
     }
+    ATRACE_END();
 
     {
         std::lock_guard<std::mutex> lk(mV4l2BufferLock);
@@ -2396,13 +2426,17 @@ Status ExternalCameraDeviceSession::configureStreams(
         return status;
     }
 
-    Mutex::Autolock _l(mLock);
-    if (!mInflightFrames.empty()) {
-        ALOGE("%s: trying to configureStreams while there are still %zu inflight frames!",
-                __FUNCTION__, mInflightFrames.size());
-        return Status::INTERNAL_ERROR;
+
+    {
+        std::lock_guard<std::mutex> lk(mInflightFramesLock);
+        if (!mInflightFrames.empty()) {
+            ALOGE("%s: trying to configureStreams while there are still %zu inflight frames!",
+                    __FUNCTION__, mInflightFrames.size());
+            return Status::INTERNAL_ERROR;
+        }
     }
 
+    Mutex::Autolock _l(mLock);
     // Add new streams
     for (const auto& stream : config.streams) {
         if (mStreamMap.count(stream.id) == 0) {
@@ -2702,14 +2736,17 @@ status_t ExternalCameraDeviceSession::fillCaptureResult(
     const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
     UPDATE(md, ANDROID_CONTROL_AE_LOCK, &ae_lock, 1);
 
-    bool afTrigger = mAfTrigger;
-    if (md.exists(ANDROID_CONTROL_AF_TRIGGER)) {
-        Mutex::Autolock _l(mLock);
-        camera_metadata_entry entry = md.find(ANDROID_CONTROL_AF_TRIGGER);
-        if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_START) {
-            mAfTrigger = afTrigger = true;
-        } else if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_CANCEL) {
-            mAfTrigger = afTrigger = false;
+    bool afTrigger = false;
+    {
+        std::lock_guard<std::mutex> lk(mAfTriggerLock);
+        afTrigger = mAfTrigger;
+        if (md.exists(ANDROID_CONTROL_AF_TRIGGER)) {
+            camera_metadata_entry entry = md.find(ANDROID_CONTROL_AF_TRIGGER);
+            if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_START) {
+                mAfTrigger = afTrigger = true;
+            } else if (entry.data.u8[0] == ANDROID_CONTROL_AF_TRIGGER_CANCEL) {
+                mAfTrigger = afTrigger = false;
+            }
         }
     }
 
diff --git a/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h b/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h
index 64134c566..14e5c9a4a 100644
--- a/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h
+++ b/camera/device/3.4/default/include/ext_device_v3_4_impl/ExternalCameraDeviceSession.h
@@ -300,6 +300,9 @@ protected:
     const std::vector<SupportedV4L2Format> mSupportedFormats;
     const CroppingType mCroppingType;
     const std::string& mCameraId;
+
+    // Not protected by mLock, this is almost a const.
+    // Setup in constructor, reset in close() after OutputThread is joined
     unique_fd mV4l2Fd;
 
     // device is closed either
@@ -327,6 +330,8 @@ protected:
 
     // Stream ID -> Camera3Stream cache
     std::unordered_map<int, Stream> mStreamMap;
+
+    std::mutex mInflightFramesLock; // protect mInflightFrames
     std::unordered_set<uint32_t>  mInflightFrames;
 
     // buffers currently circulating between HAL and camera service
@@ -338,6 +343,7 @@ protected:
     // Stream ID -> circulating buffers map
     std::map<int, CirculatingBuffers> mCirculatingBuffers;
 
+    std::mutex mAfTriggerLock; // protect mAfTrigger
     bool mAfTrigger = false;
 
     static HandleImporter sHandleImporter;