[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint

[Kieran Bingham]

Quoting Naushir Patuck (2022-12-15 09:59:51)
> On Thu, 15 Dec 2022 at 09:52, Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
> 
> > Quoting Naushir Patuck via libcamera-devel (2022-12-15 09:43:55)
> > > 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?
> >
> > I think I saw Jacopo confirm the same in a parallel reply here, but I
> > understood this to be something we shouldn't expose to applications.
> > Applications can always choose to queue an unlimited number of requests
> > to a Camera (within the limits of the universe) ... but it's the
> > pipeline handlers responsibility to ensure that requests are not queued
> > beyond the capabilities of the IPA. As such - requests may stay 'queued'
> > in the pipeline handler or CameraData object until the IPA is ready to
> > process the next one.
> >
> 
> I agree that the application never needs to know the maximum bound for
> requests.
> But I still see a problem where any pending requests queued
> in PipelineHandler::waitingRequests_
> will never make it to PipelineHandlerXXX::queueRequestDevice() if the
> application
> does not queue any further requests to the camera.

Oh - I see what you mean, yes - if we only process the queue when
requests are 'queued' then this could occur. We'll have to make sure we
prod the queue when a request completes too, which makes sense, as that
is the event that free's up the opportunity to process the next request.

--
Kieran