<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 11, 2021 at 4:49 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</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 Hiro,<br>
<br>
On Tue, May 11, 2021 at 03:15:17PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo, thank you for the work.<br>
><br>
> On Tue, May 11, 2021 at 5:39 AM Niklas Söderlund <<br>
> <a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>> wrote:<br>
><br>
> > 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<br>
> > 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<br>
> > 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'<br>
> > 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<br>
> > 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<br>
> > requests<br>
> > > + * are completed in error state.<br>
> > > + */<br>
> > > + {<br>
> > > + MutexLocker flushLock(flushMutex_);<br>
> > > + flushing_.wait(flushLock, [&] { return<br>
> > 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<br>
> > 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 :<br>
> > 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<br>
> > *request)<br>
> > > }<br>
> > > Camera3RequestDescriptor &descriptor = node.mapped();<br>
> > ><br>
> > > + /* Release flush if all the pending requests have been completed.<br>
> > */<br>
> > > + {<br>
> > > + MutexLocker flushLock(flushMutex_);<br>
> > > + if (descriptors_.empty())<br>
> > > + flushing_.notify_one();<br>
> > > + }<br>
> > > +<br>
> ><br>
><br>
> Is flushMutex_ necessary? I think it should be replaced with requestMutex_.<br>
> It is because descriptors_ is accessed in the condition of the wait<br>
> function and here, before calling notify_one().<br>
<br>
You're probably right. I could replace flushMutex_ with requestMutex_<br>
in the flush() implementation. That means I can move this block in the<br>
previous one that extracts the descriptor.<br>
<br>
><br>
><br>
> > /*<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<br>
> > state. */<br>
> > > State state_;<br>
> > ><br>
> > > + libcamera::Mutex flushMutex_; /* Protects the flushing condition<br>
> > 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<br>
> > struct camera3_device *dev,<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device<br>
> > *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>
><br>
> This implementation is good if close() and flush() are not called at any<br>
> time.<br>
> But, looks like it doesn't protect a concurrent call of close() and flush()<br>
> with configureStreams() and each other?<br>
<br>
I found no mention of close() potentially racing with flush() in the<br>
camera3.h documentation, but I now recall your team considered that a<br>
potential race.<br>
<br>
close() can be instrumented to inspect the camera state, possibily by<br>
adding a new CameraClosed state and taking care of it in<br>
processCaptureRequest() and flush(). It shouldn't be hard.<br>
<br></blockquote><div><br></div><div>It sounds good. Again I think you need to guard configureStreams() as well as processCaptureRequest().</div><div>I look forward to your next version.</div><div><br></div><div>-Hiro</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<br>
><br>
> -Hiro<br>
><br>
><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>
> ><br>
</blockquote></div></div>