[libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce stopDevice()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 12 16:55:56 CET 2021
Hello,
On Wed, Nov 10, 2021 at 06:31:09PM +0530, Umang Jain wrote:
> On 11/9/21 11:24 PM, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2021-11-09 17:47:26)
> >> On Tue, Nov 09, 2021 at 02:49:42PM +0000, Kieran Bingham wrote:
> >>> Quoting Jacopo Mondi (2021-10-28 12:15:18)
> >>>> Since a queue of waiting Requests has been introduced, not all Requests
> >>>> queued to the PipelineHandler are immediately queued to the device.
> >>>>
> >>>> As a Camera can be stopped at any time, it is required to complete the
> >>>> waiting requests after the ones queued to the device had been completed.
> >>>>
> >>>> Introduce a pure virtual PipelineHandler::stopDevice() function to be
> >>>> implemented by pipeline handlers and make the PipelineHandler::stop()
> >>>> function call it before completing pending requests.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>>> ---
> >>>> include/libcamera/internal/pipeline_handler.h | 3 ++-
> >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--
> >>>> .../pipeline/raspberrypi/raspberrypi.cpp | 4 +--
> >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +--
> >>>> src/libcamera/pipeline/simple/simple.cpp | 4 +--
> >>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +--
> >>>> src/libcamera/pipeline/vimc/vimc.cpp | 4 +--
> >>>> src/libcamera/pipeline_handler.cpp | 27 +++++++++++++++++--
> >>>> 8 files changed, 39 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >>>> index afb7990ea21b..5c3c0bc5a8b3 100644
> >>>> --- a/include/libcamera/internal/pipeline_handler.h
> >>>> +++ b/include/libcamera/internal/pipeline_handler.h
> >>>> @@ -56,7 +56,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> >>>>
> >>>> virtual int start(Camera *camera, const ControlList *controls) = 0;
> >>>> - virtual void stop(Camera *camera) = 0;
> >>>> + void stop(Camera *camera);
> >>>> bool hasPendingRequests(const Camera *camera) const;
> >>>>
> >>>> void queueRequest(Request *request);
> >>>> @@ -71,6 +71,7 @@ protected:
> >>>> void hotplugMediaDevice(MediaDevice *media);
> >>>>
> >>>> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> >>>> + virtual void stopDevice(Camera *camera) = 0;
> >>>>
> >>>> CameraManager *manager_;
> >>>>
> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>> index eb714aa61099..2b2d7904f275 100644
> >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>> @@ -135,7 +135,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >>>>
> >>>> int start(Camera *camera, const ControlList *controls) override;
> >>>> - void stop(Camera *camera) override;
> >>>> + void stopDevice(Camera *camera) override;
> >>>>
> >>>> int queueRequestDevice(Camera *camera, Request *request) override;
> >>>>
> >>>> @@ -791,7 +791,7 @@ error:
> >>>> return ret;
> >>>> }
> >>>>
> >>>> -void PipelineHandlerIPU3::stop(Camera *camera)
> >>>> +void PipelineHandlerIPU3::stopDevice(Camera *camera)
> >>>> {
> >>>> IPU3CameraData *data = cameraData(camera);
> >>>> int ret = 0;
> >>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>> index 1634ca98f481..b5b7a240d5ed 100644
> >>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>> @@ -255,7 +255,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >>>>
> >>>> int start(Camera *camera, const ControlList *controls) override;
> >>>> - void stop(Camera *camera) override;
> >>>> + void stopDevice(Camera *camera) override;
> >>>>
> >>>> int queueRequestDevice(Camera *camera, Request *request) override;
> >>>>
> >>>> @@ -878,7 +878,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -void PipelineHandlerRPi::stop(Camera *camera)
> >>>> +void PipelineHandlerRPi::stopDevice(Camera *camera)
> >>>> {
> >>>> RPiCameraData *data = cameraData(camera);
> >>>>
> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>>> index 980088628523..9860fa84a5ae 100644
> >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>>> @@ -146,7 +146,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >>>>
> >>>> int start(Camera *camera, const ControlList *controls) override;
> >>>> - void stop(Camera *camera) override;
> >>>> + void stopDevice(Camera *camera) override;
> >>>>
> >>>> int queueRequestDevice(Camera *camera, Request *request) override;
> >>>>
> >>>> @@ -827,7 +827,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> >>>> return ret;
> >>>> }
> >>>>
> >>>> -void PipelineHandlerRkISP1::stop(Camera *camera)
> >>>> +void PipelineHandlerRkISP1::stopDevice(Camera *camera)
> >>>> {
> >>>> RkISP1CameraData *data = cameraData(camera);
> >>>> int ret;
> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >>>> index 701fb4be0b71..fdff4ebd5134 100644
> >>>> --- a/src/libcamera/pipeline/simple/simple.cpp
> >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >>>> @@ -279,7 +279,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >>>>
> >>>> int start(Camera *camera, const ControlList *controls) override;
> >>>> - void stop(Camera *camera) override;
> >>>> + void stopDevice(Camera *camera) override;
> >>>>
> >>>> bool match(DeviceEnumerator *enumerator) override;
> >>>>
> >>>> @@ -1036,7 +1036,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -void SimplePipelineHandler::stop(Camera *camera)
> >>>> +void SimplePipelineHandler::stopDevice(Camera *camera)
> >>>> {
> >>>> SimpleCameraData *data = cameraData(camera);
> >>>> V4L2VideoDevice *video = data->video_;
> >>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>>> index 264f5370cf32..40654a0bc40c 100644
> >>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> >>>> @@ -74,7 +74,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >>>>
> >>>> int start(Camera *camera, const ControlList *controls) override;
> >>>> - void stop(Camera *camera) override;
> >>>> + void stopDevice(Camera *camera) override;
> >>>>
> >>>> int queueRequestDevice(Camera *camera, Request *request) override;
> >>>>
> >>>> @@ -250,7 +250,7 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -void PipelineHandlerUVC::stop(Camera *camera)
> >>>> +void PipelineHandlerUVC::stopDevice(Camera *camera)
> >>>> {
> >>>> UVCCameraData *data = cameraData(camera);
> >>>> data->video_->streamOff();
> >>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> >>>> index e453091da4b2..29960fe3f038 100644
> >>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> >>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> >>>> @@ -91,7 +91,7 @@ public:
> >>>> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >>>>
> >>>> int start(Camera *camera, const ControlList *controls) override;
> >>>> - void stop(Camera *camera) override;
> >>>> + void stopDevice(Camera *camera) override;
> >>>>
> >>>> int queueRequestDevice(Camera *camera, Request *request) override;
> >>>>
> >>>> @@ -359,7 +359,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis
> >>>> return 0;
> >>>> }
> >>>>
> >>>> -void PipelineHandlerVimc::stop(Camera *camera)
> >>>> +void PipelineHandlerVimc::stopDevice(Camera *camera)
> >>>> {
> >>>> VimcCameraData *data = cameraData(camera);
> >>>> data->video_->streamOff();
> >>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >>>> index 38edd00cebad..0f9fec1b618f 100644
> >>>> --- a/src/libcamera/pipeline_handler.cpp
> >>>> +++ b/src/libcamera/pipeline_handler.cpp
> >>>> @@ -268,8 +268,7 @@ void PipelineHandler::unlock()
> >>>> */
> >>>>
> >>>> /**
> >>>> - * \fn PipelineHandler::stop()
> >>>> - * \brief Stop capturing from all running streams
> >>>> + * \brief Stop capturing from all running streams and cancel pending requests
> >>>> * \param[in] camera The camera to stop
> >>>> *
> >>>> * This function stops capturing and processing requests immediately. All
> >>>> @@ -277,6 +276,30 @@ void PipelineHandler::unlock()
> >>>> *
> >>>> * \context This function is called from the CameraManager thread.
> >>>> */
> >>>> +void PipelineHandler::stop(Camera *camera)
> >>>> +{
> >>>> + /* Stop the pipeline handler and let the queued requests complete. */
> >>>> + stopDevice(camera);
> >>>> +
> >>>
> >>> I /think/ it's fine - but are we certain that nothing on the
> >>> waitingRequests_ will get queued between stopDevice above and clearing
> >>> below?
> >>
> >> The camera state machine shouldn't allow new Requests to be queued if
> >> the camera is stopped and the Camera::stop() documentations says
> >>
> >> * \context This function may be called in any camera state as defined in \ref
> >> * camera_operation, and shall be synchronized by the caller with other
> >> * functions that affect the camera state. If called when the camera isn't
> >> * running, it is a no-op.
> >
> > My concern wasn't so much from external requests being queued, but from
> > any potential event triggering (such as a fence completion) that might
> > move a request from the waitingRequests_ to the data->queuedRequests_()
> > at this point, before we clear it down...
We're single threaded, so as long as we make sure to really stop
everything in stop() to prevent any further event from being generated
once control returns to the event loop, it should be fine.
> >>>> + /* Cancel and signal as complete all requests waiting for fences. */
> >>>> + MutexLocker lock(waitingRequestsMutex_);
> >
> > Perhaps taking this lock before calling stopDevice() (if allowed) would
> > prevent that possibility?
>
> As mentioned in previous patch, I am still un-sure why do we need this
> lock or I don't understand if any other thread is trying to access
> waitingRequests_ (either reading or queuing to it).
I don't think we need the lock.
> >>>> +
> >>>> + for (auto &waitingRequest : waitingRequests_) {
> >>>> + waitingRequest->cancel();
> >>>> + completeRequest(waitingRequest);
> >>>> + }
> >>>> +
> >>>> + waitingRequests_.clear();
> >>>
> >>> I'd be very happy to see:
> >>>
> >>> Camera::Private *data = camera->_d();
> >>> ASSERT(data->queuedRequests_.empty());
> >>>
> >>> as an enforcement to be sure that all requests are completed by the end
> >>> of this function.
> >>
> >> Afaict stopDevice() is synchronous, it should complete all requests in
> >> flight in error state before returning
>
> yes, stopDevice() (earlier's stop()) will complete all request, so it's
> safe to assume all in flight requests are completed.
>
> >>> But I think I can get away with this on this patch:
> >>
> >> Might be a useful validation to ASSERT though
>
> Agreed, I see no harm (and more tightening)
>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \fn PipelineHandler::stopDevice()
> >>>> + * \brief Stop capturing from all running streams
> >>>> + * \param[in] camera The camera to stop
> >>>> + *
> >>>> + * This function stops capturing and processing requests immediately. All
> >>>> + * pending requests are cancelled and complete immediately in an error state.
> >>>> + */
> >>>>
> >>>> /**
> >>>> * \brief Determine if the camera has any requests pending
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list