[libcamera-devel] [PATCH v2 8/8] android: Implement flush() camera operation
Hirokazu Honda
hiroh at chromium.org
Fri May 21 06:07:26 CEST 2021
Hi Jacopo,
On Thu, May 20, 2021 at 5:29 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Hiro,
>
> On Mon, May 17, 2021 at 02:51:45PM +0900, Hirokazu Honda wrote:
> > Hi Jacopo, thank you for the patch.
> >
> >
> >
> > On Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > Implement the flush() camera operation in the CameraDevice class
> > > and make it available to the camera framework by implementing the
> > > operation wrapper in camera_ops.cpp.
> > >
> > > The flush() implementation stops the Camera and the worker thread and
> > > waits for all in-flight requests to be returned. Stopping the Camera
> > > forces all Requests already queued to be returned immediately in error
> > > state. As flush() has to wait until all of them have been returned,
> make
> > it
> > > wait on a newly introduced condition variable which is notified by the
> > > request completion handler when the queue of pending requests has been
> > > exhausted.
> > >
> > > As flush() can race with processCaptureRequest() protect the requests
> > > queueing by introducing a new CameraState::CameraFlushing state that
> > > processCaptureRequest() inspects before queuing the Request to the
> > > Camera. If flush() has been called while processCaptureRequest() was
> > > executing, return the current Request immediately in error state.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > src/android/camera_device.cpp | 91 +++++++++++++++++++++++++++++++++--
> > > src/android/camera_device.h | 9 +++-
> > > src/android/camera_ops.cpp | 8 ++-
> > > 3 files changed, 102 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
> > > index 7f8c9bd7832d..6d6730a50ec7 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -750,16 +750,64 @@ int CameraDevice::open(const hw_module_t
> > *hardwareModule)
> > >
> > > void CameraDevice::close()
> > > {
> > > - streams_.clear();
> > > + MutexLocker cameraLock(cameraMutex_);
> > > + if (state_ == CameraFlushing) {
> > > + MutexLocker flushLock(flushMutex_);
> > > + flushed_.wait(flushLock, [&] { return state_ !=
> > CameraStopped; });
> > > + camera_->release();
> > >
> > > + return;
> > > + }
> > > +
> > > + streams_.clear();
> > > stop();
> > >
> > > camera_->release();
> > > }
> > >
> > > +/*
> > > + * Flush is similar to stop() but sets the camera state to 'flushing'
> > and wait
> > > + * until all the in-flight requests have been returned before setting
> the
> > > + * camera state to stopped.
> > > + *
> > > + * Once flushing is done it unlocks concurrent camera close() calls or
> > new
> > > + * camera configurations.
> > > + */
> > > +void CameraDevice::flush()
> > > +{
> > > + {
> > > + MutexLocker cameraLock(cameraMutex_);
> > > +
> > > + if (state_ != CameraRunning)
> > > + return;
> > > +
> > > + worker_.stop();
> > > + camera_->stop();
> > > + state_ = CameraFlushing;
> > > + }
> > > +
> > > + /*
> > > + * Now wait for all the in-flight requests to be completed
> before
> > > + * continuing. Stopping the Camera guarantees that all
> in-flight
> > > + * requests are completed in error state.
> > > + */
> > > + {
> > > + MutexLocker requestsLock(requestsMutex_);
> > > + flushing_.wait(requestsLock, [&] { return
> > descriptors_.empty(); });
> > > + }
> > > +
> > > + /*
> > > + * Unlock close() or configureStreams() that might be waiting
> for
> > > + * flush to be completed.
> > > + */
> > > + MutexLocker flushLocker(flushMutex_);
> >
> >
> > I found state_ is touched without cameraMutex_ here and the condition
> > variable of flushed_.wait().
>
> I didn't get the "and the condition variable..." part.
>
>
Probably I should have said condition block.
I meant flushed_.wait(..., { HERE }) in configureStreams() by "the
condition variable" .
+ MutexLocker flushLock(flushMutex_);
+ flushed_.wait(flushLock, [&] { return state_ !=
CameraStopped; });
The part is protected by cameraMutex_ but I consider more, by that,
flushMutex_ is useless.
> However, on the first part, I don't think it's a problem, and that's
> my reasoning: if we get here the camera state is set to flushing
> already.
>
> Whatever other thread that calls a concurrent close(),
> configureStreams() or processCaptureRequest() will find the
> CameraState set to flushing, if flush() gets the CameraMutex_ critical
> section first. Those threads will see the CameraState as
> CameraFlushing and will wait on the flushed_ condition variable, until
> flush() notifies them here below, after having changed the camera state.
>
> if close() or configureStreams() gets the CameraMutex_ critical
> section first they will just stop the camera. When mutex gets in the
> critical section, it will find the camera is not running anymore, and
> will simply return.
>
> if processCaptureRequest() gets the critical section first, it will
> queue the request normally. Once flush() enters the critical section
> it will stop the camera and the request will be completed with errors
> as usual.
>
>
I got it. You're right. But I would not do so as it breaks the rule that
state_ is protected by cameraMutex_.
> > I suspect that flushMutex_ is unnecessary and should be replaced with
> > cameraMutex_.
>
> You might be right.. flushMutex_ usage in close() and
> configureStreams() can be replaced with cameraMutex_ easily.
> I'll try this, thanks for the suggestion.
>
> >
> >
> > >
> > > + state_ = CameraStopped;
> > > + flushed_.notify_one();
> > > +}
> > > +
> > > +/* Calls to stop() must be protected by cameraMutex_ being held by the
> > caller. */
> > > void CameraDevice::stop()
> > > {
> > > - MutexLocker cameraLock(cameraMutex_);
>
If I looked the flow correctly, the state is never Flushing here, so let's
add
ASSERT(state_ != CameraFlushing);
> > > if (state_ == CameraStopped)
> > > return;
> > >
> > > @@ -1652,8 +1700,20 @@ PixelFormat CameraDevice::toPixelFormat(int
> > format) const
> > > */
> > > int CameraDevice::configureStreams(camera3_stream_configuration_t
> > *stream_list)
> > > {
> > > - /* Before any configuration attempt, stop the camera. */
> > > - stop();
> > > + {
> > > + /*
> > > + * If a flush is in progress, wait for it to complete
> and
> > to
> > > + * stop the camera, otherwise before any new
> configuration
> > > + * attempt we have to stop the camera explictely.
> > > + */
> > > + MutexLocker cameraLock(cameraMutex_);
> > > + if (state_ == CameraFlushing) {
> > > + MutexLocker flushLock(flushMutex_);
> > > + flushed_.wait(flushLock, [&] { return state_ !=
> > CameraStopped; });
> > > + } else {
> > > + stop();
> > > + }
> > > + }
> >
> >
> > 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().
> >
>
> I refrained from accessing streams_ in flush to avoid locking the
> whole function, which is quite a poor solution as we're back to
> basically serialize function calls and that's it. Regarding the camera
> state, the critical section at the beginning makes sure
> configureStreams() runs with the camera in CameraStopped state, in
> which case a flush() called during configureStreams() execution will
> bail out immediately, as there are no requests to flush. OTOH if
> configureStreams() is called while flush() is executing, it will wait
> on the flushed_ condition untile flush() has completed and the camera
> is again in stopped state.
>
>
>
This sounds correct.
> > >
> > >
> > > if (stream_list->num_streams == 0) {
> > > LOG(HAL, Error) << "No streams in configuration";
> >
> >
> > CaptureRequest, which is constructed through Camera3RequestDescriptor.
> > CaptureRequest::queue() calls camera_->queueRequest(). It is necessary to
> > make sure camera_ is valid at that time.
>
> The request queueing in processCaptureRequest() is performed while
> helding the CameraMutex_ to prevent a flush to interrupt it. Did I get
> your comment wrongly ?
>
> It seems to be difficult because it is a different thread and therefore we
> > don't have any idea when it is called.
> > Although I think https://patchwork.libcamera.org/patch/12089 helps our
> > situation.
>
> Ah you meant what happens to requests that have not been yet queued on
> the Camera as they worker is waiting on their fences ? yes, I think
> stopping the worker_ should return the request immediately. I missed
> that patch, I'll give it a go.
>
>
Yes.
CameraWorker::queueRequest() is
worker_.InvokeMethod(&processRequest, ConnectionTypeQueued, request);
It is not guaranteed that Camera::queueRequest() is called by the end of
processCaptureRequest().
> Overall, my understanding is that trying to remove flushMutex_ and use
> cameraMutex_ in its place should be enough to solve your concerns ?
>
>
Yeah, I think the action item is only that.
Thanks,
-Hiro
> Thanks
> j
>
> >
> > >
> > > @@ -2021,6 +2081,25 @@ int
> > CameraDevice::processCaptureRequest(camera3_capture_request_t
> *camera3Reques
> > > if (ret)
> > > return ret;
> > >
> > > + /*
> > > + * Just before queuing the request, make sure flush() has not
> > > + * been called after this function has been executed. In that
> > > + * case, immediately return the request with errors.
> > > + */
> > > + MutexLocker cameraLock(cameraMutex_);
> > > + if (state_ == CameraFlushing || state_ == CameraStopped) {
> > > + for (camera3_stream_buffer_t &buffer :
> > descriptor.buffers_) {
> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > + buffer.release_fence = buffer.acquire_fence;
> > > + }
> > > +
> > > + notifyError(descriptor.frameNumber_,
> > > + descriptor.buffers_[0].stream,
> > > + CAMERA3_MSG_ERROR_REQUEST);
> > > +
> > > + return 0;
> > > + }
> > > +
> > > worker_.queueRequest(descriptor.request_.get());
> > >
> > > {
> > > @@ -2050,6 +2129,10 @@ void CameraDevice::requestComplete(Request
> > *request)
> > > return;
> > > }
> > >
> > > + /* Release flush if all the pending requests have been
> > completed. */
> > > + if (descriptors_.empty())
> > > + flushing_.notify_one();
> > > +
> >
> > The flush() document states
> >
> > flush() should only return when there are no more outstanding buffers or
> > requests left in the HAL.
> >
> >
> > Is it okay that flush() returns before notifyError() and
> > process_capture_result() is called?
> >
> > -Hiro
> >
> > >
> > > node = descriptors_.extract(it);
> > > }
> > > Camera3RequestDescriptor &descriptor = node.mapped();
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index ed992ae56d5d..9c55cc2674b7 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -7,6 +7,7 @@
> > > #ifndef __ANDROID_CAMERA_DEVICE_H__
> > > #define __ANDROID_CAMERA_DEVICE_H__
> > >
> > > +#include <condition_variable>
> > > #include <map>
> > > #include <memory>
> > > #include <mutex>
> > > @@ -42,6 +43,7 @@ public:
> > >
> > > int open(const hw_module_t *hardwareModule);
> > > void close();
> > > + void flush();
> > >
> > > unsigned int id() const { return id_; }
> > > camera3_device_t *camera3Device() { return &camera3Device_; }
> > > @@ -92,6 +94,7 @@ private:
> > > enum State {
> > > CameraStopped,
> > > CameraRunning,
> > > + CameraFlushing,
> > > };
> > >
> > > void stop();
> > > @@ -124,6 +127,9 @@ private:
> > > libcamera::Mutex cameraMutex_; /* Protects access to the camera
> > state. */
> > > State state_;
> > >
> > > + libcamera::Mutex flushMutex_; /* Protects the flushed_
> condition
> > variable. */
> > > + std::condition_variable flushed_;
> > > +
> > > std::shared_ptr<libcamera::Camera> camera_;
> > > std::unique_ptr<libcamera::CameraConfiguration> config_;
> > >
> > > @@ -135,8 +141,9 @@ private:
> > > std::map<int, libcamera::PixelFormat> formatsMap_;
> > > std::vector<CameraStream> streams_;
> > >
> > > - libcamera::Mutex requestsMutex_; /* Protects descriptors_. */
> > > + libcamera::Mutex requestsMutex_; /* Protects descriptors_ and
> > flushing_. */
> > > std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> > > + std::condition_variable flushing_;
> > >
> > > std::string maker_;
> > > std::string model_;
> > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> > > index 696e80436821..8a3cfa175ff5 100644
> > > --- a/src/android/camera_ops.cpp
> > > +++ b/src/android/camera_ops.cpp
> > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const
> struct
> > camera3_device *dev,
> > > {
> > > }
> > >
> > > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device
> > *dev)
> > > +static int hal_dev_flush(const struct camera3_device *dev)
> > > {
> > > + if (!dev)
> > > + return -EINVAL;
> > > +
> > > + CameraDevice *camera = reinterpret_cast<CameraDevice
> > *>(dev->priv);
> > > + camera->flush();
> > > +
> > > return 0;
> > > }
> > >
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210521/cf47f0cb/attachment-0001.htm>
More information about the libcamera-devel
mailing list