<div dir="ltr"><div>Hi Jacopo, thank you for the work.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 5:39 AM Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se">niklas.soderlund@ragnatech.se</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Jacopo,<br>
<br>
Thanks for your work.<br>
<br>
On 2021-05-10 12:52:35 +0200, Jacopo Mondi wrote:<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" target="_blank">jacopo@jmondi.org</a>><br>
> ---<br>
> src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++<br>
> src/android/camera_device.h | 6 ++++<br>
> src/android/camera_ops.cpp | 8 ++++-<br>
> 3 files changed, 76 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index fa12ce5b0133..01b3acd93c4b 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -756,6 +756,42 @@ void CameraDevice::close()<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.<br>
> + */<br>
> +void CameraDevice::flush()<br>
> +{<br>
> + {<br>
> + MutexLocker cameraLock(cameraMutex_);<br>
> +<br>
> + if (state_ != CameraRunning)<br>
> + return;<br>
> +<br>
> + state_ = CameraFlushing;<br>
> +<br>
> + /*<br>
> + * Stop the camera and set the state to flushing to prevent any<br>
> + * new request to be queued from this point on.<br>
> + */<br>
> + worker_.stop();<br>
> + camera_->stop();<br>
> + }<br>
<br>
Releasing cameraMutex_ while waiting for the flushMutex_ signal seems <br>
like a race waiting to happen as the operation is acting in <br>
synchronization with the state_ statemachine.<br>
<br>
I understand you release it as you want to lock it in <br>
CameraDevice::stop(), is there any other potential race site? If not <br>
maybe CameraDevice::stop() can be reworked? It only needs to read the <br>
state so is the mutex really needed, could it be reworked to an atomic? <br>
Or maybe there is something in the HAL itself that would prevent other <br>
callbacks to change the state from CameraFlushing -> CameraRunning while <br>
flush() is executing?<br>
<br>
> +<br>
> + /*<br>
> + * Now wait for all the in-flight requests to be completed before<br>
> + * returning. Stopping the Camera guarantees that all in-flight requests<br>
> + * are completed in error state.<br>
> + */<br>
> + {<br>
> + MutexLocker flushLock(flushMutex_);<br>
> + flushing_.wait(flushLock, [&] { return descriptors_.empty(); });<br>
> + }<br>
> +<br>
> + MutexLocker cameraLock(cameraMutex_);<br>
> + state_ = CameraStopped;<br>
> +}<br>
> +<br>
> void CameraDevice::stop()<br>
> {<br>
> MutexLocker cameraLock(cameraMutex_);<br>
> @@ -2019,6 +2055,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>
> if (ret)<br>
> return ret;<br>
> <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>
> @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request *request)<br>
> }<br>
> Camera3RequestDescriptor &descriptor = node.mapped();<br>
> <br>
> + /* Release flush if all the pending requests have been completed. */<br>
> + {<br>
> + MutexLocker flushLock(flushMutex_);<br>
> + if (descriptors_.empty())<br>
> + flushing_.notify_one();<br>
> + }<br>
> +<br></blockquote><div><br></div><div>Is flushMutex_ necessary? I think it should be replaced with requestMutex_.</div><div>It is because descriptors_ is accessed in the condition of the wait function and here, before calling notify_one().</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> /*<br>
> * Prepare the capture result for the Android camera stack.<br>
> *<br>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
> index ed992ae56d5d..4a9ef495b776 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 flushing condition variable. */<br>
> + std::condition_variable flushing_;<br>
> +<br>
> std::shared_ptr<libcamera::Camera> camera_;<br>
> std::unique_ptr<libcamera::CameraConfiguration> config_;<br>
> <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>
> </blockquote><div><br></div><div>This implementation is good if close() and flush() are not called at any time.</div><div>But, looks like it doesn't protect a concurrent call of close() and flush() with configureStreams() and each other?</div><div> </div><div>-Hiro</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br>
> -- <br>
> 2.31.1<br>
> <br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
<br>
-- <br>
Regards,<br>
Niklas Söderlund<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>