[libcamera-devel] [PATCH 08/10] libcamera: pipeline: Introduce stopDevice()
Umang Jain
umang.jain at ideasonboard.com
Wed Nov 10 14:01:09 CET 2021
Hello,
The patch looks fine to me as such,
On 11/9/21 11:24 PM, Kieran Bingham wrote:
> 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?
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).
>
>
>>>> +
>>>> + 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
>>>> --
>>>> 2.33.1
>>>>
More information about the libcamera-devel
mailing list