[libcamera-devel] [PATCH v3 3/4] libcamera: pipeline-handler: Consider max in-flight requests constraint
Umang Jain
umang.jain at ideasonboard.com
Mon Jun 20 15:14:45 CEST 2022
Hi Jacopo
On 6/20/22 16:12, Jacopo Mondi wrote:
> Hi Umang
> to be honest I was expecting this to be much simpler, with a the
> size of the FCQueue being a compile-time constant (exposed as camera
> property) and the ph base class counting the number of in-flight
> requests against it.
>
> I might have missed the use case for run-time configuration of the
> FCQueue size...
Or I went one step ahead and tried to address run-time configuration as
well.
To me, it seems FCQueue size should be set in "configure" phase so it seems
a run-time configuration to me from that angle. For example, a camera
start() > stop() > re-configure (maybe needs higher FCQueue size this
time) > start again
so..
And I am not sure if camera property is the right 'place' to put a
integer compile
time constant describing the FCQueue's size. It simply doesn't fit in my
head
but I can be wrong; I also don't have any other alternative to suggest yet
(hence the \todo on maxQueueRequests in this patch). Let's see what comes
out of the discussion .
>
> On Mon, Jun 20, 2022 at 02:10:23AM +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.
>>
>> 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 pipeline-handler
>> 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 pipeline handler.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> include/libcamera/internal/pipeline_handler.h | 6 ++
>> include/libcamera/ipa/ipu3.mojom | 1 +
>> src/ipa/ipu3/ipa_context.cpp | 18 ++++--
>> src/ipa/ipu3/ipa_context.h | 5 +-
>> src/ipa/ipu3/ipu3.cpp | 3 +-
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++
>> src/libcamera/pipeline_handler.cpp | 61 ++++++++++++++++++-
>> 7 files changed, 94 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> index c3e4c258..5a319011 100644
>> --- a/include/libcamera/internal/pipeline_handler.h
>> +++ b/include/libcamera/internal/pipeline_handler.h
>> @@ -67,9 +67,12 @@ public:
>>
>> const char *name() const { return name_; }
>>
>> + uint32_t maxQueueRequests() { return maxQueueRequests_; }
>> +
>> 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;
>> @@ -88,6 +91,9 @@ private:
>>
>> std::queue<Request *> waitingRequests_;
>>
>> + uint32_t maxQueueRequests_;
>> + uint32_t requestsQueueCounter_;
>> +
>> const char *name_;
>>
>> Mutex lock_;
>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>> index d1b1c6b8..3e7bbc6e 100644
>> --- a/include/libcamera/ipa/ipu3.mojom
>> +++ b/include/libcamera/ipa/ipu3.mojom
>> @@ -14,6 +14,7 @@ struct IPAConfigInfo {
>> libcamera.ControlInfoMap lensControls;
>> libcamera.Size bdsOutputSize;
>> libcamera.Size iif;
>> + uint32 maxQueueRequests;
>> };
>>
>> interface IPAIPU3Interface {
>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
>> index 60c58b97..564f8e6c 100644
>> --- a/src/ipa/ipu3/ipa_context.cpp
>> +++ b/src/ipa/ipu3/ipa_context.cpp
>> @@ -248,7 +248,7 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
>> * \brief FCQueue constructor
>> */
>> FCQueue::FCQueue()
>> - : latestFC_(0), oldestFC_(0), isFull_(false)
>> + : latestFC_(0), oldestFC_(0), size_(0), isFull_(false)
>> {
>> }
>>
>> @@ -269,7 +269,7 @@ FCQueue::FCQueue()
>> IPAFrameContext *FCQueue::get(uint32_t frame)
>> {
>> if (latestFC_ && frame <= latestFC_) {
>> - IPAFrameContext &frameContext = this->at(frame % kMaxFrameContexts);
>> + IPAFrameContext &frameContext = this->at(frame % size_);
>> if (frame != frameContext.frame)
>> LOG(IPAIPU3, Warning)
>> << "Got wrong frame context for frame " << frame;
>> @@ -287,10 +287,10 @@ IPAFrameContext *FCQueue::get(uint32_t frame)
>> latestFC_ = frame;
>>
>> /* Check if the queue is full to avoid over-queueing later */
>> - if (oldestFC_ && latestFC_ - oldestFC_ >= kMaxFrameContexts - 1)
>> + if (oldestFC_ && latestFC_ - oldestFC_ >= size_ - 1)
>> isFull_ = true;
>>
>> - return &this->at(latestFC_ % kMaxFrameContexts);
>> + return &this->at(latestFC_ % size_);
>> }
>> }
>>
>> @@ -329,6 +329,7 @@ void FCQueue::completeFrame(uint32_t frame)
>> */
>> void FCQueue::setSize(uint32_t size)
>> {
>> + size_ = size;
>> resize(size);
>> }
>>
>> @@ -338,6 +339,12 @@ void FCQueue::setSize(uint32_t size)
>> * \return True of FCQueue is full, False otherwise
>> */
>>
>> +/**
>> + * \fn FCQueue::size()
>> + * \brief The number of IPAFrameContext(s) FCQueue handles
>> + * \return Returns the size of FCQueue
>> + */
>> +
>> /**
>> * \brief Clear the FCQueue by resetting all the entries in the ring-buffer
>> */
>> @@ -345,6 +352,9 @@ void FCQueue::reset()
>> {
>> clear();
>>
>> + if (size_)
>> + setSize(size_);
>> +
>> isFull_ = false;
>>
>> latestFC_ = 0;
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 52a0b5e7..0da2ad26 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -21,9 +21,6 @@ namespace libcamera {
>>
>> namespace ipa::ipu3 {
>>
>> -/* Maximum number of frame contexts to be held */
>> -static constexpr uint32_t kMaxFrameContexts = 16;
>> -
>> struct IPASessionConfiguration {
>> struct {
>> ipu3_uapi_grid_config bdsGrid;
>> @@ -100,10 +97,12 @@ public:
>>
>> IPAFrameContext *get(uint32_t frame);
>> bool isFull() { return isFull_; }
>> + uint32_t size() { return size_; }
>>
>> private:
>> uint32_t latestFC_;
>> uint32_t oldestFC_;
>> + uint32_t size_;
>>
>> bool isFull_;
>> };
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 15070fa0..3218df49 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -458,8 +458,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>
>> /* Clean IPAActiveState at each reconfiguration. */
>> context_.activeState = {};
>> +
>> context_.frameContexts.reset();
>> - context_.frameContexts.setSize(kMaxFrameContexts);
>> + context_.frameContexts.setSize(configInfo.maxQueueRequests);
>>
>> if (!validateSensorControls()) {
>> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index b7dda282..d599fb95 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -672,6 +672,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>> configInfo.bdsOutputSize = config->imguConfig().bds;
>> configInfo.iif = config->imguConfig().iif;
>>
>> + /*
>> + * \todo Find a better position where maxQueueRequests can be
>> + * defined and fetched. Maybe in IPA Interface? Camera property?
>> + * Not sure.
>> + */
>> + uint32_t maxQueueRequests = 16;
>> + configInfo.maxQueueRequests = maxQueueRequests;
>> + setMaxQueueRequests(maxQueueRequests);
>> +
>> ret = data->ipa_->configure(configInfo, &data->ipaControls_);
>> if (ret) {
>> LOG(IPU3, Error) << "Failed to configure IPA: "
>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> index 7ebd76ad..08ec28de 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>> @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(Pipeline)
>> * respective factories.
>> */
>> PipelineHandler::PipelineHandler(CameraManager *manager)
>> - : manager_(manager), lockOwner_(false)
>> + : manager_(manager), maxQueueRequests_(0), requestsQueueCounter_(0),
>> + lockOwner_(false)
>> {
>> }
>>
>> @@ -312,6 +313,9 @@ void PipelineHandler::stop(Camera *camera)
>> /* Make sure no requests are pending. */
>> Camera::Private *data = camera->_d();
>> ASSERT(data->queuedRequests_.empty());
>> +
>> + maxQueueRequests_ = 0;
>> + requestsQueueCounter_ = 0;
>> }
>>
>> /**
>> @@ -397,6 +401,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_) {
>> @@ -424,6 +431,10 @@ void PipelineHandler::doQueueRequests()
>> if (!request->_d()->prepared_)
>> break;
>>
>> + if (maxQueueRequests_ &&
>> + requestsQueueCounter_ >= maxQueueRequests_)
>> + break;
>> +
>> doQueueRequest(request);
>> waitingRequests_.pop();
>> }
>> @@ -500,6 +511,9 @@ void PipelineHandler::completeRequest(Request *request)
>> ASSERT(!req->hasPendingBuffers());
>> data->queuedRequests_.pop_front();
>> camera->requestComplete(req);
>> +
>> + if (maxQueueRequests_)
>> + requestsQueueCounter_--;
>> }
>> }
>>
>> @@ -616,6 +630,51 @@ 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.
>> + */
>> +
>> +/**
>> + * \fn PipelineHandler::maxQueueRequests()
>> + * \brief Retrieve the maximum number of in-flight requests handled by the
>> + * underlying hardware
>> + * \return Number of requests that can be queued to underlying hardware
>> + */
>> +
>> +/**
>> + * \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.31.1
>>
More information about the libcamera-devel
mailing list