[libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce stopDevice()
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 9 18:54:12 CET 2021
Quoting Jacopo Mondi (2021-11-09 17:47:26)
> 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.
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...
>
> >
> >
> >
> > > + /* 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?
> > > +
> > > + 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