[libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame Context queue to libipa
Jacopo Mondi
jacopo at jmondi.org
Thu Aug 4 14:54:37 CEST 2022
Hi Umang,
I'm applying comments on top of Kierans' RFC and I have two
questions here
On Sat, Jul 23, 2022 at 02:21:48AM +0530, Umang Jain via libcamera-devel wrote:
> Hi Kieran,
>
> On 7/21/22 17:43, Kieran Bingham wrote:
> > From: Umang Jain <umang.jain at ideasonboard.com>
> >
> > Provide a direct FCQueue abstraction to facilitate creating a queue of
> > Frame Context structures.
>
>
> Ah we are back again to have FCQueue separately (in class files) :-)
>
> >
> > The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue
>
>
> You can just say that the constructors for IPAFrameContext are dropped.
>
>
> > added to the IPU3, but the Algorithms are not yet moved from the single
>
> nit: s/IPU3/IPAIPU3/
>
> Furthermore, a queue is already present in the form of:
>
> std::array<IPAFrameContext, kMaxFrameContexts> frameContexts;
>
> It is just that the queue is transformed to FCQueue. Since this is a RFC, I
> won't dwell much on the commit message, it can get accurately described in
> future interations.
>
> > 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;
>
>
> Okay, we dropped the ControlList here, because every algo will get the
> ControlList in the new interface
>
> > };
> > 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();
> context_.frameContexts.clear();
>
> Should also be done in IPAIPU3::stop().
>
I understand IPA::configure() is not enough, as we can have star/stop
sequences without a reconfiguration in between.
But why stop() and not start()
> > 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
>
>
> s/queueing/queuing/
>
Both spelling seems to be correct, and we have both in the codebase.
I'll keep what Kieran had initially.
Thanks
j
> > + * \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
>
>
> s/queueing/queuing/
>
> > + * 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.
>
>
> I get the point here, but I feel the documentation on get()/PFCError etc. is
> a bit jerky.
>
> Once we have some context setup for FCQ, goals, error situations etc. maybe
> it will fit into the flow.
>
> > + */
> > +
> > +/**
> > + * \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
> s/intiialised/initialised/
> > + * 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>
>
> Nice, templates wohoo!
>
> > +class FCQueue : public std::array<FrameContext, kMaxFrameContexts>
>
>
> Private inheritance will be required. We don't want to expose std::array
> APIs to get used accidentally.
>
> > +{
> > +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;
> > + }
> > +};
>
>
> For now, it looks more or less as per stated in the design.
>
> Thank you for the work!
>
> > +
> > +} /* 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',
> > ])
More information about the libcamera-devel
mailing list