[libcamera-devel] [PATCH 2/2] libcamera: pipeline-handler: Consider max in-flight requests constraint
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 14 10:19:54 CET 2022
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.
> > >
> > > 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.
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...
> > 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.
>
> 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
happen transparently if a pipeline handler register
properties::PipelineDepth ? Am I overthinking this ?
> >
> > 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