[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint
Naushir Patuck
naush at raspberrypi.com
Thu Dec 15 10:43:55 CET 2022
Hi all,
On Thu, 15 Dec 2022 at 05:18, Paul Elder via libcamera-devel <
libcamera-devel at lists.libcamera.org> wrote:
> 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.
>
My understanding of PipelineDepth is in-line with the above - it gives a
minimum
number of in-flight requests needed for the pipeline handler. This new
property must give an upper bound on the number of in-flight requests the
pipeline
handler can handle. For the RPi case, the min is 1 and max is unbounded.
I can see a possible bug(?) introduced in this series however. Say a
pipeline
handler sets maxQueueRequest to 16. An application comes along and queues
20
requests and then just waits for completion of those 20 requests. The
PipelineHandler base class calls queueRequestDevice for the first 16
requests,
but nothing that I can see will trigger queuing of the last 4 requests once
the
pipeline handler has completed the first N requests. Unless I've missed
something?
Regards,
Naush
> 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
> > > > > > > > >
> > > > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221215/4dd518b2/attachment.htm>
More information about the libcamera-devel
mailing list