[libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame Context queue to libipa
Jacopo Mondi
jacopo at jmondi.org
Mon Jul 25 10:29:17 CEST 2022
Hi Kieran,
one comment on the class implementation before doing a full review
On Thu, Jul 21, 2022 at 01:13:02PM +0100, Kieran Bingham via libcamera-devel wrote:
> From: Umang Jain <umang.jain at ideasonboard.com>
>
> Provide a direct FCQueue abstraction to facilitate creating a queue of
> Frame Context structures.
>
> The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue
> added to the IPU3, but the Algorithms are not yet moved from the single
> FrameContext exposed by the IPA context to use the correct one from the
> queue until the Algorithm interfaces are fully updated.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/ipa/ipu3/ipa_context.cpp | 17 -------
> src/ipa/ipu3/ipa_context.h | 15 ++----
> src/ipa/ipu3/ipu3.cpp | 13 +++--
> src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++
> src/ipa/libipa/fc_queue.h | 93 ++++++++++++++++++++++++++++++++++
> src/ipa/libipa/meson.build | 2 +
> 6 files changed, 204 insertions(+), 32 deletions(-)
> create mode 100644 src/ipa/libipa/fc_queue.cpp
> create mode 100644 src/ipa/libipa/fc_queue.h
>
> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> index 13cdb835ef7f..dd139cb4c868 100644
> --- a/src/ipa/ipu3/ipa_context.cpp
> +++ b/src/ipa/ipu3/ipa_context.cpp
> @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 {
> * <linux/intel-ipu3.h> struct ipu3_uapi_gamma_corr_lut for further details.
> */
>
> -/**
> - * \brief Default constructor for IPAFrameContext
> - */
> -IPAFrameContext::IPAFrameContext() = default;
> -
> -/**
> - * \brief Construct a IPAFrameContext instance
> - */
> -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> - : frame(id), frameControls(reqControls)
> -{
> - sensor = {};
> -}
> -
> /**
> * \var IPAFrameContext::frame
> * \brief The frame number
> *
> - * \var IPAFrameContext::frameControls
> - * \brief Controls sent in by the application while queuing the request
> - *
> * \var IPAFrameContext::sensor
> * \brief Effective sensor values that were applied for the frame
> *
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 42e11141d3a1..193fc44a821a 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -8,8 +8,6 @@
>
> #pragma once
>
> -#include <array>
> -
> #include <linux/intel-ipu3.h>
>
> #include <libcamera/base/utils.h>
> @@ -17,13 +15,12 @@
> #include <libcamera/controls.h>
> #include <libcamera/geometry.h>
>
> +#include <libipa/fc_queue.h>
> +
> 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;
> @@ -77,23 +74,19 @@ struct IPAActiveState {
> };
>
> struct IPAFrameContext {
> - IPAFrameContext();
> - IPAFrameContext(uint32_t id, const ControlList &reqControls);
> + uint32_t frame;
>
> struct {
> uint32_t exposure;
> double gain;
> } sensor;
> -
> - uint32_t frame;
> - ControlList frameControls;
> };
>
> struct IPAContext {
> IPASessionConfiguration configuration;
> IPAActiveState activeState;
>
> - std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
> + FCQueue<IPAFrameContext> frameContexts;
> };
>
> } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2f6bb672f7bb..55e82cd404f4 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -38,6 +38,8 @@
> #include "algorithms/tone_mapping.h"
> #include "libipa/camera_sensor_helper.h"
>
> +#include "ipa_context.h"
> +
> /* Minimum grid width, expressed as a number of cells */
> static constexpr uint32_t kMinGridWidth = 16;
> /* Maximum grid width, expressed as a number of cells */
> @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>
> /* Clean IPAActiveState at each reconfiguration. */
> context_.activeState = {};
> - IPAFrameContext initFrameContext;
> - context_.frameContexts.fill(initFrameContext);
> + context_.frameContexts.clear();
>
> if (!validateSensorControls()) {
> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> const ipu3_uapi_stats_3a *stats =
> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>
> - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> + IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>
> if (frameContext.frame != frame)
> LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> {
> /* \todo Start processing for 'frame' based on 'controls'. */
> - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> + IPAFrameContext &frameContext = context_.frameContexts.initialise(frame);
> +
> + /* \todo Implement queueRequest to each algorithm. */
> + (void)frameContext;
> + (void)controls;
> }
>
> /**
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> new file mode 100644
> index 000000000000..ecb47f287350
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -0,0 +1,96 @@
> +/* 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
> + */
> +
> +/**
> + * \class FCQueue
> + * \brief A support class for queueing Frame Context instances through the IPA
> + * \tparam FrameContext The IPA specific Frame Context type for this queue
> + *
> + * The Frame Context Queue provides a simple circular buffer implementation to
> + * store IPA specific context for each frame through its lifetime within the
> + * IPA.
> + *
> + * FrameContexts are expected to be referenced by a monotonically increasing
> + * sequence count referring to a Frame sequence.
> + *
> + * 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
> + * 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 FrameContexts should inherit from the IPAFrameContext to support
> + * the minimum required features for a FrameContext, including the frame number
> + * and error flags that relate to the frame.
> + *
> + * FrameContexts are overwritten when they are recycled and re-initialised by
> + * the first access made on them by either initialise(frame) or get(frame). If a
> + * FrameContext is first accessed through get(frame) before calling initialise()
> + * a PFCError is automatically raised on the FrameContext to be transferred to
> + * the Request when it completes.
> + */
> +
> +/**
> + * \fn FCQueue::clear()
> + * \brief Reinitialise all data on the queue
> + *
> + * Reset the queue and ensure all FrameContext slots are re-initialised.
> + */
> +
> +/**
> + * \fn FCQueue::initialise(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.
> + *
> + * If the FrameContext was already initialized for this sequence, a warning will
> + * be reported and the previously initialized FrameContext is returned.
> + *
> + * \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 intiialised for the \a frame, it will be
> + * initialised and a PFCError will be flagged on the context to be transferred
> + * to the Request when it completes.
> + *
> + * \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..82ce36811926
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * fc_queue.h - IPA Frame context queue
> + *
> + */
> +
> +#pragma once
> +
> +#include <array>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(FCQueue)
> +
> +namespace ipa {
> +
> +/*
> + * Maximum number of frame contexts to be held onto
> + *
> + * \todo Should be larger than ISP delay + sensor delay
> + */
> +static constexpr uint32_t kMaxFrameContexts = 16;
> +
> +template<typename FrameContext>
I presume the idea of using a template is to allow IPAs to define
their own frame context and use this class as a container.
However the class assumes that the FrameContext template argument has
a well defined interface, specifically it requires a 'uint32_t frame'
field.
There is no guarantee at all that the class used to instantiate the
template has such field with the here proposed design, am I wrong ?
Shouldn't we create a base class in libipa such as:
class IPAFrameContext
{
public:
uint32_t frame = 0;
};
And let IPA implementation subclass it ?
Can we force it to be a pure virtual class by making the constructor
virtual ? There might be some more clever ways to make this a
non-instantiable interface...
How to force the FCQueue to nicely accept a class derived from
IPAFrameContext might also require some thinking, but I presume we
might want enforce that ?
> +class FCQueue : public std::array<FrameContext, kMaxFrameContexts>
> +{
> +private:
> + void initialise(FrameContext &frameContext, const uint32_t frame)
> + {
> + frameContext = {};
> + frameContext.frame = frame;
> + }
> +
> +public:
> + void clear()
> + {
> + this->fill({});
> + }
> +
> + FrameContext &initialise(const uint32_t frame)
> + {
> + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);
> +
> + /*
> + * Do not re-initialise if a get() call has already fetched this
> + * frame context to preseve the error flags already raised.
> + */
> + if (frame == frameContext.frame && frame != 0) {
> + LOG(FCQueue, Warning)
> + << "Frame " << frame << " already initialised";
> + } else {
> + initialise(frameContext, frame);
> + }
> +
> + return frameContext;
> + }
> +
> + FrameContext &get(uint32_t frame)
> + {
> + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);
> +
> + if (frame != frameContext.frame) {
> + LOG(FCQueue, Warning)
> + << "Obtained an uninitialised FrameContext for "
> + << frame;
> +
> + initialise(frameContext, frame);
> +
> + /*
> + * 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.
> + */
> + frameContext.error |= Request::PFCError;
> + }
> +
> + return frameContext;
> + }
> +};
> +
> +} /* 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',
> ])
> --
> 2.34.1
>
More information about the libcamera-devel
mailing list