[libcamera-devel] [PATCH v2 04/10] ipa: libipa: Introduce FrameContextQueue
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 24 00:52:05 CEST 2022
Hi Kieran,
On Tue, Aug 23, 2022 at 12:55:24PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi (2022-08-18 08:44:55)
> > Hi Kieran
> >
> > thanks a lot for getting to the bottom of this.
> >
> > I still can't reproduce it here, but your analysis seems correct to me
> > and indeed the context = {} might prove problematic.
> >
> > See below
> >
> > On Wed, Aug 17, 2022 at 09:33:08PM +0100, Kieran Bingham wrote:
> > > Quoting Umang Jain via libcamera-devel (2022-08-11 10:13:42)
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the clarifications,
> > > >
> > > > On 8/11/22 14:08, Jacopo Mondi wrote:
> > > > > Hi Umang
> > > > >
> > > > > On Thu, Aug 11, 2022 at 01:48:50PM +0530, Umang Jain wrote:
> > > > >> Hi all,
> > > > >>
> > > > >> Few comments and some open question for discussion on underruns...
> > > > >>
> > > > >> On 8/5/22 19:23, Jacopo Mondi wrote:
> > > > >>> From: Umang Jain <umang.jain at ideasonboard.com>
> > > > >>>
> > > > >>> Introduce a common implementation in libipa to represent the queue of
> > > > >>> frame contexts.
> > > > >>>
> > > > >>> Adjust the IPU3 IPAFrameContext to provide the basic class members
> > > > >>> required to work with the generic FCQueue implementation, before
> > > > >>> introducing an IPAFrameContext class in the next patches.
> > > > >>>
> > > > >>> Opportunely handle cleaning up the queue and the IPA context at
> > > > >>> IPA::stop() time.
> > >
> > > This is bad. It breaks and causes the IPA to crash...
> > >
> > > The CTS test
> > > android.hardware.cts.CameraTest#testJpegCallbackStartPreview triggers
> > > this first...
> > >
> > > This may be the first test that has a configure ... start ... stop ...
> > > start sequence without a reconfigure. Which I think is where we break.
> > >
> > > Essentially - we can't just clear the entire context_ now. Certainly not
> > > between stop/start cycles.
> > >
> > > > >>> 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>
> > > > >>> ---
> > > > >>> src/ipa/ipu3/ipa_context.cpp | 20 +------
> > > > >>> src/ipa/ipu3/ipa_context.h | 16 ++---
> > > > >>> src/ipa/ipu3/ipu3.cpp | 18 +++---
> > > > >>> src/ipa/libipa/fc_queue.cpp | 112 +++++++++++++++++++++++++++++++++++
> > > > >>> src/ipa/libipa/fc_queue.h | 109 ++++++++++++++++++++++++++++++++++
> > > > >>> src/ipa/libipa/meson.build | 2 +
> > > > >>> src/ipa/rkisp1/rkisp1.cpp | 11 ++--
> > > > >>> 7 files changed, 250 insertions(+), 38 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..9605c8a1106d 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
> > > > >>> *
> > > > >>> @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > > > >>> *
> > > > >>> * \var IPAFrameContext::sensor.gain
> > > > >>> * \brief Analogue gain multiplier
> > > > >>> + *
> > > > >>> + * \var IPAFrameContext::error
> > > > >>> + * \brief The error flags for this frame context
> > > > >>> */
> > > > >>> } /* namespace libcamera::ipa::ipu3 */
> > > > >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > > >>> index 42e11141d3a1..dc5b7c30aaf9 100644
> > > > >>> --- a/src/ipa/ipu3/ipa_context.h
> > > > >>> +++ b/src/ipa/ipu3/ipa_context.h
> > > > >>> @@ -8,22 +8,20 @@
> > > > >>> #pragma once
> > > > >>> -#include <array>
> > > > >>> -
> > > > >>> #include <linux/intel-ipu3.h>
> > > > >>> #include <libcamera/base/utils.h>
> > > > >>> #include <libcamera/controls.h>
> > > > >>> #include <libcamera/geometry.h>
> > > > >>> +#include <libcamera/request.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 +75,21 @@ 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;
> > > > >>> + Request::Errors error;
> > > > >>> };
> > > > >>> 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..209a6b040f8f 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 */
> > > > >>> @@ -348,6 +350,9 @@ int IPAIPU3::start()
> > > > >>> */
> > > > >>> void IPAIPU3::stop()
> > > > >>> {
> > > > >>> + /* Clean the IPA context at the end of the streaming session. */
> > > > >>> + context_.frameContexts.clear();
> > > > >>> + context_ = {};
> > >
> > > I think this context_ = {}; is the line causing me trouble right now.
> > >
> > > The IPU3 IPA crashes with a divide by zero error in :
> > >
> > > libcamera::ipa::ipu3::algorithms::Af::afEstimateVariance()
> > >
> > > where y_items.size() = 0, because Af::process() has set afRawBufferLen =
> > > 0 from
> > >
> > > /* Evaluate the AF buffer length */
> > > uint32_t afRawBufferLen = context.configuration.af.afGrid.width *
> > > context.configuration.af.afGrid.height;
> > >
> > > So because context has been completely zeroed, and not reconfigured - we
> > > end up with a zero length span, and a div/0.
> >
> > Ouch
> >
> > Still doesn't make sense to me that here it doesn't fail..
> >
> > >
> > > I expect simply removing this context_ = {}; may be sufficient. This may
> > > need to be considered for the RKISP too.
> > >
> >
> > Ok, looking at the context in more detail, it indeed contains the
> > session configuration, which shall not be zeroed, but just overwritten
> > when a new configure() is issued.
> >
> > context_ contains the active state as well, I'm still trying to figure
> > out if that one needs to be cleaned in order not to operate on
> > stale data when a new streaming session is started.
> >
> > The frame context queue does indeed need to be cleared between
> > streaming session instead.
> >
> > Overall we have a context_ which contains 3 items with different
> > lifetime management requirements.
> >
> > I wonder if it's a good idea to store all of them together or it would
> > be easier to break the FCQ and the active state out from the
> > context_. I actually wonder if a struct context {} makes any sense,
> > when the IPA class can have the three items (configuration, queue and
> > state) as three class members and handle their lifetime separately.
> >
> > To be honest, the role of active state is still a little bit unclear
> > to me. It contains a global state of algorithm results without any
> > synchronization with the current frame. Re-reading Laurent's comment
> > on 05/10 of this series and your reply, it seems we agree the active
> > state shall be removed or reworked.
>
> Potentially removed, but I think there's still scope for it. Things like
> the current modes of the algorithms might need to be there, it depends
> on if algorithms need to look at the modes of other algorithms.
They important word here is "current". If we want to keep an "active
state", I'll want a formal definition of "current".
> I know for instance, Kate would like to disable AWB during AF scanning.
> [0]
>
> I would expect this to happen through the active state, rather than the
> frame context.
>
> https://patchwork.libcamera.org/project/libcamera/list/?series=3096
>
> Overall I don't expect the ActiveState to be large, as algorithms could
> probably put that information in their own private storage. But maybe if
> it's stored as part of the overall context it provides a convenience.
>
> In otherwords, I think it needs to be considered 'case by case' as we go
> through and move data into the FrameContext (and thus the fcq).
>
> > For now I think it's then fine to remove the context = {}; from this
> > series, and then rework how the IPA context is modeled on top. Do you
> > all agree with this ?
>
> Yes, I think as long as we don't clear the entire context structure it's
> fine.
>
> > > Yes, I've removed this line, and the test doesn't fail now. I'll rerun
> > > the whole CTS suite.
> > >
> > > Same for RKISP below...
> > >
> > > > >>> }
> > > > >>> /**
> > > > >>> @@ -454,11 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> > > > >>> calculateBdsGrid(configInfo.bdsOutputSize);
> > > > >>> - /* Clean IPAActiveState at each reconfiguration. */
> > > > >>> - context_.activeState = {};
> > > > >>> - IPAFrameContext initFrameContext;
> > > > >>> - context_.frameContexts.fill(initFrameContext);
> > > > >>> -
> > > > >>> if (!validateSensorControls()) {
> > > > >>> LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > > > >>> return -EINVAL;
> > > > >>> @@ -569,7 +569,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 +618,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..37ffca60b992
> > > > >>> --- /dev/null
> > > > >>> +++ b/src/ipa/libipa/fc_queue.cpp
> > > > >>> @@ -0,0 +1,112 @@
> > > > >>> +/* 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 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
> > > > >>> + * 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/Frame/frame
> > > > >>
> > > > >>> + * 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, including the frame number and the 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). 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
> > > > >>
> > > > >> s/Camera/camera/
> > > > >>
> > > > >>> + * 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
> > > > >>
> > > > >> s/case/case,
> > > > >>
> > > > >>> + * is now way the controls associated with the late Request could be
> > > > >>
> > > > >> s/now/no
> > > > >>
> > > > > Ack to all the above ones, thanks
> > > > >
> > > > >>> + * applied in time, as the frame as already been processed by the IPA,
> > > > >>> + * the PFCError flag is automatically raised on the FrameContext.
> > > > >>> + */
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \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().
> > > > >>> + */
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * \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.
> > > > >>> + *
> > > > >>> + * 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 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..023b52420fe7
> > > > >>> --- /dev/null
> > > > >>> +++ b/src/ipa/libipa/fc_queue.h
> > > > >>> @@ -0,0 +1,109 @@
> > > > >>> +/* 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/request.h>
> > > > >>> +
> > > > >>> +namespace libcamera {
> > > > >>> +
> > > > >>> +LOG_DECLARE_CATEGORY(FCQueue)
> > > > >>> +
> > > > >>> +namespace ipa {
> > > > >>> +
> > > > >>> +/*
> > > > >>> + * Maximum number of frame contexts to be held onto
> > > > >>> + *
> > > > >>> + * \todo Should be platform-specific and match the pipeline depth
> > > > >>> + */
> > > > >>> +static constexpr uint32_t kMaxFrameContexts = 16;
> > > > >>> +
> > > > >>> +template<typename FrameContext>
> > > > >>> +class FCQueue : private 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.
> > > > >>> + *
> > > > >>> + * \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 ?
> > > > >>
> > > > >> I believe we should log under-runs conditions. Whether we log it as ERROR or
> > > > >> WARN or DEBUG might be a candidate for discussion. It depends on how we
> > > > >> classify such condition
> > > > >>
> > > > >> Are under-run requests expected to completed with non-fatal errors set? If
> > > > > They indeed expect to be completed :)
> > > >
> > > >
> > > > Ok. This was somehow missing from my mind-map about so many discussions
> > > > and PFC document.
> > > >
> > > > Thanks for clarifying that underruns need to be supported.
> > > >
> > > > >
> > > > >> yes, I believe we set RequestError::Underrun here and return the
> > > > >> initialised frameContext. But then how the Request will be completed ? As
> > > > >> the frame number passed here is quite < the current ongoing capture sequence
> > > > >> on sensor side. So, we might end up initialising the context but it's never
> > > > >> .get() with this frame number, for e.g. while populating metadata in
> > > > >> processStatsBuffer()? So we might require some special handling there to
> > > > >> detect that a frameContext belongs to a under-run request or not.
> > > > >>
> > > > >> I think I need to map out how will a under-run request will get completed
> > > > >> successfully - IFF we are supporting that case.
> > > > >>
> > > > > There are legit question, but to properly reply to them I think we
> > > > > need to finalise the design of the per-frame-control model. In
> > > > > example, for some devices with limited memory (Pi Zero, in example),
> > > > > operating in underrun mode seems to be the default, and we have to
> > > > > handle that case properly.
> > > >
> > > >
> > > > Agreed.
> > > >
> > > > Do those discussions block merging of the series? I see you already have
> > > > a \todo here, so maybe it is meant to be handled on top and we can merge
> > > > this set?
> > > >
> > > > >
> > > > >>> + */
> > > > >>> + if (frame != 0 && frame <= frameContext.frame)
> > > > >>> + 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 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 " << frame << " has been overwritten";
> > > > >>
> > > > >> Maybe the log can be improved:
> > > > >>
> > > > >> LOG(FCQueue, Fatal) << "Frame Context for " << frame << " has been overwritten by "
> > > > >> << frameContext.frame;
> > > > >>
> > > > >>> +
> > > > >>> + if (frame == frameContext.frame)
> > > > >>> + return frameContext;
> > > > >>> +
> > > > >>> + LOG(FCQueue, Warning)
> > > > >>> + << "Obtained an uninitialised FrameContext for " << frame;
> > > > >>
> > > > >> I think currently this Warning is being logged for both cases :
> > > > >>
> > > > >> "if (frame < frameContext.frame)" and "if (frame > frameContext.frame)"
> > > > > The
> > > > > if (frame < frameContext.frame)
> > > > > LOG(FCQueue, Fatal)
> > > > > << "Frame " << frame << " has been overwritten";
> > > > >
> > > > > will abort execution, so we don't get to this warning.
> > > > >
> > > > >> Is this expected ? Or are we counting on the "Fatal" log above, which shall
> > > > >> terminate the entire program?
> > > > >>
> > > > > That's the idea, yes.
> > > >
> > > >
> > > > Got it!
> > > >
> > > > I don't see any other issues with the code here, other than the minors
> > > > pointed out so,
> > > >
> > > > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > > >
> > > > Not sure how would tags work with multiple authors in this case.. but
> > > > anyways, we can figure it out on the way!
> > > >
> > > > >
> > > > > The above Fatal is triggered by a programming error in the PH/IPA as
> > > > > we should never get more requests queued than the FCQ size, so the
> > > > > assertion above should not be triggered if not during development of
> > > > > new platforms.
> > > > >
> > > > >>> +
> > > > >>> + 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',
> > > > >>> ])
> > > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > >>> index 6cf4d1699ce5..af22dbeb3da5 100644
> > > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > >>> @@ -49,7 +49,7 @@ public:
> > > > >>> int init(const IPASettings &settings, unsigned int hwRevision,
> > > > >>> ControlInfoMap *ipaControls) override;
> > > > >>> int start() override;
> > > > >>> - void stop() override {}
> > > > >>> + void stop() override;
> > > > >>> int configure(const IPACameraSensorInfo &info,
> > > > >>> const std::map<uint32_t, IPAStream> &streamConfig,
> > > > >>> @@ -189,6 +189,12 @@ int IPARkISP1::start()
> > > > >>> return 0;
> > > > >>> }
> > > > >>> +void IPARkISP1::stop()
> > > > >>> +{
> > > > >>> + /* Clean the IPA context at the end of the streaming session. */
> > > > >>> + context_ = {};
> > >
> > > Indeed - watch out here. I think this breaks start/stop/start cycles as
> > > suddenly the session configuration is now zeroed.
> > >
> > > > >>> +}
> > > > >>> +
> > > > >>> /**
> > > > >>> * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
> > > > >>> * if the connected sensor does not provide enough information to properly
> > > > >>> @@ -228,9 +234,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > > >>> << "Exposure: " << minExposure << "-" << maxExposure
> > > > >>> << " Gain: " << minGain << "-" << maxGain;
> > > > >>> - /* Clean context at configuration */
> > > > >>> - context_ = {};
> > > > >>> -
> > > > >>> /* Set the hardware revision for the algorithms. */
> > > > >>> context_.configuration.hw.revision = hwRevision_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list