[libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce stopDevice()
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 9 15:49:42 CET 2021
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?
> + /* Cancel and signal as complete all requests waiting for fences. */
> + MutexLocker lock(waitingRequestsMutex_);
> +
> + 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.
But I think I can get away with this on this patch:
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
> --
> 2.33.1
>
More information about the libcamera-devel
mailing list