[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 14 08:59:08 CET 2022
Hi Umang
On Wed, Dec 14, 2022 at 11:07:25AM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> On 12/13/22 9:25 PM, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Mon, Dec 12, 2022 at 08:32:56PM +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 (for e.g. the IPA).
> > It's my understanding that the max number of in-flight requests is
> > what the properties::PipelineDepth property expresses.
>
> Indeed, that's why understanding as well. The properties::PipelineDepth
> should be set by the pipeline-handlers.
>
> We need to identify and atleast agree upon the correct PipelineDepth for the
> two platforms that uses the FCQueue (IPU3 and RkISP1) as well. Some grey
> areas there as well
I think 16 is fine for now. At least doesn't change what we already
have today
> >
> > Can't we access that property at queue request time, or maybe better we
> > could cache it at camera registration time, since it's constant ?
>
> If we intent to use the properties::PipelineDepth, as the size of the
> FCQueue - it needs to exist / set before IPA is constructed.
>
> (See IPAIPU3::IPAIPU3() for context)
>
The property can still be defined in the IPA header as IPA can access
it safely.
Pipeline Handlers will have to register the property -before- the
camera is registered, but looking at IPU3 that's already the case now
(data->properties_ gets populated before calling registerCamera())
> So I think queue request time is quite late there. Camera registeration time
> might be possible with some changes (looking at
> PipelineHandlerIPU3::registerCameras())
What I meant is either
1) At PipelineHandler::queueRequest() time, the PipelineHandler base
class accesses Camera::properties() and fetches
properties::PipelineDepth
2) At PipelineHandler::registerCamera() time the property value is
cached, but this would require to cache per-camera data in
PipelineHandler, something we don't have, or to populate a class
member of Camera::Private for that purpose..
Sorry for not having been clear
>
>
> >
> > Also, can this feature be tested in test/ ? Probably not, as it
> > requires a compliant pipeline handler ? Maybe vimc can be instrumented
>
> I have been thinking about that as well. It's shouldn't be too difficult.
> > ?
> >
> > > 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 PipelineHandler
> > > 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 and RkISP1 pipeline handlers. This will
> > > prevent over-writing of frame contexts (i.e. FCQueue) in these two
> > > pipeline handlers.
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > include/libcamera/internal/pipeline_handler.h | 4 ++
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 1 +
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +
> > > src/libcamera/pipeline_handler.cpp | 51 ++++++++++++++++++-
> > > 4 files changed, 56 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > index ec4f662d..83f8bd9f 100644
> > > --- a/include/libcamera/internal/pipeline_handler.h
> > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > @@ -70,6 +70,7 @@ public:
> > > 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;
> > > @@ -97,6 +98,9 @@ private:
> > > Mutex lock_;
> > > unsigned int useCount_ LIBCAMERA_TSA_GUARDED_BY(lock_);
> > >
> > > + uint32_t maxQueueRequests_;
> > > + uint32_t requestsQueueCounter_;
> > > +
> > > friend class PipelineHandlerFactoryBase;
> > > };
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index e4d79ea4..d1d42f78 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -422,6 +422,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> > > : PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
> > > {
> > > + setMaxQueueRequests(ipa::ipu3::kMaxFrameContexts);
> > > }
> > >
> > > std::unique_ptr<CameraConfiguration>
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index eb9ad65c..a48adba9 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -599,6 +599,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> > > : PipelineHandler(manager), hasSelfPath_(true)
> > > {
> > > + setMaxQueueRequests(ipa::rkisp1::kMaxFrameContexts);
> > > }
> > >
> > > std::unique_ptr<CameraConfiguration>
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index cfade490..103f9db0 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -67,7 +67,8 @@ LOG_DEFINE_CATEGORY(Pipeline)
> > > * through the PipelineHandlerFactoryBase::create() function.
> > > */
> > > PipelineHandler::PipelineHandler(CameraManager *manager)
> > > - : manager_(manager), useCount_(0)
> > > + : manager_(manager), useCount_(0), maxQueueRequests_(0),
> > > + requestsQueueCounter_(0)
> > > {
> > > }
> > >
> > > @@ -428,6 +429,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_) {
> > > @@ -455,6 +459,10 @@ void PipelineHandler::doQueueRequests()
> > > if (!request->_d()->prepared_)
> > > break;
> > >
> > > + if (maxQueueRequests_ &&
> > > + requestsQueueCounter_ >= maxQueueRequests_)
> > > + break;
> > > +
> > > doQueueRequest(request);
> > > waitingRequests_.pop();
> > > }
> > > @@ -531,6 +539,9 @@ void PipelineHandler::completeRequest(Request *request)
> > > ASSERT(!req->hasPendingBuffers());
> > > data->queuedRequests_.pop_front();
> > > camera->requestComplete(req);
> > > +
> > > + if (maxQueueRequests_)
> > > + requestsQueueCounter_--;
> > > }
> > > }
> > >
> > > @@ -647,6 +658,44 @@ 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.
> > > + */
> > > +
> > > +/**
> > > + * \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.38.1
> > >
>
More information about the libcamera-devel
mailing list