[libcamera-devel] [PATCH v4 04/32] ipa: libipa: Introduce FrameContextQueue
Jacopo Mondi
jacopo at jmondi.org
Wed Sep 21 19:49:12 CEST 2022
Hi Laurent
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 ?
> - 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
> + */
> +
> +/**
> + * \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" ?
> + * 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.
> + */
> +
> +/**
> + * \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."
> + *
> + * 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 ?
> + {
> + }
> +
> + 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