[libcamera-devel] [PATCH v4 09/32] ipa: ipu3: Use base FrameContext class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 22 21:54:54 CEST 2022
Hi Jacopo,
On Wed, Sep 21, 2022 at 08:10:30PM +0200, Jacopo Mondi wrote:
> On Thu, Sep 08, 2022 at 04:41:37AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > Inherit from the base FrameContext class in the IPU3 IPAFrameContext.
> > This allows dropping the frame member, which is now stored in the base
> > class.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/ipa/ipu3/ipa_context.cpp | 24 ++++--------------------
> > src/ipa/ipu3/ipa_context.h | 7 ++++---
> > src/ipa/ipu3/ipu3.cpp | 5 +----
> > 3 files changed, 9 insertions(+), 27 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> > index 13cdb835ef7f..9cfca0db3a0d 100644
> > --- a/src/ipa/ipu3/ipa_context.cpp
> > +++ b/src/ipa/ipu3/ipa_context.cpp
> > @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 {
> > * most recently computed by the IPA algorithms.
> > */
> >
> > -/**
> > - * \struct IPAFrameContext
> > - * \brief Context for a frame
> > - *
> > - * The frame context stores data specific to a single frame processed by the
> > - * IPA. Each frame processed by the IPA has a context associated with it,
> > - * accessible through the IPAContext structure.
> > - *
> > - * Fields in the frame context should reflect values and controls
> > - * associated with the specific frame as requested by the application, and
> > - * as configured by the hardware. Fields can be read by algorithms to
> > - * determine if they should update any specific action for this frame, and
> > - * finally to update the metadata control lists when the frame is fully
> > - * completed.
> > - */
> > -
> > /**
> > * \struct IPAContext
> > * \brief Global IPA context data shared between all algorithms
> > @@ -188,15 +172,15 @@ IPAFrameContext::IPAFrameContext() = default;
> > /**
> > * \brief Construct a IPAFrameContext instance
> > */
> > -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls)
> > - : frame(id), frameControls(reqControls)
> > +IPAFrameContext::IPAFrameContext(const ControlList &reqControls)
> > + : frameControls(reqControls)
> > {
> > sensor = {};
> > }
> >
> > /**
> > - * \var IPAFrameContext::frame
> > - * \brief The frame number
> > + * \struct IPAFrameContext
> > + * \brief IPU3-specific FrameContext
> > *
> > * \var IPAFrameContext::frameControls
> > * \brief Controls sent in by the application while queuing the request
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index 42e11141d3a1..e8fc42769075 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -17,6 +17,8 @@
> > #include <libcamera/controls.h>
> > #include <libcamera/geometry.h>
> >
> > +#include <libipa/fc_queue.h>
> > +
> > namespace libcamera {
> >
> > namespace ipa::ipu3 {
> > @@ -76,16 +78,15 @@ struct IPAActiveState {
> > } toneMapping;
> > };
> >
> > -struct IPAFrameContext {
> > +struct IPAFrameContext : public FrameContext {
>
> This could now be called IPU3FrameContext :)
There's a subsequent patch in the series that handles renames :-)
> > IPAFrameContext();
> > - IPAFrameContext(uint32_t id, const ControlList &reqControls);
> > + IPAFrameContext(const ControlList &reqControls);
> >
> > struct {
> > uint32_t exposure;
> > double gain;
> > } sensor;
> >
> > - uint32_t frame;
> > ControlList frameControls;
> > };
> >
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index e5a763fd2b08..b1b23fd8f927 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -607,9 +607,6 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >
> > IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts];
> >
> > - if (frameContext.frame != frame)
> > - LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context";
> > -
> > frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > frameContext.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >
> > @@ -654,7 +651,7 @@ 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 };
> > + context_.frameContexts[frame % kMaxFrameContexts] = { controls };
>
> I understand this right that once moving to the FCQ the frame number
> initialization and the check above will be there performed ?
That's right.
> Should we allow then to have a constructor for the IPA-specific class
> that accepts a frame number ?
What would you use that for ?
> Nits apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > }
> >
> > /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list