[libcamera-devel] [RFC PATCH 04/12] ipa: ipu3: Move the Frame Context queue to libipa
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jul 26 12:48:52 CEST 2022
Quoting Kieran Bingham (2022-07-26 11:00:03)
> Quoting Jacopo Mondi (2022-07-25 16:20:51)
> > Hi Kieran
> >
> > 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;
> >
> > The size of the FCQueue should probably come from the IPA
> > implementation, as different platforms will have a different queue
> > depth to handle.
> >
> > It can be made a second template argument.
>
> Yes, this is absoultely a platform specific value.
>
> I simply haven't made it adjustable yet.
>
> I believe it could also be handled on top, as '16' is bigger than both
> expected platforms currently ;-)
>
> I'd rather handle that as a separate patch as well - as whatever size it
> is contstructed as needs to be conveyed to the pipeline handler too.
>
> Either the PH should state what size the IPA should make, or otherwise
> they should both be hardcoded to the same size.
>
> But that's enough discussion that I think it deserves it's own
> patch/implementation.
>
>
> > > -
> > > 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;
> >
> > I'm sure this will be expanded later but it could be simplified as
> >
> > void IPAIPU3::queueRequest(const uint32_t frame, [[maybe_unused]] const ControlList &controls)
> > {
> > /* \todo Start processing for 'frame' based on 'controls'. */
> > context_.frameContexts.initialise(frame);
>
> Yes, but I don't want to make add the maybe_unused's just to remove
> them.
>
> And The frame context will be used by the last patch in this series.
>
> If you insist, we can simplify here, but it's just churn eitherway, on
> the way to get to the last patch.
>
>
> > }
> >
> > > }
> > >
> > > /**
> > > 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)
> >
> > Is this needed here or can be moved to the .h ?
>
> This is the definition, that has to be in a .cpp doesn't it ?
>
> It's also 'private' to the fc_queue. I wouldn't expect other components
> to use this log category.
>
> >
> > > +
> > > +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
> >
> > frame context or FrameContext, here and in the other parts of the
> > documentation ?
>
> I prefer capitalizing the component names, so that it's clear that we're
> talking about a 'libcamera noun'.
>
> In the instance above, I could remove the space..
> (s/Frame Context/FrameContext)
>
> >
> > > + * \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.
> > "Frame sequence number" ?
>
> Ack.
>
>
> > > + *
> > > + * 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
> >
> > IPA specific frame context implementations shall inherit from the
> > IPAFrameContext base class 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
> >
> > That's what a circular array does.
> >
> > I would rather express the fact that the number of available contexts
> > is required to be larger than the platform pipeline depth (with a
> > \todo to reference the property once we have it defined). After
> > pipelineDepth frames have been processed the FrameContext is recycled
> > and forever lost.
>
> The important part to me here is to describe the behaviour of overflow
> indeed.
>
> > > + * 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.
> >
> > This would happen because we have a request underrun, right ?
> >
> > I would state that. Like:
> >
> > 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 now way the controls associated with the late Request could be
> > applied in time, as the frame as already been processed by the IPA,
> > the PFCError flag is automatically raised on the FrameContext.
> >
> > How does it sound ?
>
> I'll try to respin it all into an updated v2.
>
>
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn FCQueue::clear()
> > > + * \brief Reinitialise all data on the queue
> >
> > Or "Clear the queue content" ?
>
> Sure.
>
> >
> > > + *
> > > + * Reset the queue and ensure all FrameContext slots are re-initialised.
> >
> > I would add that the Queue has to be re-initialized at every streaming
> > session start, likely in IPA::configure()
>
> Yes, but actually IPA::start() is the real key point to ensure the queue
> is clear. configure() should be irrelevant in fact.
>
> Except I fear start() may race with queueReqeust(), so I'll have to
> verify this. In which case, configure() and stop() would be an
> alternative.
>
>
> > > + */
> > > +
> > > +/**
> > > + * \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.
> > > + *
> >
> > Should we say it is expected to be called at queueRequest() time ?
>
> Yes, probably.
>
> >
> > > + * \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
> > > + *
> >
> > Unecessary empty line
> >
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <array>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/controls.h>
> >
> > Not needed it seems
> >
> > > +
> > > +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>
> > > +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) {
> >
> > This should be <= as it needs to catch cases where the frame context
> > has been possibile overwritten by algorithms calling get() for a
> > number of times larger than the pipeline depth.
>
> Yes!
>
> I was working on the theory that the PFCError flag would already be
> raised by the get() call, but if we overflow even the internal storage
> here, I wonder if we should also explicitly set the PFCError here again
> as well.
>
>
>
> >
> > > + LOG(FCQueue, Warning)
> > > + << "Frame " << frame << " already initialised";
> > > + } else {
> > > + initialise(frameContext, frame);
> > > + }
> >
> > No brances needed it seems
> >
> > > +
> > > + return frameContext;
> > > + }
> > > +
> > > + FrameContext &get(uint32_t frame)
> > > + {
> > > + FrameContext &frameContext = this->at(frame & kMaxFrameContexts);
> > > +
> > > + if (frame != frameContext.frame) {
> >
> > Or if (frame == frameContext.frame)
> > return frameContext;
> >
> > To save one indentation level. Up to you.
>
> Oh yes, definitely ;-)
>
>
An assertion here might be useful to ensure that get() isn't trying to
initialise over a 'newer' frame.
ASSERT(frameContext.frame < frame);
If frameContext.frame > frame - then it means something is trying to
access the FCQ while the incoming request queue is overflowing the queue
which I believe we will, but have not yet prevented from happening on
the Pipeline Handler side.
>
> >
> > > + 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