<div dir="ltr">Hi Jacopo, thank you for the patch.<br><br><br><br>On Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br>><br>> Implement the flush() camera operation in the CameraDevice class<br>> and make it available to the camera framework by implementing the<br>> operation wrapper in camera_ops.cpp.<br>><br>> The flush() implementation stops the Camera and the worker thread and<br>> waits for all in-flight requests to be returned. Stopping the Camera<br>> forces all Requests already queued to be returned immediately in error<br>> state. As flush() has to wait until all of them have been returned, make it<br>> wait on a newly introduced condition variable which is notified by the<br>> request completion handler when the queue of pending requests has been<br>> exhausted.<br>><br>> As flush() can race with processCaptureRequest() protect the requests<br>> queueing by introducing a new CameraState::CameraFlushing state that<br>> processCaptureRequest() inspects before queuing the Request to the<br>> Camera. If flush() has been called while processCaptureRequest() was<br>> executing, return the current Request immediately in error state.<br>><br>> Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>><br>> ---<br>>  src/android/camera_device.cpp | 91 +++++++++++++++++++++++++++++++++--<br>>  src/android/camera_device.h   |  9 +++-<br>>  src/android/camera_ops.cpp    |  8 ++-<br>>  3 files changed, 102 insertions(+), 6 deletions(-)<br>><br>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>> index 7f8c9bd7832d..6d6730a50ec7 100644<br>> --- a/src/android/camera_device.cpp<br>> +++ b/src/android/camera_device.cpp<br>> @@ -750,16 +750,64 @@ int CameraDevice::open(const hw_module_t *hardwareModule)<br>><br>>  void CameraDevice::close()<br>>  {<br>> -       streams_.clear();<br>> +       MutexLocker cameraLock(cameraMutex_);<br>> +       if (state_ == CameraFlushing) {<br>> +               MutexLocker flushLock(flushMutex_);<br>> +               flushed_.wait(flushLock, [&] { return state_ != CameraStopped; });<br>> +               camera_->release();<br>><br>> +               return;<br>> +       }<br>> +<br>> +       streams_.clear();<br>>         stop();<br>><br>>         camera_->release();<br>>  }<br>><br>> +/*<br>> + * Flush is similar to stop() but sets the camera state to 'flushing' and wait<br>> + * until all the in-flight requests have been returned before setting the<br>> + * camera state to stopped.<br>> + *<br>> + * Once flushing is done it unlocks concurrent camera close() calls or new<br>> + * camera configurations.<br>> + */<br>> +void CameraDevice::flush()<br>> +{<br>> +       {<br>> +               MutexLocker cameraLock(cameraMutex_);<br>> +<br>> +               if (state_ != CameraRunning)<br>> +                       return;<br>> +<br>> +               worker_.stop();<br>> +               camera_->stop();<br>> +               state_ = CameraFlushing;<br>> +       }<br>> +<br>> +       /*<br>> +        * Now wait for all the in-flight requests to be completed before<br>> +        * continuing. Stopping the Camera guarantees that all in-flight<br>> +        * requests are completed in error state.<br>> +        */<br>> +       {<br>> +               MutexLocker requestsLock(requestsMutex_);<br>> +               flushing_.wait(requestsLock, [&] { return descriptors_.empty(); });<br>> +       }<br>> +<br>> +       /*<br>> +        * Unlock close() or configureStreams() that might be waiting for<br>> +        * flush to be completed.<br>> +        */<br>> +       MutexLocker flushLocker(flushMutex_);<br><br><br>I found state_ is touched without cameraMutex_ here and the condition variable of flushed_.wait().<br>I suspect that flushMutex_ is unnecessary and should be replaced with cameraMutex_.<br><br> <br>><br>> +       state_ = CameraStopped;<br>> +       flushed_.notify_one();<br>> +}<br>> +<br>> +/* Calls to stop() must be protected by cameraMutex_ being held by the caller. */<br>>  void CameraDevice::stop()<br>>  {<br>> -       MutexLocker cameraLock(cameraMutex_);<br>>         if (state_ == CameraStopped)<br>>                 return;<br>><br>> @@ -1652,8 +1700,20 @@ PixelFormat CameraDevice::toPixelFormat(int format) const<br>>   */<br>>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)<br>>  {<br>> -       /* Before any configuration attempt, stop the camera. */<br>> -       stop();<br>> +       {<br>> +               /*<br>> +                * If a flush is in progress, wait for it to complete and to<br>> +                * stop the camera, otherwise before any new configuration<br>> +                * attempt we have to stop the camera explictely.<br>> +                */<br>> +               MutexLocker cameraLock(cameraMutex_);<br>> +               if (state_ == CameraFlushing) {<br>> +                       MutexLocker flushLock(flushMutex_);<br>> +                       flushed_.wait(flushLock, [&] { return state_ != CameraStopped; });<br>> +               } else {<br>> +                       stop();<br>> +               }<br>> +       }<br><br><br>I wonder if we should hold cameraMutex_ entirely in configureStreams() because we are touching streams_ and camera_, which are also accessed by flush() and stop().<br> <br>><br>><br>>         if (stream_list->num_streams == 0) {<br>>                 LOG(HAL, Error) << "No streams in configuration";<br><br><br>CaptureRequest, which is constructed through Camera3RequestDescriptor.<br>CaptureRequest::queue() calls camera_->queueRequest(). It is necessary to make sure camera_ is valid at that time.<br>It seems to be difficult because it is a different thread and therefore we don't have any idea when it is called.<br>Although I think <a href="https://patchwork.libcamera.org/patch/12089">https://patchwork.libcamera.org/patch/12089</a> helps our situation.<br> <br>><br>> @@ -2021,6 +2081,25 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>>         if (ret)<br>>                 return ret;<br>><br>> +       /*<br>> +        * Just before queuing the request, make sure flush() has not<br>> +        * been called after this function has been executed. In that<br>> +        * case, immediately return the request with errors.<br>> +        */<br>> +       MutexLocker cameraLock(cameraMutex_);<br>> +       if (state_ == CameraFlushing || state_ == CameraStopped) {<br>> +               for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {<br>> +                       buffer.status = CAMERA3_BUFFER_STATUS_ERROR;<br>> +                       buffer.release_fence = buffer.acquire_fence;<br>> +               }<br>> +<br>> +               notifyError(descriptor.frameNumber_,<br>> +                           descriptor.buffers_[0].stream,<br>> +                           CAMERA3_MSG_ERROR_REQUEST);<br>> +<br>> +               return 0;<br>> +       }<br>> +<br>>         worker_.queueRequest(descriptor.request_.get());<br>><br>>         {<br>> @@ -2050,6 +2129,10 @@ void CameraDevice::requestComplete(Request *request)<br>>                         return;<br>>                 }<br>><br>> +               /* Release flush if all the pending requests have been completed. */<br>> +               if (descriptors_.empty())<br>> +                       flushing_.notify_one();<br>> +<br><br>The flush() document states <br><blockquote style="margin:0 0 0 40px;border:none;padding:0px">flush() should only return when there are no more outstanding buffers or requests left in the HAL.</blockquote><div><br></div>Is it okay that flush() returns before notifyError() and process_capture_result() is called?<br> <div>-Hiro</div><div><br>><br>>                 node = descriptors_.extract(it);<br>>         }<br>>         Camera3RequestDescriptor &descriptor = node.mapped();<br>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>> index ed992ae56d5d..9c55cc2674b7 100644<br>> --- a/src/android/camera_device.h<br>> +++ b/src/android/camera_device.h<br>> @@ -7,6 +7,7 @@<br>>  #ifndef __ANDROID_CAMERA_DEVICE_H__<br>>  #define __ANDROID_CAMERA_DEVICE_H__<br>><br>> +#include <condition_variable><br>>  #include <map><br>>  #include <memory><br>>  #include <mutex><br>> @@ -42,6 +43,7 @@ public:<br>><br>>         int open(const hw_module_t *hardwareModule);<br>>         void close();<br>> +       void flush();<br>><br>>         unsigned int id() const { return id_; }<br>>         camera3_device_t *camera3Device() { return &camera3Device_; }<br>> @@ -92,6 +94,7 @@ private:<br>>         enum State {<br>>                 CameraStopped,<br>>                 CameraRunning,<br>> +               CameraFlushing,<br>>         };<br>><br>>         void stop();<br>> @@ -124,6 +127,9 @@ private:<br>>         libcamera::Mutex cameraMutex_; /* Protects access to the camera state. */<br>>         State state_;<br>><br>> +       libcamera::Mutex flushMutex_; /* Protects the flushed_ condition variable. */<br>> +       std::condition_variable flushed_;<br>> +<br>>         std::shared_ptr<libcamera::Camera> camera_;<br>>         std::unique_ptr<libcamera::CameraConfiguration> config_;<br>><br>> @@ -135,8 +141,9 @@ private:<br>>         std::map<int, libcamera::PixelFormat> formatsMap_;<br>>         std::vector<CameraStream> streams_;<br>><br>> -       libcamera::Mutex requestsMutex_; /* Protects descriptors_. */<br>> +       libcamera::Mutex requestsMutex_; /* Protects descriptors_ and flushing_. */<br>>         std::map<uint64_t, Camera3RequestDescriptor> descriptors_;<br>> +       std::condition_variable flushing_;<br>><br>>         std::string maker_;<br>>         std::string model_;<br>> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp<br>> index 696e80436821..8a3cfa175ff5 100644<br>> --- a/src/android/camera_ops.cpp<br>> +++ b/src/android/camera_ops.cpp<br>> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,<br>>  {<br>>  }<br>><br>> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)<br>> +static int hal_dev_flush(const struct camera3_device *dev)<br>>  {<br>> +       if (!dev)<br>> +               return -EINVAL;<br>> +<br>> +       CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);<br>> +       camera->flush();<br>> +<br>>         return 0;<br>>  }<br>><br>> --<br>> 2.31.1<br>><br>> _______________________________________________<br>> libcamera-devel mailing list<br>> <a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</a><br>> <a href="https://lists.libcamera.org/listinfo/libcamera-devel">https://lists.libcamera.org/listinfo/libcamera-devel</a></div></div>