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

[Umang Jain]

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
>
> 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)

So I think queue request time is quite late there. Camera registeration 
time might be possible with some changes (looking at 
PipelineHandlerIPU3::registerCameras())


>
> 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
>>