[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint
Paul Elder
paul.elder at ideasonboard.com
Thu Dec 15 06:18:22 CET 2022
Hi Jacopo,
On Wed, Dec 14, 2022 at 02:41:30PM +0100, Jacopo Mondi wrote:
> Hi Paul
>
> On Wed, Dec 14, 2022 at 09:13:06PM +0900, Paul Elder wrote:
> > Hi,
> >
> > On Wed, Dec 14, 2022 at 10:19:54AM +0100, Jacopo Mondi via libcamera-devel wrote:
> > > Hi Umang
> > >
> > > On Wed, Dec 14, 2022 at 02:11:12PM +0530, Umang Jain wrote:
> > > > Hi Jacopo
> > > >
> > > > On 12/14/22 1:29 PM, Jacopo Mondi wrote:
> > > > > 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.
> >
> > (Besides the fact that I can't find properties::PipelineDepth)
> > controls::PipelineDepth says that it "Specifies the number of pipeline
> > stages the frame went through from when it was exposed to when the final
> > completed result was available to the framework.", which to me sounds
> > like the *minimum* amount of in-flight requests.
>
> It's a draft control copied in to please Android.
> My understanding was that it had to be made a non-mutable property
Ah, indeed it says that :)
>
> >
> > I thought that here we're defining the *maximum* allowed in-flight
> > requests. Not that all of the requests have to actually be doing
> > something in hardware, but that it's the amount of requests the the
> > pipeline handler is capable of handling internally without overflowing.
> >
> > > > > >
> > > > > > 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.
> > > >
> > > > You mean via a global const variable like it's done in Patch 1/2 ?
> > >
> > > Yes, like in patch 1/2
> > >
> > >
> > > > >
> > > > > 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
> > > >
> > > > I think it's a over-kill here. queueRequest() is a recurring call and I
> > > > don't want fetching of PipelineDepth here.
> >
> > Oh one hand, yeah imo getting it from Camera::properties() is more
> > correct, but on the other hand it does indeed seem overkill.
> >
> > >
> > > While I consider a lookup on an unordered_map<> of a few items not
> > > that expensive, I agree it's in an hot path, and it would be better to
> > > cache the value somewhere. Caching it inside Camera or Camera::Private
> > > is a duplication, as it's already part of
> > > Camera::Private::properties_, but I would be fine with the
> > > PipelineHandler base class doing something like this (as proposed in
> > > my previous 2) point below)
> > >
> > > void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> > > {
> > >
> > > /* Cache the pipeline depth in the Camera private data. */
> > > const auto &pipelineDepth = camera->properties().get(properties::PipelineDepth);
> > >
> > > Camera::Private *data = camera->_d();
> > > data->pipelineDepth = pipelineDepth ? 0 : *pipelineDepth;
> > >
> > > ...
> > > }
> > >
> > > Otherwise the PipelineHandler base class can be added with a class
> > > member, and each registred camera will overwrite that value, it's less
> > > nice, but all cameras from the same PH instance will have the same
> > > pipeline depth, so...
> >
> > Perhaps caching would be a good middle ground.
> >
> > >
> > >
> > > > > 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..
> > > >
> > > > That's interesting because I initially didn't think of multiple cameras
> > > > registered with one instance of pipeline-handler.
> > > > It also makes me wonder -
> > > >
> > > > - Is properties::PipelineDepth Camera specific or platform-specific ? Seems
> > > > now it's Camera specific
> > > > - Then defining it IPA headers won't be a good option, in cases where
> > > > PipelineDepth for different cameras differ.
> > >
> > > Why camera specific ? Each camera will indeed register it as a
> > > property for applications to retrieve it, but the
> > > value depends on the platform afaict and it can be a compile-time constant
> > > defined by the IPA headers. Even better, the same compile-time
> > > constant will be used to size the FCQ.
> >
> > If it's platform-specific then wouldn't the pipeline handler have to
> > report it based on what platform it's running on? So we can't really
> >hardcode a constant. Maybe putting it in properties is indeed better.
>
> As far as I get it, this property applies to pipelines with an ISP (no
> uvc, no simple) and those pipelines by definition runs on a single
> platform, don't they ?
Well, the rkisp1 pipeline runs on imx8mp too... (with a few on-the-list
but out-of-tree kernel patches)
>
> >
> > >
> > > >
> > > > Need some thinking ...
> > > >
> > > > But none the less, this is a something that can be done on top with wider
> > > > discussion / context on PipelineDepth. It shouldn't necessarily block the
> > > > series.
> > >
> > > My whole point is that I think we should find a better mechanism for
> > > registering pipeline depth compared to having pipeline handlers call
> > > setMaxQueueRequests() as it requires an explicit action that can
> > > easily be overlooked, while we want the queue overflow protection to
> >
> > I mean, we could initialize kMaxFrameContexts to an invalid value (-1?)
> > and then force *all* pipeline handlers to explictly set it, even if to
> > zero. Then it becomes really easy to catch.
>
> The property doesn't make much sense for UVC, right ?
>
> Not all pipelines will use libipa and have an FCQ (RPi in example at
> the moment). I don't think forcing all of them is actually necessary.
Hm, that's true. No initial invalid value then.
>
> But the pipelines that do need to throttle the number of requests in
> flight with the help of the PipelineHandler base class should ideally
> get it for free by simply registering the property ?
Yeah that does sound more nice.
>
> >
> > Or note it down as a compliancy thing and have lc-compliance test for
> > it. I presume this will become a required property to be exposed anyway.
> >
> > In any case, the goal is to report to the user the "maximum number of
> > requests that are allowed to be queued in the camera", right? So it'll
> > have to go through properties anyway. Perhaps then it's best to expose
> > it via properties (as it's already the interface we have for exposing
> > properties to the user), and then the base PipelineHandler can cache it
> > for the queuing guard. That would get rid of the special explicit call,
> > and would still achieve the goal.
>
> That would be my preference. There might be a discussions if
> properties::PipelineDepth is the right property to report it or it
> will conflict with Android's pipeline depth, which is different.
+1
Although to me this sounds separate from PipelineDepth, because my
understanding is that that is the maximum amount of Requests that can be
in-flight, while what we want here is that plus some extra that the
application is allowed to "overqueue" that the pipeline handler will
automagically handle internally.
Am I misunderstanding something?
Paul
>
> >
> > I suppose then we'd actually need a compliance test for the property,
> > but I guess we have a few more of those anyway.
> >
> > My two cents.
>
> Thanks ;)
> j
>
> >
> >
> > Paul
> >
> > > happen transparently if a pipeline handler register
> > > properties::PipelineDepth ? Am I overthinking this ?
> > >
> > >
> > > > >
> > > > > 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