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

[Umang Jain]

Hi Kieran, Naushir,

On 12/15/22 4:18 PM, Kieran Bingham via libcamera-devel wrote:
> 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

Are we advertising this as a standard policy ? If yes, I want this more 
explicit to be documented since I have had a hard time looking for it.

Though the PipelineDepth is still under discussion, it was asssumed 
(till today?) that applications would likely queue requests within a 
certain bound.
>>> 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.

The queuing of 20 requests in one go - sounds like a special case to me. 
Usually, applications (atleast what I expected) queue a certain number 
of requests and wait for their completion (i.e. the requestCompleted 
callback to invoke) and then queue more requests through the callback 
handler.
> 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.

I agree that this can happen. More so, when the pipeline depth (no. of 
processing blocks each frame) is huge.

But what are we really doing here - we are transferring the intended 
behavior(as above) from application's requestCompleted callback to 
pipeline-handler base class. So conceptually both the behaviour are 
similar in my opinion. However, stating/document the infinite queuing of 
requests for application's PoV would be a nice addition on top and would 
make the corresponding  patch much more obvious.

Apart from that, it would be nice to document -

             Requests required to be queued to capture X frames

  Since this is also something I didn't find explicit information on and 
it would possibly be dependent on pipeline-depth (no. of processing 
blocks) (?). There is a lc-compliance test regarding this - "Test single 
capture cycles" that test X request for X captures but on the other 
hand, some tests get skipped with the following error:

[7:43:48.210256888] [55043]  INFO Camera camera.cpp:1026 configuring 
streams: (0) 640x480-MJPEG
Camera needs 4 requests, can't test only 2
../src/apps/lc-compliance/simple_capture.cpp:91: Skipped

So I suspect some grey area here as well ? Possibly I need to tinker on 
other platforms to really use how many requests it takes to capture X 
no. of frames ...
>
> --
> Kieran