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

[Umang Jain]

Hi again,

one more comment on this front,

On 12/15/22 9:34 PM, Umang Jain via libcamera-devel wrote:
> 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

It's not actually upon queuing of request. The waitingRequests_ is 
processed when the request has finished 'preparing' i.e. 
doQueueRequest() is a callback to Request::Private::prepared signal.

So it depends where/how to invoke the Request::Private::Prepare() - even 
after you queue the Request.
>> 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
>