[{"id":16870,"web_url":"https://patchwork.libcamera.org/comment/16870/","msgid":"","date":"2021-05-10T20:39:09","subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-05-10 12:52:35 +0200, Jacopo Mondi wrote:\n> Implement the flush() camera operation in the CameraDevice class\n> and make it available to the camera framework by implementing the\n> operation wrapper in camera_ops.cpp.\n> \n> The flush() implementation stops the Camera and the worker thread and\n> waits for all in-flight requests to be returned. Stopping the Camera\n> forces all Requests already queued to be returned immediately in error\n> state. As flush() has to wait until all of them have been returned, make it\n> wait on a newly introduced condition variable which is notified by the\n> request completion handler when the queue of pending requests has been\n> exhausted.\n> \n> As flush() can race with processCaptureRequest() protect the requests\n> queueing by introducing a new CameraState::CameraFlushing state that\n> processCaptureRequest() inspects before queuing the Request to the\n> Camera. If flush() has been called while processCaptureRequest() was\n> executing, return the current Request immediately in error state.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++\n> src/android/camera_device.h | 6 ++++\n> src/android/camera_ops.cpp | 8 ++++-\n> 3 files changed, 76 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index fa12ce5b0133..01b3acd93c4b 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -756,6 +756,42 @@ void CameraDevice::close()\n> \tcamera_->release();\n> }\n> \n> +/*\n> + * Flush is similar to stop() but sets the camera state to 'flushing' and wait\n> + * until all the in-flight requests have been returned.\n> + */\n> +void CameraDevice::flush()\n> +{\n> +\t{\n> +\t\tMutexLocker cameraLock(cameraMutex_);\n> +\n> +\t\tif (state_ != CameraRunning)\n> +\t\t\treturn;\n> +\n> +\t\tstate_ = CameraFlushing;\n> +\n> +\t\t/*\n> +\t\t * Stop the camera and set the state to flushing to prevent any\n> +\t\t * new request to be queued from this point on.\n> +\t\t */\n> +\t\tworker_.stop();\n> +\t\tcamera_->stop();\n> +\t}\n\nReleasing cameraMutex_ while waiting for the flushMutex_ signal seems \nlike a race waiting to happen as the operation is acting in \nsynchronization with the state_ statemachine.\n\nI understand you release it as you want to lock it in \nCameraDevice::stop(), is there any other potential race site? If not \nmaybe CameraDevice::stop() can be reworked? It only needs to read the \nstate so is the mutex really needed, could it be reworked to an atomic? \nOr maybe there is something in the HAL itself that would prevent other \ncallbacks to change the state from CameraFlushing -> CameraRunning while \nflush() is executing?\n\n> +\n> +\t/*\n> +\t * Now wait for all the in-flight requests to be completed before\n> +\t * returning. Stopping the Camera guarantees that all in-flight requests\n> +\t * are completed in error state.\n> +\t */\n> +\t{\n> +\t\tMutexLocker flushLock(flushMutex_);\n> +\t\tflushing_.wait(flushLock, [&] { return descriptors_.empty(); });\n> +\t}\n> +\n> +\tMutexLocker cameraLock(cameraMutex_);\n> +\tstate_ = CameraStopped;\n> +}\n> +\n> void CameraDevice::stop()\n> {\n> \tMutexLocker cameraLock(cameraMutex_);\n> @@ -2019,6 +2055,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\n> +\t/*\n> +\t * Just before queuing the request, make sure flush() has not\n> +\t * been called after this function has been executed. In that\n> +\t * case, immediately return the request with errors.\n> +\t */\n> +\tMutexLocker cameraLock(cameraMutex_);\n> +\tif (state_ == CameraFlushing || state_ == CameraStopped) {\n> +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\tbuffer.release_fence = buffer.acquire_fence;\n> +\t\t}\n> +\n> +\t\tnotifyError(descriptor.frameNumber_,\n> +\t\t\t descriptor.buffers_[0].stream,\n> +\t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> \tworker_.queueRequest(descriptor.request_.get());\n> \n> \t{\n> @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \tCamera3RequestDescriptor &descriptor = node.mapped();\n> \n> +\t/* Release flush if all the pending requests have been completed. */\n> +\t{\n> +\t\tMutexLocker flushLock(flushMutex_);\n> +\t\tif (descriptors_.empty())\n> +\t\t\tflushing_.notify_one();\n> +\t}\n> +\n> \t/*\n> \t * Prepare the capture result for the Android camera stack.\n> \t *\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index ed992ae56d5d..4a9ef495b776 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> #define __ANDROID_CAMERA_DEVICE_H__\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -42,6 +43,7 @@ public:\n> \n> \tint open(const hw_module_t *hardwareModule);\n> \tvoid close();\n> +\tvoid flush();\n> \n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> @@ -92,6 +94,7 @@ private:\n> \tenum State {\n> \t\tCameraStopped,\n> \t\tCameraRunning,\n> +\t\tCameraFlushing,\n> \t};\n> \n> \tvoid stop();\n> @@ -124,6 +127,9 @@ private:\n> \tlibcamera::Mutex cameraMutex_; /* Protects access to the camera state. */\n> \tState state_;\n> \n> +\tlibcamera::Mutex flushMutex_; /* Protects the flushing condition variable. */\n> +\tstd::condition_variable flushing_;\n> +\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \n> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> index 696e80436821..8a3cfa175ff5 100644\n> --- a/src/android/camera_ops.cpp\n> +++ b/src/android/camera_ops.cpp\n> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> {\n> }\n> \n> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> +static int hal_dev_flush(const struct camera3_device *dev)\n> {\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> +\tcamera->flush();\n> +\n> \treturn 0;\n> }\n> \n> -- \n> 2.31.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9C2DCBF839\n\tfor ;\n\tMon, 10 May 2021 20:39:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD73C6891D;\n\tMon, 10 May 2021 22:39:12 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7036F602BE\n\tfor ;\n\tMon, 10 May 2021 22:39:11 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id h4so25414798lfv.0\n\tfor ;\n\tMon, 10 May 2021 13:39:11 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tu16sm2359881lfl.83.2021.05.10.13.39.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 10 May 2021 13:39:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"YNsHLUJp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=L8Lq3SuazCWFbkNhbYhlWP6R276RGoy9v/GRrfS/4LA=;\n\tb=YNsHLUJpKsAF4XqgoHTBdTSIiyj5MW1TAqlsu3ka+8MxPOaawFicAjJZl0fJUYpUrj\n\tdL2l7Yf7ohQZgpCkrYaroiTb8p7kcS2KXq9dm4p2jZDqV1M0sXS+eRwWpvdpXVH1meM0\n\toQcH0I2nFePiUfxiZ3SZX7HhmZ6SsAyUUtJs7wnXCtnjMni1x+KyXaSoN+j/hfZdjOHp\n\tHX8n4k0l8c2gGuG84jGptKGtr7SsNhPj8XBdgBxmjq0m1V9VO40N720hZ+gQ/5sT7y6X\n\tic8v5VEV14KTlzAINhAPb6nB6JEsChkIS1TNEGwdxyKJSAQvG3AQNxARv6SSHegPG7t6\n\tTULw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=L8Lq3SuazCWFbkNhbYhlWP6R276RGoy9v/GRrfS/4LA=;\n\tb=KW5cyMmIxXA/GNpobi4eAvIRwLc8sVhS4t0KCE3wft5qn4UyzZj7Y9IYcckXZhlTti\n\t5OIA6fO6hUjwEwIKBteAX3yrmRsBe9aRR4czVrahni8lfekr8mGbi11uT5qAIGkiLdyj\n\tjE6L8MK0nua/QFQo9fr6YUZ6qLgs7T0bG2d/RewocVLNhgTkh+2F+J7quaXYdfVU75UM\n\tszE2ZvGp+5q7RiCKEilRwe5werWMO7/FknPzjWD6TxMALJdfGvJzXF68tbTM7PsK+VPj\n\tFuRYRAWsg92LAICD2cowmvoEqxdJ2Mv3pTq3IFokYVR/yEZu37I8sJynEiu8S7c51w/G\n\tGSUw==","X-Gm-Message-State":"AOAM530If9EwYjsmfp8qzrci/1WkAQrM1uMP4IIfY1Jl2tHok3vQLusY\n\t1BY0ev14H/PD6Q7NJKM6SQy0yjWqT6BmSw1V","X-Google-Smtp-Source":"ABdhPJzuBh/btGb2reSy3W5Y4zzvhwrMNyLVg9xUqvNYDE3tTTNY8kQYCe10MXMx3DB0/26MDTBCag==","X-Received":"by 2002:a19:ca05:: with SMTP id\n\ta5mr10307690lfg.236.1620679150826; \n\tMon, 10 May 2021 13:39:10 -0700 (PDT)","Date":"Mon, 10 May 2021 22:39:09 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-9-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210510105235.28319-9-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16882,"web_url":"https://patchwork.libcamera.org/comment/16882/","msgid":"","date":"2021-05-11T06:15:17","subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the work.\n\nOn Tue, May 11, 2021 at 5:39 AM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-10 12:52:35 +0200, Jacopo Mondi wrote:\n> > Implement the flush() camera operation in the CameraDevice class\n> > and make it available to the camera framework by implementing the\n> > operation wrapper in camera_ops.cpp.\n> >\n> > The flush() implementation stops the Camera and the worker thread and\n> > waits for all in-flight requests to be returned. Stopping the Camera\n> > forces all Requests already queued to be returned immediately in error\n> > state. As flush() has to wait until all of them have been returned, make\n> it\n> > wait on a newly introduced condition variable which is notified by the\n> > request completion handler when the queue of pending requests has been\n> > exhausted.\n> >\n> > As flush() can race with processCaptureRequest() protect the requests\n> > queueing by introducing a new CameraState::CameraFlushing state that\n> > processCaptureRequest() inspects before queuing the Request to the\n> > Camera. If flush() has been called while processCaptureRequest() was\n> > executing, return the current Request immediately in error state.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++\n> > src/android/camera_device.h | 6 ++++\n> > src/android/camera_ops.cpp | 8 ++++-\n> > 3 files changed, 76 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> > index fa12ce5b0133..01b3acd93c4b 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -756,6 +756,42 @@ void CameraDevice::close()\n> > camera_->release();\n> > }\n> >\n> > +/*\n> > + * Flush is similar to stop() but sets the camera state to 'flushing'\n> and wait\n> > + * until all the in-flight requests have been returned.\n> > + */\n> > +void CameraDevice::flush()\n> > +{\n> > + {\n> > + MutexLocker cameraLock(cameraMutex_);\n> > +\n> > + if (state_ != CameraRunning)\n> > + return;\n> > +\n> > + state_ = CameraFlushing;\n> > +\n> > + /*\n> > + * Stop the camera and set the state to flushing to\n> prevent any\n> > + * new request to be queued from this point on.\n> > + */\n> > + worker_.stop();\n> > + camera_->stop();\n> > + }\n>\n> Releasing cameraMutex_ while waiting for the flushMutex_ signal seems\n> like a race waiting to happen as the operation is acting in\n> synchronization with the state_ statemachine.\n>\n> I understand you release it as you want to lock it in\n> CameraDevice::stop(), is there any other potential race site? If not\n> maybe CameraDevice::stop() can be reworked? It only needs to read the\n> state so is the mutex really needed, could it be reworked to an atomic?\n> Or maybe there is something in the HAL itself that would prevent other\n> callbacks to change the state from CameraFlushing -> CameraRunning while\n> flush() is executing?\n>\n> > +\n> > + /*\n> > + * Now wait for all the in-flight requests to be completed before\n> > + * returning. Stopping the Camera guarantees that all in-flight\n> requests\n> > + * are completed in error state.\n> > + */\n> > + {\n> > + MutexLocker flushLock(flushMutex_);\n> > + flushing_.wait(flushLock, [&] { return\n> descriptors_.empty(); });\n> > + }\n> > +\n> > + MutexLocker cameraLock(cameraMutex_);\n> > + state_ = CameraStopped;\n> > +}\n> > +\n> > void CameraDevice::stop()\n> > {\n> > MutexLocker cameraLock(cameraMutex_);\n> > @@ -2019,6 +2055,26 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > if (ret)\n> > return ret;\n> >\n> > +\n> > + /*\n> > + * Just before queuing the request, make sure flush() has not\n> > + * been called after this function has been executed. In that\n> > + * case, immediately return the request with errors.\n> > + */\n> > + MutexLocker cameraLock(cameraMutex_);\n> > + if (state_ == CameraFlushing || state_ == CameraStopped) {\n> > + for (camera3_stream_buffer_t &buffer :\n> descriptor.buffers_) {\n> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + buffer.release_fence = buffer.acquire_fence;\n> > + }\n> > +\n> > + notifyError(descriptor.frameNumber_,\n> > + descriptor.buffers_[0].stream,\n> > + CAMERA3_MSG_ERROR_REQUEST);\n> > +\n> > + return 0;\n> > + }\n> > +\n> > worker_.queueRequest(descriptor.request_.get());\n> >\n> > {\n> > @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request\n> *request)\n> > }\n> > Camera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > + /* Release flush if all the pending requests have been completed.\n> */\n> > + {\n> > + MutexLocker flushLock(flushMutex_);\n> > + if (descriptors_.empty())\n> > + flushing_.notify_one();\n> > + }\n> > +\n>\n\nIs flushMutex_ necessary? I think it should be replaced with requestMutex_.\nIt is because descriptors_ is accessed in the condition of the wait\nfunction and here, before calling notify_one().\n\n\n> /*\n> > * Prepare the capture result for the Android camera stack.\n> > *\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index ed992ae56d5d..4a9ef495b776 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > #define __ANDROID_CAMERA_DEVICE_H__\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -42,6 +43,7 @@ public:\n> >\n> > int open(const hw_module_t *hardwareModule);\n> > void close();\n> > + void flush();\n> >\n> > unsigned int id() const { return id_; }\n> > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > @@ -92,6 +94,7 @@ private:\n> > enum State {\n> > CameraStopped,\n> > CameraRunning,\n> > + CameraFlushing,\n> > };\n> >\n> > void stop();\n> > @@ -124,6 +127,9 @@ private:\n> > libcamera::Mutex cameraMutex_; /* Protects access to the camera\n> state. */\n> > State state_;\n> >\n> > + libcamera::Mutex flushMutex_; /* Protects the flushing condition\n> variable. */\n> > + std::condition_variable flushing_;\n> > +\n> > std::shared_ptr camera_;\n> > std::unique_ptr config_;\n> >\n> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e80436821..8a3cfa175ff5 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const\n> struct camera3_device *dev,\n> > {\n> > }\n> >\n> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device\n> *dev)\n> > +static int hal_dev_flush(const struct camera3_device *dev)\n> > {\n> > + if (!dev)\n> > + return -EINVAL;\n> > +\n> > + CameraDevice *camera = reinterpret_cast(dev->priv);\n> > + camera->flush();\n> > +\n> > return 0;\n> > }\n> >\n\n\nThis implementation is good if close() and flush() are not called at any\ntime.\nBut, looks like it doesn't protect a concurrent call of close() and flush()\nwith configureStreams() and each other?\n\n-Hiro\n\n\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 23B2DBF829\n\tfor ;\n\tTue, 11 May 2021 06:15:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 819266891C;\n\tTue, 11 May 2021 08:15:30 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 989C868915\n\tfor ;\n\tTue, 11 May 2021 08:15:28 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id a4so28049639ejk.1\n\tfor ;\n\tMon, 10 May 2021 23:15:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"EhMxzvJH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Oo2mXX++Xj8ZQmXSMNqu9BeIJV7hbx/2HCBIj3T6srA=;\n\tb=EhMxzvJHV2qvngDPd+8PJseuqJX5zWqPYvZlP/cHMCkDqqyJlthe8W7xYHd/pd2vxE\n\tL/tfu58pUucxC5eeALJQDTcfy3GyQyPKH8TrzCDhuvhmwj/XOgfEqbLfVTm9UV5GJcax\n\t7Yod2x3X6afbJAtx7ICyzLMaAlpKamC/JgbEM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Oo2mXX++Xj8ZQmXSMNqu9BeIJV7hbx/2HCBIj3T6srA=;\n\tb=K95ER65dcQuBYjAVNjpXHY9hk9ADZgEdi27D15bR0WC+2J3vDOHJULEmVHhHedlIH5\n\tcapFAB3C0AVmNNl/4UKplREKcxMEG7884mzQnW5Zn9iNM8mVY9u4vlY330yjZKHd21bF\n\th6BcgkDfkAZoQQdSTGWG2yTk66OVWPpAjy2wbi91AcMPTuGfISi2RaeIQ3CXAVqzf6qP\n\tHhWcDXMdKE8P7p566HiQhD5p6OpMrVMdvX7SLBeOHL1wbj7Ifjz3O5OKOUYX3Twusb+s\n\tfc7tk3bWx45UuLJ6ABx0YcCFbyuEHgWU5U3BuuQwiZgc6i7Ch3WYp1tAbievBdwF7T1X\n\t9UYQ==","X-Gm-Message-State":"AOAM532qSyZsgaMU3lkmggijXaMmMtfXv9/JasoOhaZ2bMmWD0EV2C0U\n\t7atskg3GIPf8Du69LfFGrYW8OyB3+rR0SLT2wEWbtA==","X-Google-Smtp-Source":"ABdhPJzFpu0QZgzEGS4YgERFIrTIawxDyJFsammUNj5LnA810FcMzuerJKdGfDxlbjSpVdsUAYgZOmIaL1uEKujTSGk=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr30239174ejc.292.1620713728208; \n\tMon, 10 May 2021 23:15:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-9-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 15:15:17 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============8987766243692894871==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16883,"web_url":"https://patchwork.libcamera.org/comment/16883/","msgid":"<20210511074432.swjd6fvmjtvglf5d@uno.localdomain>","date":"2021-05-11T07:44:32","subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, May 10, 2021 at 10:39:09PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-10 12:52:35 +0200, Jacopo Mondi wrote:\n> > Implement the flush() camera operation in the CameraDevice class\n> > and make it available to the camera framework by implementing the\n> > operation wrapper in camera_ops.cpp.\n> >\n> > The flush() implementation stops the Camera and the worker thread and\n> > waits for all in-flight requests to be returned. Stopping the Camera\n> > forces all Requests already queued to be returned immediately in error\n> > state. As flush() has to wait until all of them have been returned, make it\n> > wait on a newly introduced condition variable which is notified by the\n> > request completion handler when the queue of pending requests has been\n> > exhausted.\n> >\n> > As flush() can race with processCaptureRequest() protect the requests\n> > queueing by introducing a new CameraState::CameraFlushing state that\n> > processCaptureRequest() inspects before queuing the Request to the\n> > Camera. If flush() has been called while processCaptureRequest() was\n> > executing, return the current Request immediately in error state.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++\n> > src/android/camera_device.h | 6 ++++\n> > src/android/camera_ops.cpp | 8 ++++-\n> > 3 files changed, 76 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index fa12ce5b0133..01b3acd93c4b 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -756,6 +756,42 @@ void CameraDevice::close()\n> > \tcamera_->release();\n> > }\n> >\n> > +/*\n> > + * Flush is similar to stop() but sets the camera state to 'flushing' and wait\n> > + * until all the in-flight requests have been returned.\n> > + */\n> > +void CameraDevice::flush()\n> > +{\n> > +\t{\n> > +\t\tMutexLocker cameraLock(cameraMutex_);\n> > +\n> > +\t\tif (state_ != CameraRunning)\n> > +\t\t\treturn;\n> > +\n> > +\t\tstate_ = CameraFlushing;\n> > +\n> > +\t\t/*\n> > +\t\t * Stop the camera and set the state to flushing to prevent any\n> > +\t\t * new request to be queued from this point on.\n> > +\t\t */\n> > +\t\tworker_.stop();\n> > +\t\tcamera_->stop();\n> > +\t}\n>\n> Releasing cameraMutex_ while waiting for the flushMutex_ signal seems\n> like a race waiting to happen as the operation is acting in\n> synchronization with the state_ statemachine.\n\nThe two operations happens one after the other, not at the same time,\nam I mistaken ?\n\n>\n> I understand you release it as you want to lock it in\n> CameraDevice::stop(), is there any other potential race site? If not\n\nNope, that's not CameraDevice::stop() but raher\nlibcamera::Camera::stop()\n\ncameraMutex serves to protect against any racing call to\nprocessCaptureRequest() that wants to inspect the camera state to\ndecide if the request has to be queued or returned with error.\n\n\n> maybe CameraDevice::stop() can be reworked? It only needs to read the\n> state so is the mutex really needed, could it be reworked to an atomic?\n> Or maybe there is something in the HAL itself that would prevent other\n> callbacks to change the state from CameraFlushing -> CameraRunning while\n> flush() is executing?\n>\n> > +\n> > +\t/*\n> > +\t * Now wait for all the in-flight requests to be completed before\n> > +\t * returning. Stopping the Camera guarantees that all in-flight requests\n> > +\t * are completed in error state.\n> > +\t */\n> > +\t{\n> > +\t\tMutexLocker flushLock(flushMutex_);\n> > +\t\tflushing_.wait(flushLock, [&] { return descriptors_.empty(); });\n> > +\t}\n> > +\n> > +\tMutexLocker cameraLock(cameraMutex_);\n> > +\tstate_ = CameraStopped;\n> > +}\n> > +\n> > void CameraDevice::stop()\n> > {\n> > \tMutexLocker cameraLock(cameraMutex_);\n> > @@ -2019,6 +2055,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > +\n> > +\t/*\n> > +\t * Just before queuing the request, make sure flush() has not\n> > +\t * been called after this function has been executed. In that\n> > +\t * case, immediately return the request with errors.\n> > +\t */\n> > +\tMutexLocker cameraLock(cameraMutex_);\n> > +\tif (state_ == CameraFlushing || state_ == CameraStopped) {\n> > +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\t\tbuffer.release_fence = buffer.acquire_fence;\n> > +\t\t}\n> > +\n> > +\t\tnotifyError(descriptor.frameNumber_,\n> > +\t\t\t descriptor.buffers_[0].stream,\n> > +\t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > \tworker_.queueRequest(descriptor.request_.get());\n> >\n> > \t{\n> > @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request *request)\n> > \t}\n> > \tCamera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > +\t/* Release flush if all the pending requests have been completed. */\n> > +\t{\n> > +\t\tMutexLocker flushLock(flushMutex_);\n> > +\t\tif (descriptors_.empty())\n> > +\t\t\tflushing_.notify_one();\n> > +\t}\n> > +\n> > \t/*\n> > \t * Prepare the capture result for the Android camera stack.\n> > \t *\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index ed992ae56d5d..4a9ef495b776 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > #define __ANDROID_CAMERA_DEVICE_H__\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -42,6 +43,7 @@ public:\n> >\n> > \tint open(const hw_module_t *hardwareModule);\n> > \tvoid close();\n> > +\tvoid flush();\n> >\n> > \tunsigned int id() const { return id_; }\n> > \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> > @@ -92,6 +94,7 @@ private:\n> > \tenum State {\n> > \t\tCameraStopped,\n> > \t\tCameraRunning,\n> > +\t\tCameraFlushing,\n> > \t};\n> >\n> > \tvoid stop();\n> > @@ -124,6 +127,9 @@ private:\n> > \tlibcamera::Mutex cameraMutex_; /* Protects access to the camera state. */\n> > \tState state_;\n> >\n> > +\tlibcamera::Mutex flushMutex_; /* Protects the flushing condition variable. */\n> > +\tstd::condition_variable flushing_;\n> > +\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> >\n> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e80436821..8a3cfa175ff5 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > {\n> > }\n> >\n> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > +static int hal_dev_flush(const struct camera3_device *dev)\n> > {\n> > +\tif (!dev)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> > +\tcamera->flush();\n> > +\n> > \treturn 0;\n> > }\n> >\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 74482BF839\n\tfor ;\n\tTue, 11 May 2021 07:43:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A00146891F;\n\tTue, 11 May 2021 09:43:50 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 86C58602B7\n\tfor ;\n\tTue, 11 May 2021 09:43:49 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id AAD3C240016;\n\tTue, 11 May 2021 07:43:48 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 09:44:32 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210511074432.swjd6fvmjtvglf5d@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-9-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16884,"web_url":"https://patchwork.libcamera.org/comment/16884/","msgid":"<20210511075006.73mdqwliijsgqdyu@uno.localdomain>","date":"2021-05-11T07:50:06","subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, May 11, 2021 at 03:15:17PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the work.\n>\n> On Tue, May 11, 2021 at 5:39 AM Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n>\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2021-05-10 12:52:35 +0200, Jacopo Mondi wrote:\n> > > Implement the flush() camera operation in the CameraDevice class\n> > > and make it available to the camera framework by implementing the\n> > > operation wrapper in camera_ops.cpp.\n> > >\n> > > The flush() implementation stops the Camera and the worker thread and\n> > > waits for all in-flight requests to be returned. Stopping the Camera\n> > > forces all Requests already queued to be returned immediately in error\n> > > state. As flush() has to wait until all of them have been returned, make\n> > it\n> > > wait on a newly introduced condition variable which is notified by the\n> > > request completion handler when the queue of pending requests has been\n> > > exhausted.\n> > >\n> > > As flush() can race with processCaptureRequest() protect the requests\n> > > queueing by introducing a new CameraState::CameraFlushing state that\n> > > processCaptureRequest() inspects before queuing the Request to the\n> > > Camera. If flush() has been called while processCaptureRequest() was\n> > > executing, return the current Request immediately in error state.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++\n> > > src/android/camera_device.h | 6 ++++\n> > > src/android/camera_ops.cpp | 8 ++++-\n> > > 3 files changed, 76 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp\n> > b/src/android/camera_device.cpp\n> > > index fa12ce5b0133..01b3acd93c4b 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -756,6 +756,42 @@ void CameraDevice::close()\n> > > camera_->release();\n> > > }\n> > >\n> > > +/*\n> > > + * Flush is similar to stop() but sets the camera state to 'flushing'\n> > and wait\n> > > + * until all the in-flight requests have been returned.\n> > > + */\n> > > +void CameraDevice::flush()\n> > > +{\n> > > + {\n> > > + MutexLocker cameraLock(cameraMutex_);\n> > > +\n> > > + if (state_ != CameraRunning)\n> > > + return;\n> > > +\n> > > + state_ = CameraFlushing;\n> > > +\n> > > + /*\n> > > + * Stop the camera and set the state to flushing to\n> > prevent any\n> > > + * new request to be queued from this point on.\n> > > + */\n> > > + worker_.stop();\n> > > + camera_->stop();\n> > > + }\n> >\n> > Releasing cameraMutex_ while waiting for the flushMutex_ signal seems\n> > like a race waiting to happen as the operation is acting in\n> > synchronization with the state_ statemachine.\n> >\n> > I understand you release it as you want to lock it in\n> > CameraDevice::stop(), is there any other potential race site? If not\n> > maybe CameraDevice::stop() can be reworked? It only needs to read the\n> > state so is the mutex really needed, could it be reworked to an atomic?\n> > Or maybe there is something in the HAL itself that would prevent other\n> > callbacks to change the state from CameraFlushing -> CameraRunning while\n> > flush() is executing?\n> >\n> > > +\n> > > + /*\n> > > + * Now wait for all the in-flight requests to be completed before\n> > > + * returning. Stopping the Camera guarantees that all in-flight\n> > requests\n> > > + * are completed in error state.\n> > > + */\n> > > + {\n> > > + MutexLocker flushLock(flushMutex_);\n> > > + flushing_.wait(flushLock, [&] { return\n> > descriptors_.empty(); });\n> > > + }\n> > > +\n> > > + MutexLocker cameraLock(cameraMutex_);\n> > > + state_ = CameraStopped;\n> > > +}\n> > > +\n> > > void CameraDevice::stop()\n> > > {\n> > > MutexLocker cameraLock(cameraMutex_);\n> > > @@ -2019,6 +2055,26 @@ int\n> > CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > if (ret)\n> > > return ret;\n> > >\n> > > +\n> > > + /*\n> > > + * Just before queuing the request, make sure flush() has not\n> > > + * been called after this function has been executed. In that\n> > > + * case, immediately return the request with errors.\n> > > + */\n> > > + MutexLocker cameraLock(cameraMutex_);\n> > > + if (state_ == CameraFlushing || state_ == CameraStopped) {\n> > > + for (camera3_stream_buffer_t &buffer :\n> > descriptor.buffers_) {\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + buffer.release_fence = buffer.acquire_fence;\n> > > + }\n> > > +\n> > > + notifyError(descriptor.frameNumber_,\n> > > + descriptor.buffers_[0].stream,\n> > > + CAMERA3_MSG_ERROR_REQUEST);\n> > > +\n> > > + return 0;\n> > > + }\n> > > +\n> > > worker_.queueRequest(descriptor.request_.get());\n> > >\n> > > {\n> > > @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request\n> > *request)\n> > > }\n> > > Camera3RequestDescriptor &descriptor = node.mapped();\n> > >\n> > > + /* Release flush if all the pending requests have been completed.\n> > */\n> > > + {\n> > > + MutexLocker flushLock(flushMutex_);\n> > > + if (descriptors_.empty())\n> > > + flushing_.notify_one();\n> > > + }\n> > > +\n> >\n>\n> Is flushMutex_ necessary? I think it should be replaced with requestMutex_.\n> It is because descriptors_ is accessed in the condition of the wait\n> function and here, before calling notify_one().\n\nYou're probably right. I could replace flushMutex_ with requestMutex_\nin the flush() implementation. That means I can move this block in the\nprevious one that extracts the descriptor.\n\n>\n>\n> > /*\n> > > * Prepare the capture result for the Android camera stack.\n> > > *\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index ed992ae56d5d..4a9ef495b776 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -7,6 +7,7 @@\n> > > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > > #define __ANDROID_CAMERA_DEVICE_H__\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -42,6 +43,7 @@ public:\n> > >\n> > > int open(const hw_module_t *hardwareModule);\n> > > void close();\n> > > + void flush();\n> > >\n> > > unsigned int id() const { return id_; }\n> > > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > > @@ -92,6 +94,7 @@ private:\n> > > enum State {\n> > > CameraStopped,\n> > > CameraRunning,\n> > > + CameraFlushing,\n> > > };\n> > >\n> > > void stop();\n> > > @@ -124,6 +127,9 @@ private:\n> > > libcamera::Mutex cameraMutex_; /* Protects access to the camera\n> > state. */\n> > > State state_;\n> > >\n> > > + libcamera::Mutex flushMutex_; /* Protects the flushing condition\n> > variable. */\n> > > + std::condition_variable flushing_;\n> > > +\n> > > std::shared_ptr camera_;\n> > > std::unique_ptr config_;\n> > >\n> > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > > index 696e80436821..8a3cfa175ff5 100644\n> > > --- a/src/android/camera_ops.cpp\n> > > +++ b/src/android/camera_ops.cpp\n> > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const\n> > struct camera3_device *dev,\n> > > {\n> > > }\n> > >\n> > > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device\n> > *dev)\n> > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > {\n> > > + if (!dev)\n> > > + return -EINVAL;\n> > > +\n> > > + CameraDevice *camera = reinterpret_cast(dev->priv);\n> > > + camera->flush();\n> > > +\n> > > return 0;\n> > > }\n> > >\n>\n>\n> This implementation is good if close() and flush() are not called at any\n> time.\n> But, looks like it doesn't protect a concurrent call of close() and flush()\n> with configureStreams() and each other?\n\nI found no mention of close() potentially racing with flush() in the\ncamera3.h documentation, but I now recall your team considered that a\npotential race.\n\nclose() can be instrumented to inspect the camera state, possibily by\nadding a new CameraClosed state and taking care of it in\nprocessCaptureRequest() and flush(). It shouldn't be hard.\n\nThanks\n j\n\n>\n> -Hiro\n>\n>\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 499E3BF829\n\tfor ;\n\tTue, 11 May 2021 07:49:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0A356891B;\n\tTue, 11 May 2021 09:49:24 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66129602B7\n\tfor ;\n\tTue, 11 May 2021 09:49:23 +0200 (CEST)","from uno.localdomain (host-82-59-136-116.retail.telecomitalia.it\n\t[82.59.136.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 2CE4D1C0015;\n\tTue, 11 May 2021 07:49:21 +0000 (UTC)"],"X-Originating-IP":"82.59.136.116","Date":"Tue, 11 May 2021 09:50:06 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210511075006.73mdqwliijsgqdyu@uno.localdomain>","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-9-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16889,"web_url":"https://patchwork.libcamera.org/comment/16889/","msgid":"","date":"2021-05-11T09:23:39","subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, May 11, 2021 at 4:49 PM Jacopo Mondi wrote:\n\n> Hi Hiro,\n>\n> On Tue, May 11, 2021 at 03:15:17PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo, thank you for the work.\n> >\n> > On Tue, May 11, 2021 at 5:39 AM Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2021-05-10 12:52:35 +0200, Jacopo Mondi wrote:\n> > > > Implement the flush() camera operation in the CameraDevice class\n> > > > and make it available to the camera framework by implementing the\n> > > > operation wrapper in camera_ops.cpp.\n> > > >\n> > > > The flush() implementation stops the Camera and the worker thread and\n> > > > waits for all in-flight requests to be returned. Stopping the Camera\n> > > > forces all Requests already queued to be returned immediately in\n> error\n> > > > state. As flush() has to wait until all of them have been returned,\n> make\n> > > it\n> > > > wait on a newly introduced condition variable which is notified by\n> the\n> > > > request completion handler when the queue of pending requests has\n> been\n> > > > exhausted.\n> > > >\n> > > > As flush() can race with processCaptureRequest() protect the requests\n> > > > queueing by introducing a new CameraState::CameraFlushing state that\n> > > > processCaptureRequest() inspects before queuing the Request to the\n> > > > Camera. If flush() has been called while processCaptureRequest() was\n> > > > executing, return the current Request immediately in error state.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 63\n> +++++++++++++++++++++++++++++++++++\n> > > > src/android/camera_device.h | 6 ++++\n> > > > src/android/camera_ops.cpp | 8 ++++-\n> > > > 3 files changed, 76 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp\n> > > b/src/android/camera_device.cpp\n> > > > index fa12ce5b0133..01b3acd93c4b 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -756,6 +756,42 @@ void CameraDevice::close()\n> > > > camera_->release();\n> > > > }\n> > > >\n> > > > +/*\n> > > > + * Flush is similar to stop() but sets the camera state to\n> 'flushing'\n> > > and wait\n> > > > + * until all the in-flight requests have been returned.\n> > > > + */\n> > > > +void CameraDevice::flush()\n> > > > +{\n> > > > + {\n> > > > + MutexLocker cameraLock(cameraMutex_);\n> > > > +\n> > > > + if (state_ != CameraRunning)\n> > > > + return;\n> > > > +\n> > > > + state_ = CameraFlushing;\n> > > > +\n> > > > + /*\n> > > > + * Stop the camera and set the state to flushing to\n> > > prevent any\n> > > > + * new request to be queued from this point on.\n> > > > + */\n> > > > + worker_.stop();\n> > > > + camera_->stop();\n> > > > + }\n> > >\n> > > Releasing cameraMutex_ while waiting for the flushMutex_ signal seems\n> > > like a race waiting to happen as the operation is acting in\n> > > synchronization with the state_ statemachine.\n> > >\n> > > I understand you release it as you want to lock it in\n> > > CameraDevice::stop(), is there any other potential race site? If not\n> > > maybe CameraDevice::stop() can be reworked? It only needs to read the\n> > > state so is the mutex really needed, could it be reworked to an atomic?\n> > > Or maybe there is something in the HAL itself that would prevent other\n> > > callbacks to change the state from CameraFlushing -> CameraRunning\n> while\n> > > flush() is executing?\n> > >\n> > > > +\n> > > > + /*\n> > > > + * Now wait for all the in-flight requests to be completed\n> before\n> > > > + * returning. Stopping the Camera guarantees that all in-flight\n> > > requests\n> > > > + * are completed in error state.\n> > > > + */\n> > > > + {\n> > > > + MutexLocker flushLock(flushMutex_);\n> > > > + flushing_.wait(flushLock, [&] { return\n> > > descriptors_.empty(); });\n> > > > + }\n> > > > +\n> > > > + MutexLocker cameraLock(cameraMutex_);\n> > > > + state_ = CameraStopped;\n> > > > +}\n> > > > +\n> > > > void CameraDevice::stop()\n> > > > {\n> > > > MutexLocker cameraLock(cameraMutex_);\n> > > > @@ -2019,6 +2055,26 @@ int\n> > > CameraDevice::processCaptureRequest(camera3_capture_request_t\n> *camera3Reques\n> > > > if (ret)\n> > > > return ret;\n> > > >\n> > > > +\n> > > > + /*\n> > > > + * Just before queuing the request, make sure flush() has not\n> > > > + * been called after this function has been executed. In that\n> > > > + * case, immediately return the request with errors.\n> > > > + */\n> > > > + MutexLocker cameraLock(cameraMutex_);\n> > > > + if (state_ == CameraFlushing || state_ == CameraStopped) {\n> > > > + for (camera3_stream_buffer_t &buffer :\n> > > descriptor.buffers_) {\n> > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > + buffer.release_fence = buffer.acquire_fence;\n> > > > + }\n> > > > +\n> > > > + notifyError(descriptor.frameNumber_,\n> > > > + descriptor.buffers_[0].stream,\n> > > > + CAMERA3_MSG_ERROR_REQUEST);\n> > > > +\n> > > > + return 0;\n> > > > + }\n> > > > +\n> > > > worker_.queueRequest(descriptor.request_.get());\n> > > >\n> > > > {\n> > > > @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request\n> > > *request)\n> > > > }\n> > > > Camera3RequestDescriptor &descriptor = node.mapped();\n> > > >\n> > > > + /* Release flush if all the pending requests have been\n> completed.\n> > > */\n> > > > + {\n> > > > + MutexLocker flushLock(flushMutex_);\n> > > > + if (descriptors_.empty())\n> > > > + flushing_.notify_one();\n> > > > + }\n> > > > +\n> > >\n> >\n> > Is flushMutex_ necessary? I think it should be replaced with\n> requestMutex_.\n> > It is because descriptors_ is accessed in the condition of the wait\n> > function and here, before calling notify_one().\n>\n> You're probably right. I could replace flushMutex_ with requestMutex_\n> in the flush() implementation. That means I can move this block in the\n> previous one that extracts the descriptor.\n>\n> >\n> >\n> > > /*\n> > > > * Prepare the capture result for the Android camera stack.\n> > > > *\n> > > > diff --git a/src/android/camera_device.h\n> b/src/android/camera_device.h\n> > > > index ed992ae56d5d..4a9ef495b776 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -7,6 +7,7 @@\n> > > > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > > > #define __ANDROID_CAMERA_DEVICE_H__\n> > > >\n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -42,6 +43,7 @@ public:\n> > > >\n> > > > int open(const hw_module_t *hardwareModule);\n> > > > void close();\n> > > > + void flush();\n> > > >\n> > > > unsigned int id() const { return id_; }\n> > > > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > > > @@ -92,6 +94,7 @@ private:\n> > > > enum State {\n> > > > CameraStopped,\n> > > > CameraRunning,\n> > > > + CameraFlushing,\n> > > > };\n> > > >\n> > > > void stop();\n> > > > @@ -124,6 +127,9 @@ private:\n> > > > libcamera::Mutex cameraMutex_; /* Protects access to the camera\n> > > state. */\n> > > > State state_;\n> > > >\n> > > > + libcamera::Mutex flushMutex_; /* Protects the flushing\n> condition\n> > > variable. */\n> > > > + std::condition_variable flushing_;\n> > > > +\n> > > > std::shared_ptr camera_;\n> > > > std::unique_ptr config_;\n> > > >\n> > > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > > > index 696e80436821..8a3cfa175ff5 100644\n> > > > --- a/src/android/camera_ops.cpp\n> > > > +++ b/src/android/camera_ops.cpp\n> > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const\n> > > struct camera3_device *dev,\n> > > > {\n> > > > }\n> > > >\n> > > > -static int hal_dev_flush([[maybe_unused]] const struct\n> camera3_device\n> > > *dev)\n> > > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > > {\n> > > > + if (!dev)\n> > > > + return -EINVAL;\n> > > > +\n> > > > + CameraDevice *camera = reinterpret_cast *>(dev->priv);\n> > > > + camera->flush();\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> >\n> >\n> > This implementation is good if close() and flush() are not called at any\n> > time.\n> > But, looks like it doesn't protect a concurrent call of close() and\n> flush()\n> > with configureStreams() and each other?\n>\n> I found no mention of close() potentially racing with flush() in the\n> camera3.h documentation, but I now recall your team considered that a\n> potential race.\n>\n> close() can be instrumented to inspect the camera state, possibily by\n> adding a new CameraClosed state and taking care of it in\n> processCaptureRequest() and flush(). It shouldn't be hard.\n>\n>\nIt sounds good. Again I think you need to guard configureStreams() as well\nas processCaptureRequest().\nI look forward to your next version.\n\n-Hiro\n\n> Thanks\n> j\n>\n> >\n> > -Hiro\n> >\n> >\n> > > > --\n> > > > 2.31.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D41BABF839\n\tfor ;\n\tTue, 11 May 2021 09:23:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47F826891F;\n\tTue, 11 May 2021 11:23:52 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF686602B7\n\tfor ;\n\tTue, 11 May 2021 11:23:50 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id s7so17346473edq.12\n\tfor ;\n\tTue, 11 May 2021 02:23:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hz0dbfCm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=m3dbKmzvfshbsQxBQIpJP9Rlf1s6GH5yhwHLKNDelcY=;\n\tb=hz0dbfCmqndb3bN2YpP7ka32r3JEGKESedn4eQIYh0wR+z10AdKxxq4lVQD2Vhxw4K\n\toOMR1LeuuswzeZ2QbiJKFAeJTcwL9wB9Z8oFG7I3MF3/DAI3w4L1jPS9rSY5y/Z0nFXH\n\tFv9sxsPgyfuSmX1DLAAdyhUiWgqnhl9WsT6mI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=m3dbKmzvfshbsQxBQIpJP9Rlf1s6GH5yhwHLKNDelcY=;\n\tb=tox/zv7bpKRixw0I1K7wfwHNKIvKHr71PpAoi7RGZmrZjyx7OZaE1qMfl2LDWAJFmq\n\tIQzDJlfFJDbWVwYhukgCLndPhdTWFRhEVEAvK86vtbhZSsnG9inc+xibxWh19x1RYUKP\n\ttK5NtlFaXuTJD++PMZ345+SSrcD2HI6KaSEFoSk/YGILpLu7VsRR1XqeYcH+57mLz8dm\n\t3A5KtxKKbpNdIXjtiCLGsvxc2/MRMMK1OoTBDPZvOLnXP3J81WVi1xCCoqMsp4Ug+3o4\n\ths4hHiDFDgiN+MCUmOkOvFCLWmuxOaHVQLwBXU65XTlWhNjaTDKyHkp2DRdcG8tKnNSk\n\t8dQg==","X-Gm-Message-State":"AOAM530vqRsgpOjbvyVK/XOg/2PT32l/zXvVPpvAR4shwRH3umswMmVf\n\tVFJWrPmROOsvDmAoW5AnTCwL9kmcwcRKwZLjna+Vxw==","X-Google-Smtp-Source":"ABdhPJxlg4Crlef/PznEaZ6XfIRDqtRtJbecaP66u8DuUKoerZ7Xpki7urpdR//WPaymVYzOh4b0lOuQPT2/7zCudHw=","X-Received":"by 2002:aa7:c610:: with SMTP id\n\th16mr34478479edq.202.1620725030342; \n\tTue, 11 May 2021 02:23:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20210510105235.28319-1-jacopo@jmondi.org>\n\t<20210510105235.28319-9-jacopo@jmondi.org>\n\t\n\t\n\t<20210511075006.73mdqwliijsgqdyu@uno.localdomain>","In-Reply-To":"<20210511075006.73mdqwliijsgqdyu@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 11 May 2021 18:23:39 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 8/8] android: Implement flush() camera\n\toperation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============6125963102235320267==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]