[libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce stopDevice()
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 9 18:47:26 CET 2021
Hi Kieran
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.
>
>
>
> > + /* 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.
Afaict stopDevice() is synchronous, it should complete all requests in
flight in error state before returning
>
> But I think I can get away with this on this patch:
>
Might be a useful validation to ASSERT though
>
> 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