[libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame Context queue to libipa
Jacopo Mondi
jacopo at jmondi.org
Mon Jul 25 12:00:52 CEST 2022
One additional note
On Mon, Jul 25, 2022 at 10:29:17AM +0200, Jacopo Mondi via libcamera-devel wrote:
> 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 ?
This happens later :)
>
> 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 ?
>
Please also note that is a non-problem as to my surprise the templates
argument substitution compile-time checks if the provided type matches
the requested interface. The C++ templating system does duck typing
and if the provided FrameContext template argument does not inherit
from IPAFrameContext it breaks with:
../src/ipa/ipu3/ipu3.cpp:577:26: error: ‘struct libcamera::ipa::ipu3::IPU3FrameContext’ has no member named ‘frame’
577 | if (frameContext.frame != frame)
| ^~~~~
However we can exploit std::type_traits to more explicitly check for the
FrameContext template argument to be a derived class of
IPAFrameContext with:
template<typename FrameContext>
class FCQueue : public std::array<FrameContext, kMaxFrameContexts>
{
private:
using valueType =
std::enable_if_t<std::is_base_of_v<IPAFrameContext, FrameContext>,
FrameContext>;
However the error message, in case FrameContext does not inherit from
IPAFrameContext, is not much more helpful I'm afraid
/usr/include/c++/12.1.0/type_traits: In substitution of ‘template<bool _Cond, class _Tp> using enable_if_t = typename std::enable_if::type [with bool _Cond = false; _Tp = libcamera::ipa::ipu3::IPU3FrameContext]’:
../src/ipa/libipa/fc_queue.h:41:8: required from ‘class libcamera::ipa::FCQueue<libcamera::ipa::ipu3::IPU3FrameContext>’
../src/ipa/ipu3/ipa_context.h:88:28: required from here
/usr/include/c++/12.1.0/type_traits:2614:11: error: no type named ‘type’ in ‘struct std::enable_if<false, libcamera::ipa::ipu3::IPU3FrameContext>’
2614 | using enable_if_t = typename enable_if<_Cond, _Tp>::type;
But could help pointing to the source file where a comment might
explain the required inheritance.
Anyway, as it breaks at compile time, this is a minor nit indeed
> > +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