<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 20, 2021 at 5:29 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">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 Mon, May 17, 2021 at 02:51:45PM +0900, Hirokazu Honda wrote:<br>
> 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" target="_blank">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<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 | 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<br>
> *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_ !=<br>
> 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'<br>
> 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<br>
> 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<br>
> 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<br>
> variable of flushed_.wait().<br>
<br>
I didn't get the "and the condition variable..." part.<br>
<br></blockquote><div><br></div><div><div>Probably I should have said condition block.</div><div></div></div><div>I meant flushed_.wait(..., { HERE }) in configureStreams() by "the condition variable" .</div><div> + MutexLocker flushLock(flushMutex_);<br> + flushed_.wait(flushLock, [&] { return state_ != CameraStopped; });<br></div><div><br></div><div>The part is protected by cameraMutex_ but I consider more, by that, flushMutex_ is useless. </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">
However, on the first part, I don't think it's a problem, and that's<br>
my reasoning: if we get here the camera state is set to flushing<br>
already.<br>
<br>
Whatever other thread that calls a concurrent close(),<br>
configureStreams() or processCaptureRequest() will find the<br>
CameraState set to flushing, if flush() gets the CameraMutex_ critical<br>
section first. Those threads will see the CameraState as<br>
CameraFlushing and will wait on the flushed_ condition variable, until<br>
flush() notifies them here below, after having changed the camera state.<br>
<br>
if close() or configureStreams() gets the CameraMutex_ critical<br>
section first they will just stop the camera. When mutex gets in the<br>
critical section, it will find the camera is not running anymore, and<br>
will simply return.<br>
<br>
if processCaptureRequest() gets the critical section first, it will<br>
queue the request normally. Once flush() enters the critical section<br>
it will stop the camera and the request will be completed with errors<br>
as usual.<br>
<br></blockquote><div><br></div><div>I got it. You're right. But I would not do so as it breaks the rule that state_ is protected by cameraMutex_.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> I suspect that flushMutex_ is unnecessary and should be replaced with<br>
> cameraMutex_.<br>
<br>
You might be right.. flushMutex_ usage in close() and<br>
configureStreams() can be replaced with cameraMutex_ easily.<br>
I'll try this, thanks for the suggestion.<br>
<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<br>
> caller. */<br>
> > void CameraDevice::stop()<br>
> > {<br>
> > - MutexLocker cameraLock(cameraMutex_);<br></blockquote><div><br></div><div><br></div><div>If I looked the flow correctly, the state is never Flushing here, so let's add</div><div>ASSERT(state_ != CameraFlushing);</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > if (state_ == CameraStopped)<br>
> > return;<br>
> ><br>
> > @@ -1652,8 +1700,20 @@ PixelFormat CameraDevice::toPixelFormat(int<br>
> format) const<br>
> > */<br>
> > int CameraDevice::configureStreams(camera3_stream_configuration_t<br>
> *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<br>
> 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_ !=<br>
> CameraStopped; });<br>
> > + } else {<br>
> > + stop();<br>
> > + }<br>
> > + }<br>
><br>
><br>
> I wonder if we should hold cameraMutex_ entirely in configureStreams()<br>
> because we are touching streams_ and camera_, which are also accessed by<br>
> flush() and stop().<br>
><br>
<br>
I refrained from accessing streams_ in flush to avoid locking the<br>
whole function, which is quite a poor solution as we're back to<br>
basically serialize function calls and that's it. Regarding the camera<br>
state, the critical section at the beginning makes sure<br>
configureStreams() runs with the camera in CameraStopped state, in<br>
which case a flush() called during configureStreams() execution will<br>
bail out immediately, as there are no requests to flush. OTOH if<br>
configureStreams() is called while flush() is executing, it will wait<br>
on the flushed_ condition untile flush() has completed and the camera<br>
is again in stopped state.<br>
<br>
<br></blockquote><div><br></div><div>This sounds correct.</div><div> </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>
> > 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<br>
> make sure camera_ is valid at that time.<br>
<br>
The request queueing in processCaptureRequest() is performed while<br>
helding the CameraMutex_ to prevent a flush to interrupt it. Did I get<br>
your comment wrongly ?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> It seems to be difficult because it is a different thread and therefore we<br>
> don't have any idea when it is called.<br>
> Although I think <a href="https://patchwork.libcamera.org/patch/12089" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/12089</a> helps our<br>
> situation.<br>
<br>
Ah you meant what happens to requests that have not been yet queued on<br>
the Camera as they worker is waiting on their fences ? yes, I think<br>
stopping the worker_ should return the request immediately. I missed<br>
that patch, I'll give it a go.<br>
<br></blockquote><div><br></div><div>Yes.</div><div>CameraWorker::queueRequest() is </div><div>worker_.InvokeMethod(&processRequest, ConnectionTypeQueued, request);</div><div>It is not guaranteed that Camera::queueRequest() is called by the end of processCaptureRequest().</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Overall, my understanding is that trying to remove flushMutex_ and use<br>
cameraMutex_ in its place should be enough to solve your concerns ?<br>
<br></blockquote><div><br></div><div>Yeah, I think the action item is only that.</div><div><br></div><div>Thanks,</div><div>-Hiro</div><div> </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>
> ><br>
> > @@ -2021,6 +2081,25 @@ int<br>
> 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 :<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>
> > @@ -2050,6 +2129,10 @@ void CameraDevice::requestComplete(Request<br>
> *request)<br>
> > return;<br>
> > }<br>
> ><br>
> > + /* Release flush if all the pending requests have been<br>
> completed. */<br>
> > + if (descriptors_.empty())<br>
> > + flushing_.notify_one();<br>
> > +<br>
><br>
> The flush() document states<br>
><br>
> flush() should only return when there are no more outstanding buffers or<br>
> requests left in the HAL.<br>
><br>
><br>
> Is it okay that flush() returns before notifyError() and<br>
> process_capture_result() is called?<br>
><br>
> -Hiro<br>
><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<br>
> state. */<br>
> > State state_;<br>
> ><br>
> > + libcamera::Mutex flushMutex_; /* Protects the flushed_ condition<br>
> 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<br>
> 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<br>
> 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<br>
> *>(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" 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>