[libcamera-devel] [RFC PATCH] ipa: ipu3: Retain request controls in the IPA

[paul.elder at ideasonboard.com]

Hello,

On Thu, May 19, 2022 at 06:02:29PM +0200, Umang Jain via libcamera-devel wrote:
> Hi Kieran,
> 
> On 5/19/22 16:16, Kieran Bingham via libcamera-devel wrote:
> > To ensure consistency in processing control lists, an application does
> > not have to continually set controls that were already set, but not
> > expected to change.
> 
> 
> So what is this implying...
> 
> If application sends in manual-exposure=100 just for frame 25th, what is it
> expected to do to "unset" it from frame 26th?

imo it depends on what the desired effect is by "unsetting" it. If the
desire is to revert to the default value, then it's not possible (unless
the application saves it and re-applies it). If the desire is to revert
to auto then just set auto mode back on.

> 
> - Send in manual-exposure = 0 ?

That'll just set exposure to 0 :D

> 
> - send in no manual-exposure, in that case, I assume that the retained
> control list (purpose of this patch) is to set exposure=100 for frame 26th,
> which is probably right thing to do?, I am not sure...

Yeah, that's what should happen (what this patch enables).

> 
> 
> > 
> > To ensure we keep that state, merge incoming request control lists with
> > a stored ControlList that is owned by the IPA and store the state of
> > that merged list to the per frame FrameContext so that algorithms know
> > the full expected control state at that time.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > ---
> > 
> > RFC:
> > 
> > 1)
> > 
> > If ControlList.update() were implemented like .merge, but with
> > overriding existing values, instead of leaving them then this could be
> > simplified to :
> > 
> > +	retainedControls_.update(controls);
> > +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
> > 
> > Not yet sure if that's more efficient or if it doesn't make much
> > difference.

It makes the code look nicer :p

> > 
> > 2)
> > 
> > This will not support triggered controls that are set once to
> > 'fire'... Do we have / plan to have any of those?

Yeah that's kind of a big deal for AE.

Although the solution could be as simple (read: hacky) as having the IPA
overwrite the ExposureValue/AnalogueGain values at control application
time to the values calculated by AE when in auto mode. Those controls
need to report in metadata the actual values that were used anyway, so
it might actually be a viable solution.

Then, when the mode is switched from auto -> disabled and the control
isn't resubmitted, it'll retain the value that it had from the last time
auto was on (because the IPA overwrote them).

tl;dr, If the mode is auto and the manual controls are supplied, IPA
should overwrite the requested manual control values in the retained
requested contols probably at the same time as it reports them in
metadata. This is reationalized as "submitting manual controls is
invalid if the mode is auto" anyway, plus "the manual controls in
metadata are used to report the actual values that were used".

> > 
> > 3)
> > 
> > Do we want to keep both a delta control list (the incoming controls) as
> > well as the merged/retained list for full state so IPA's can know if
> > something was changed?
> 
> I think so yes. Algorithms might need to look up the delta to know in
> advance whether or not a manual control is set for a frame or not.
> Accordingly, they might decide the relevant parameter(s)
> For e.g. manual exposure but automatic gain.

I actually don't think this is necessary. If a control doesn't change,
it doesn't need to be resubmitted and it'll take the most recently set
value. From the IPA's point of view (with retention), every control will
be set every request. So it's not important to see the delta.

The exception of course is with trigger controls, which we don't want to
allow. AE almost had this, but I think we can rationalize it as
mentioned above: submitting manual controls during auto is invalid, so
we can do whatever we want, thus we don't even have to treat it as a
trigger, we can just overwrite it.


Paul

> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
> >   1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 2f6bb672f7bb..89fc34f86e46 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -176,6 +176,9 @@ private:
> >   	/* Local parameter storage */
> >   	struct IPAContext context_;
> > +
> > +	/* Control retention to maintain mode states */
> > +	ControlList retainedControls_;
> >   };
> >   /**
> > @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >   	/* Clean IPAActiveState at each reconfiguration. */
> >   	context_.activeState = {};
> > +	retainedControls_.clear();
> > +
> >   	IPAFrameContext initFrameContext;
> >   	context_.frameContexts.fill(initFrameContext);
> > @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
> >    */
> >   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
> >   {
> > +	/*
> > +	 * Controls are retained, to ensure any mode updates are persistant.
> > +	 * We merge them into our class control storage before assigning to
> > +	 * the frame context.
> > +	 *
> > +	 * \todo This will not support trigger controls and may need rework if
> > +	 *	 we add any to prevent continually retriggering.
> > +	 *
> > +	 * \todo We may wish to store both the full merged control list, as well
> > +	 *	 as the delta (\a controls) to facilitate algorithms identifying
> > +	 *	 when things have changed.
> > +	 */
> > +	ControlList mergeControls = controls;
> > +	mergeControls.merge(retainedControls_);
> > +	retainedControls_ = mergeControls;
> > +
> >   	/* \todo Start processing for 'frame' based on 'controls'. */
> > -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
> > +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
> >   }
> >   /**