[PATCH 1/3] ipa: rkisp1: Initialise AGC from FrameDurationLimits controls

[Paul Elder]

On Sun, Oct 27, 2024 at 08:54:02PM +0200, Laurent Pinchart wrote:
> On Mon, Oct 14, 2024 at 04:47:45PM +0100, Kieran Bingham wrote:
> > The IPA calculates and reports the FrameDurationLimits to applications
> > by configuring the ControlInfo accordingly during
> > IPARkISP1::updateControls()
> > 
> > We later need to know these limits during Agc::configure() for
> > initialising the ActiveState of the AGC implementation with the limits.
> > 
> > Store the FrameDurationLimits ControlInfo in the ControlInfoMap which is
> > now present in the IPAContext so that it is commonly available for the
> > AGC algorithm, removing the 'todo' accordingly.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 9 +++------
> >  src/ipa/rkisp1/rkisp1.cpp         | 5 ++---
> >  2 files changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 17d074d9c03e..33f902862c4a 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -178,12 +178,9 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >  	context.activeState.agc.meteringMode =
> >  		static_cast<controls::AeMeteringModeEnum>(meteringModes_.begin()->first);
> >  
> > -	/*
> > -	 * \todo This should probably come from FrameDurationLimits instead,
> > -	 * except it's computed in the IPA and not here so we'd have to
> > -	 * recompute it.
> > -	 */
> > -	context.activeState.agc.maxFrameDuration = context.configuration.sensor.maxShutterSpeed;
> > +	/* Limit the frame duration to match current initialisation */
> > +	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> 
> const
> 
> > +	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> 
> That's long, maybe
> 
> 	context.activeState.agc.maxFrameDuration =
> 		std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> 
> or
> 
> 	int64_t maxFrameDuration = frameDurationLimits.max().get<int64_t>();
> 	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> >  
> >  	/*
> >  	 * Define the measurement window for AGC as a centered rectangle
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 9e161cabdea4..47777ece783f 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -432,9 +432,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> >  		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> >  	}
> >  
> > -	ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> > -							      frameDurations[1],
> > -							      frameDurations[2]);
> > +	context_.ctrlMap[&controls::FrameDurationLimits] =
> > +		ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]);
> 
> I wonder if all this should be moved to the AGC algorithm. There are
> cross-dependencies between the IPARkISP1 class and the AGC algorithm
> that make the code fragile. Cleanups would be nice.

Probably. It'll need some change in the rkisp1 algorithm interface
because we'd have to pass sensor controls to the algorithms at both init
and configure time. I'll add a todo at least.


Paul

> 
> >  
> >  	ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());
> >  	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);