[libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce FrameContextQueue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 22 21:22:16 CEST 2022
Hi Jacopo,
On Wed, Sep 21, 2022 at 07:49:12PM +0200, Jacopo Mondi wrote:
> On Thu, Sep 08, 2022 at 04:41:32AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Umang Jain <umang.jain at ideasonboard.com>
> >
> > Introduce a common implementation in libipa to represent the queue of
> > frame contexts.
> >
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > Changes since v3:
> >
> > - Split the IPU3 changes to a separate patch
> > - Use composition instead of inheritance
>
>
>
> > - Use vector instead of array for backend storage
>
> I guess this is because now the FCQ size has dynamic size ?
That's right.
> > - Make the queue size dynamic
> > - Rename initialise() to init()
> > ---
> > src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
> > src/ipa/libipa/fc_queue.h | 108 +++++++++++++++++++++++++++++++++
> > src/ipa/libipa/meson.build | 2 +
> > 3 files changed, 227 insertions(+)
> > create mode 100644 src/ipa/libipa/fc_queue.cpp
> > create mode 100644 src/ipa/libipa/fc_queue.h
> >
> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > new file mode 100644
> > index 000000000000..b81d497e255a
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.cpp
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fc_queue.cpp - IPA Frame context queue
> > + */
> > +
> > +#include "fc_queue.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \file fc_queue.h
> > + * \brief Queue to access per-frame Context
>
> contexts maybe, or does Context name a type
Maybe Context was a type at some point, or maybe I considered turning it
into a type, but it's not now, so I'll use "contexts". I'll also replace
"Frame Context" with "FrameContext" below.
> > + */
> > +
> > +/**
> > + * \class FCQueue
> > + * \brief A support class for queueing FrameContext instances through the IPA
> > + * \tparam FrameContext The IPA specific FrameContext derived class type
> > + *
> > + * The frame context queue provides a simple circular buffer implementation to
> > + * store IPA specific context for each frame through its lifetime within the
> > + * IPA.
> > + *
> > + * FrameContext instances are expected to be referenced by a monotonically
> > + * increasing sequence count corresponding to a frame sequence number.
> > + *
> > + * A FrameContext is initialised for a given frame when the corresponding
> > + * Request is first queued into the IPA. From that point on the FrameContext can
> > + * be obtained by the IPA and its algorithms by referencing it from the frame
> > + * sequence number.
> > + *
> > + * A frame sequence number from the image source must correspond to the request
>
> "A frame sequence number" or "The frame sequence number" ?
Ack.
> > + * sequence number for this implementation to be supported in an IPA. It is
> > + * expected that the same sequence number will be used to reference the context
> > + * of the frame from the point of queueing the request, specifying controls for
> > + * a given frame, and processing of any ISP related controls and statistics for
> > + * the same corresponding image.
> > + *
> > + * IPA specific frame context implementations shall inherit from the
> > + * IPAFrameContext base class to support the minimum required features for a
> > + * FrameContext.
> > + *
> > + * FrameContexts are overwritten when they are recycled and re-initialised by
> > + * the first access made on them by either init(frame) or get(frame). It is
> > + * required that the number of available slots in the frame context queue is
> > + * larger or equal to the maximum number of in-flight requests a pipeline can
> > + * handle to avoid overwriting frame context instances that still have to be
> > + * processed.
> > + *
> > + * In the case an application does not queue requests to the camera fast
> > + * enough, frames can be produced and processed by the IPA without a
> > + * corresponding Request being queued. In this case the IPA algorithm
> > + * will try to access the FrameContext with a call to get() before it
> > + * had been initialized at queueRequest() time. In this case, there
> > + * is no way the controls associated with the late Request could be
> > + * applied in time, as the frame as already been processed by the IPA.
>
> s/as already/has already/
>
> > + *
> > + * \todo Set an error flag for per-frame control errors.
> > + */
> > +
> > +/**
> > + * \fn FCQueue::FCQueue(unsigned int size)
> > + * \brief Construct a frame context queue of a specified size
> > + * \param[in] size The number of contexts in the queue
> > + */
> > +
> > +/**
> > + * \fn FCQueue::clear()
> > + * \brief Clear the context queue
> > + *
> > + * Reset the queue and ensure all FrameContext slots are re-initialised.
> > + * IPA modules are expected to clear the frame context queue at the beginning of
> > + * a new streaming session, in IPAModule::start().
>
> I recall we had a discussion and if I'm not mistaken clearing the FCQ at
> start() time would create issues with Requests queued to the camera
> before Camera::start() was called.
>
> iirc we settled on stop() as the "right" place to clear the FCQ
> between streaming sessions (assuming it gets initialized in a clean
> state ofc).
>
> However the doc here matches the v3 version.
That's a good point. I'll see if I can fix it.
> > + */
> > +
> > +/**
> > + * \fn FCQueue::init(uint32_t frame)
> > + * \brief Initialize and return the Frame Context at slot specified by \a frame
> > + * \param[in] frame The frame context sequence number
> > + *
> > + * The first call to obtain a FrameContext from the FCQueue should be handled
> > + * through this call. The FrameContext will be initialised for the frame and
> > + * returned to the caller if it was not already initialised.
>
> This sounds like "it is returned to the caller if it was not already
> initialised."
>
> What about "The FrameContext will be initialised, if not initialised
> already, and returned to the caller."
Ack.
> > + *
> > + * If the FrameContext was already initialized for this sequence, a warning will
> > + * be reported and the previously initialized FrameContext is returned.
> > + *
> > + * Frame contexts are expected to be initialised when a Request is first passed
> > + * to the IPA module in IPAModule::queueRequest().
> > + *
> > + * \return A reference to the FrameContext for sequence \a frame
> > + */
> > +
> > +/**
> > + * \fn FCQueue::get()
> > + * \brief Obtain the Frame Context at slot specified by \a frame
> > + * \param[in] frame The frame context sequence number
> > + *
> > + * Obtains an existing FrameContext from the queue and returns it to the caller.
> > + *
> > + * If the FrameContext is not correctly initialised for the \a frame, it will be
> > + * initialised.
> > + *
> > + * \return A reference to the FrameContext for sequence \a frame
> > + */
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > new file mode 100644
> > index 000000000000..0f3af0f3a189
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fc_queue.h - IPA Frame context queue
> > + */
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +template<typename FrameContext>
> > +class FCQueue
> > +{
> > +public:
> > + FCQueue(unsigned int size)
> > + : contexts_(size)
>
> I'm not sure I get why the FCQ size should be dynamic. If I recall
> correctly Umang had this on v2 ? The size of the FCQ is a camera
> property which corresponds to the max number of requests in flight
> a camera can handle and its fixed at compile time ?
>
> Using a libcamera::property also guarantee that the two sizes matches.
> Is it too demanding for IPA implementations ?
Good point. I wanted to allow different IPA modules to use different
context queue sizes, and didn't consider making this a template
parameter. I'll check what it entails.
> > + {
> > + }
> > +
> > + void clear()
> > + {
> > + std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> > + }
> > +
> > + FrameContext &init(const uint32_t frame)
> > + {
> > + FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > +
> > + /*
> > + * Do not re-initialise if a get() call has already fetched this
> > + * frame context to preseve the context.
> > + *
> > + * \todo If the the sequence number of the context to initialise
> > + * is smaller than the sequence number of the queue slot to use,
> > + * it means that we had a serious request underrun and more
> > + * frames than the queue size has been produced since the last
> > + * time the application has queued a request. Does this deserve
> > + * an error condition ?
> > + */
> > + if (frame != 0 && frame <= frameContext.frame)
> > + LOG(FCQueue, Warning)
> > + << "Frame " << frame << " already initialised";
> > + else
> > + init(frameContext, frame);
> > +
> > + return frameContext;
> > + }
> > +
> > + FrameContext &get(uint32_t frame)
> > + {
> > + FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > +
> > + /*
> > + * If the IPA algorithms try to access a frame context slot which
> > + * has been already overwritten by a newer context, it means the
> > + * frame context queue has overflowed and the desired context
> > + * has been forever lost. The pipeline handler shall avoid
> > + * queueing more requests to the IPA than the frame context
> > + * queue size.
> > + */
> > + if (frame < frameContext.frame)
> > + LOG(FCQueue, Fatal) << "Frame Context for " << frame
> > + << " has been overwritten by "
> > + << frameContext.frame;
> > +
> > + if (frame == frameContext.frame)
> > + return frameContext;
> > +
> > + /*
> > + * The frame context has been retrieved before it was
> > + * initialised through the initialise() call. This indicates an
> > + * algorithm attempted to access a Frame context before it was
> > + * queued to the IPA. Controls applied for this request may be
> > + * left unhandled.
> > + *
> > + * \todo Set an error flag for per-frame control errors.
> > + */
> > + LOG(FCQueue, Warning)
> > + << "Obtained an uninitialised FrameContext for " << frame;
> > +
> > + init(frameContext, frame);
> > +
> > + return frameContext;
> > + }
> > +
> > +private:
> > + void init(FrameContext &frameContext, const uint32_t frame)
> > + {
> > + frameContext = {};
> > + frameContext.frame = frame;
> > + }
> > +
> > + std::vector<FrameContext> contexts_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index fb894bc614af..016b8e0ec9be 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -3,6 +3,7 @@
> > libipa_headers = files([
> > 'algorithm.h',
> > 'camera_sensor_helper.h',
> > + 'fc_queue.h',
> > 'histogram.h',
> > 'module.h',
> > ])
> > @@ -10,6 +11,7 @@ libipa_headers = files([
> > libipa_sources = files([
> > 'algorithm.cpp',
> > 'camera_sensor_helper.cpp',
> > + 'fc_queue.cpp',
> > 'histogram.cpp',
> > 'module.cpp',
> > ])
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list