[libcamera-devel] [PATCH v3 3/4] libcamera: pipeline-handler: Consider max in-flight requests constraint
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 5 18:35:15 CEST 2022
Quoting Umang Jain via libcamera-devel (2022-06-20 14:14:45)
> Hi Jacopo
>
> On 6/20/22 16:12, Jacopo Mondi wrote:
> > Hi Umang
> > to be honest I was expecting this to be much simpler, with a the
> > size of the FCQueue being a compile-time constant (exposed as camera
> > property) and the ph base class counting the number of in-flight
> > requests against it.
> >
> > I might have missed the use case for run-time configuration of the
> > FCQueue size...
>
>
> Or I went one step ahead and tried to address run-time configuration as
> well.
> To me, it seems FCQueue size should be set in "configure" phase so it seems
> a run-time configuration to me from that angle. For example, a camera
>
> start() > stop() > re-configure (maybe needs higher FCQueue size this
> time) > start again
I believe the size of the FCQueue can always be constant (as defined by a
pipeline handler/IPA combination).
The requirement for the sizing is dependant on pipeline depths, not any
specific configuration of the session - so it should not require any
resize dependant upon the camera configuration.
--
Kieran
>
> so..
>
> And I am not sure if camera property is the right 'place' to put a
> integer compile
> time constant describing the FCQueue's size. It simply doesn't fit in my
> head
> but I can be wrong; I also don't have any other alternative to suggest yet
> (hence the \todo on maxQueueRequests in this patch). Let's see what comes
> out of the discussion .
>
> >
> > On Mon, Jun 20, 2022 at 02:10:23AM +0530, Umang Jain via libcamera-devel wrote:
> >> This patch introduces a new constraint in the pipeline handler base
> >> class which allows various derived pipeline handlers to set and
> >> restrict the number of maximum in-flight requests that can be queued to
> >> the underlying components.
> >>
> >> The pipeline handler is now equipped with the responsibility of not to
> >> over queue the requests to the underlying layers. The derived
> >> pipeline handler (or even IPA) can also have various kind of requests
> >> queue(arrays, queues or ring-buffer) hence, it might be an issue where
> >> the application queues the requests at a rate where these kind of
> >> queues can over-flow. The patch ensures that the base pipeline-handler
> >> will never let the requests overflow to underlying layers, once the
> >> derived pipeline handler sets its maximum capacity of handling
> >> in-flight requests using PipelineHandler::setMaxQueueRequests().
> >>
> >> The queue request management introduced in the pipeline handler base
> >> class is now used by the IPU3 pipeline handler.
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> include/libcamera/internal/pipeline_handler.h | 6 ++
> >> include/libcamera/ipa/ipu3.mojom | 1 +
> >> src/ipa/ipu3/ipa_context.cpp | 18 ++++--
> >> src/ipa/ipu3/ipa_context.h | 5 +-
> >> src/ipa/ipu3/ipu3.cpp | 3 +-
> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++
> >> src/libcamera/pipeline_handler.cpp | 61 ++++++++++++++++++-
> >> 7 files changed, 94 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >> index c3e4c258..5a319011 100644
> >> --- a/include/libcamera/internal/pipeline_handler.h
> >> +++ b/include/libcamera/internal/pipeline_handler.h
> >> @@ -67,9 +67,12 @@ public:
> >>
> >> const char *name() const { return name_; }
> >>
> >> + uint32_t maxQueueRequests() { return maxQueueRequests_; }
> >> +
> >> protected:
> >> void registerCamera(std::shared_ptr<Camera> camera);
> >> void hotplugMediaDevice(MediaDevice *media);
> >> + void setMaxQueueRequests(uint32_t maxRequests);
> >>
> >> virtual int queueRequestDevice(Camera *camera, Request *request) = 0;
> >> virtual void stopDevice(Camera *camera) = 0;
> >> @@ -88,6 +91,9 @@ private:
> >>
> >> std::queue<Request *> waitingRequests_;
> >>
> >> + uint32_t maxQueueRequests_;
> >> + uint32_t requestsQueueCounter_;
> >> +
> >> const char *name_;
> >>
> >> Mutex lock_;
> >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> >> index d1b1c6b8..3e7bbc6e 100644
> >> --- a/include/libcamera/ipa/ipu3.mojom
> >> +++ b/include/libcamera/ipa/ipu3.mojom
> >> @@ -14,6 +14,7 @@ struct IPAConfigInfo {
> >> libcamera.ControlInfoMap lensControls;
> >> libcamera.Size bdsOutputSize;
> >> libcamera.Size iif;
> >> + uint32 maxQueueRequests;
> >> };
> >>
> >> interface IPAIPU3Interface {
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 60c58b97..564f8e6c 100644
> >> --- a/src/ipa/ipu3/ipa_context.cpp
> >> +++ b/src/ipa/ipu3/ipa_context.cpp
> >> @@ -248,7 +248,7 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> >> * \brief FCQueue constructor
> >> */
> >> FCQueue::FCQueue()
> >> - : latestFC_(0), oldestFC_(0), isFull_(false)
> >> + : latestFC_(0), oldestFC_(0), size_(0), isFull_(false)
> >> {
> >> }
> >>
> >> @@ -269,7 +269,7 @@ FCQueue::FCQueue()
> >> IPAFrameContext *FCQueue::get(uint32_t frame)
> >> {
> >> if (latestFC_ && frame <= latestFC_) {
> >> - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
> >> + IPAFrameContext &frameContext = this->at(frame % size_);
> >> if (frame != frameContext.frame)
> >> LOG(IPAIPU3, Warning)
> >> << "Got wrong frame context for frame " << frame;
> >> @@ -287,10 +287,10 @@ IPAFrameContext *FCQueue::get(uint32_t frame)
> >> latestFC_ = frame;
> >>
> >> /* Check if the queue is full to avoid over-queueing later */
> >> - if (oldestFC_ && latestFC_ - oldestFC_ >= kMaxFrameContexts - 1)
> >> + if (oldestFC_ && latestFC_ - oldestFC_ >= size_ - 1)
> >> isFull_ = true;
> >>
> >> - return &this->at(latestFC_ % kMaxFrameContexts);
> >> + return &this->at(latestFC_ % size_);
> >> }
> >> }
> >>
> >> @@ -329,6 +329,7 @@ void FCQueue::completeFrame(uint32_t frame)
> >> */
> >> void FCQueue::setSize(uint32_t size)
> >> {
> >> + size_ = size;
> >> resize(size);
> >> }
> >>
> >> @@ -338,6 +339,12 @@ void FCQueue::setSize(uint32_t size)
> >> * \return True of FCQueue is full, False otherwise
> >> */
> >>
> >> +/**
> >> + * \fn FCQueue::size()
> >> + * \brief The number of IPAFrameContext(s) FCQueue handles
> >> + * \return Returns the size of FCQueue
> >> + */
> >> +
> >> /**
> >> * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
> >> */
> >> @@ -345,6 +352,9 @@ void FCQueue::reset()
> >> {
> >> clear();
> >>
> >> + if (size_)
> >> + setSize(size_);
> >> +
> >> isFull_ = false;
> >>
> >> latestFC_ = 0;
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index 52a0b5e7..0da2ad26 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -21,9 +21,6 @@ namespace libcamera {
> >>
> >> namespace ipa::ipu3 {
> >>
> >> -/* Maximum number of frame contexts to be held */
> >> -static constexpr uint32_t kMaxFrameContexts = 16;
> >> -
> >> struct IPASessionConfiguration {
> >> struct {
> >> ipu3_uapi_grid_config bdsGrid;
> >> @@ -100,10 +97,12 @@ public:
> >>
> >> IPAFrameContext *get(uint32_t frame);
> >> bool isFull() { return isFull_; }
> >> + uint32_t size() { return size_; }
> >>
> >> private:
> >> uint32_t latestFC_;
> >> uint32_t oldestFC_;
> >> + uint32_t size_;
> >>
> >> bool isFull_;
> >> };
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index 15070fa0..3218df49 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -458,8 +458,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >>
> >> /* Clean IPAActiveState at each reconfiguration. */
> >> context_.activeState = {};
> >> +
> >> context_.frameContexts.reset();
> >> - context_.frameContexts.setSize(kMaxFrameContexts);
> >> + context_.frameContexts.setSize(configInfo.maxQueueRequests);
> >>
> >> if (!validateSensorControls()) {
> >> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index b7dda282..d599fb95 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -672,6 +672,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >> configInfo.bdsOutputSize = config->imguConfig().bds;
> >> configInfo.iif = config->imguConfig().iif;
> >>
> >> + /*
> >> + * \todo Find a better position where maxQueueRequests can be
> >> + * defined and fetched. Maybe in IPA Interface? Camera property?
> >> + * Not sure.
> >> + */
> >> + uint32_t maxQueueRequests = 16;
> >> + configInfo.maxQueueRequests = maxQueueRequests;
> >> + setMaxQueueRequests(maxQueueRequests);
> >> +
> >> ret = data->ipa_->configure(configInfo, &data->ipaControls_);
> >> if (ret) {
> >> LOG(IPU3, Error) << "Failed to configure IPA: "
> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> index 7ebd76ad..08ec28de 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >> * respective factories.
> >> */
> >> PipelineHandler::PipelineHandler(CameraManager *manager)
> >> - : manager_(manager), lockOwner_(false)
> >> + : manager_(manager), maxQueueRequests_(0), requestsQueueCounter_(0),
> >> + lockOwner_(false)
> >> {
> >> }
> >>
> >> @@ -312,6 +313,9 @@ void PipelineHandler::stop(Camera *camera)
> >> /* Make sure no requests are pending. */
> >> Camera::Private *data = camera->_d();
> >> ASSERT(data->queuedRequests_.empty());
> >> +
> >> + maxQueueRequests_ = 0;
> >> + requestsQueueCounter_ = 0;
> >> }
> >>
> >> /**
> >> @@ -397,6 +401,9 @@ void PipelineHandler::doQueueRequest(Request *request)
> >> Camera::Private *data = camera->_d();
> >> data->queuedRequests_.push_back(request);
> >>
> >> + if (maxQueueRequests_)
> >> + requestsQueueCounter_++;
> >> +
> >> request->_d()->sequence_ = data->requestSequence_++;
> >>
> >> if (request->_d()->cancelled_) {
> >> @@ -424,6 +431,10 @@ void PipelineHandler::doQueueRequests()
> >> if (!request->_d()->prepared_)
> >> break;
> >>
> >> + if (maxQueueRequests_ &&
> >> + requestsQueueCounter_ >= maxQueueRequests_)
> >> + break;
> >> +
> >> doQueueRequest(request);
> >> waitingRequests_.pop();
> >> }
> >> @@ -500,6 +511,9 @@ void PipelineHandler::completeRequest(Request *request)
> >> ASSERT(!req->hasPendingBuffers());
> >> data->queuedRequests_.pop_front();
> >> camera->requestComplete(req);
> >> +
> >> + if (maxQueueRequests_)
> >> + requestsQueueCounter_--;
> >> }
> >> }
> >>
> >> @@ -616,6 +630,51 @@ void PipelineHandler::disconnect()
> >> * constant for the whole lifetime of the pipeline handler.
> >> */
> >>
> >> +/**
> >> + * \var PipelineHandler::maxQueueRequests_
> >> + * \brief Maximum number of in-flight requests that can be queued
> >> + *
> >> + * A hardware can handle a certain number of maximum requests at a given
> >> + * point. If such a constraint exists, set maxQueueRequests_ via
> >> + * \a setMaxQueueRequests() in the derived pipeline handler.
> >> + *
> >> + * The derived pipeline handler can choose not to define such constraint as
> >> + * well. In that case, the derived pipeline handler can avoid setting
> >> + * \a setMaxQueueReqeuests(), hence \a maxQueueRequests_ and
> >> + * \a requestsQueueCounter_ will be 0.
> >> + */
> >> +
> >> +/**
> >> + * \var PipelineHandler::requestsQueueCounter_
> >> + * \brief Number of requests queued to the underlying hardware
> >> + *
> >> + * If \a setMaxQueueRequests() is set by the derived pipeline handler,
> >> + * requestsQueueCounter_ reflects the number of requests queued
> >> + * to the underlying hardware by the pipeline handler.
> >> + */
> >> +
> >> +/**
> >> + * \fn PipelineHandler::maxQueueRequests()
> >> + * \brief Retrieve the maximum number of in-flight requests handled by the
> >> + * underlying hardware
> >> + * \return Number of requests that can be queued to underlying hardware
> >> + */
> >> +
> >> +/**
> >> + * \brief Sets the maximum number of requests that can be queued
> >> + * \param[in] maxRequests Maximum number of in-flight requests
> >> + *
> >> + * A hardware can handle a certain number of requests at a given point.
> >> + * This function sets the maximum number of in-flight requests that can
> >> + * be queued to the hardware by the pipeline handler. Each derived pipeline
> >> + * handler should set the maximum number of in-flight requests it can handle
> >> + * at a given point using this function, if at all such a constraint exists.
> >> + */
> >> +void PipelineHandler::setMaxQueueRequests(uint32_t maxRequests)
> >> +{
> >> + maxQueueRequests_ = maxRequests;
> >> +}
> >> +
> >> /**
> >> * \fn PipelineHandler::name()
> >> * \brief Retrieve the pipeline handler name
> >> --
> >> 2.31.1
> >>
More information about the libcamera-devel
mailing list