[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint
Umang Jain
umang.jain at ideasonboard.com
Wed Dec 14 09:41:12 CET 2022
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.
>>
>> 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 ?
>
> 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.
> 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.
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.
>
> 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