[RFC PATCH 7/7] ipa: rkisp1: Set frameContext.agc in queueRequest for auto mode also

[Paul Elder]

On Mon, Jan 13, 2025 at 02:40:30PM +0100, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the review. 
> 
> On Mon, Jan 13, 2025 at 12:31:02AM +0200, Laurent Pinchart wrote:
> > Hi Stefan,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Dec 20, 2024 at 05:26:53PM +0100, Stefan Klug wrote:
> > > If the agc is in auto mode, exposure time and gain used to be set on the
> > > frame context within prepare(). As exposure time and gain are used by
> > > getSensorControls(0) from within start() that is too late (prepare()
> > > hasn't run yet). Also prepare() is documented as the place to initialize
> > > the params buffer, not the frame context. From the pipeline point of
> > > view, prepare() gets called immediately after queueRequest(). Therefore
> > > we can safely move setting the frame context into queueRequest() to fix
> > > the sensor controls for start().
> > 
> > I don't think that's right. When queueRequest() is called the active
> > state doesn't contain recent enough agc values.
> > 
> > It may be that computeParams() is called right after queueRequest() in
> > the pipeline handler, but that's not how it should be.
> 
> Hmm, also a place where more conceptual work is needed (or at least I
> need to draw some graphs for me). In my understanding computeParams()
> should be called right before the isp starts to process the given frame.
> This is at least maxSensorDelay too late for sending out the sensor
> controls for that frame. So setting frameContext.agc.exposure in
> computeParams()/prepare() is the wrong place. I agree that
> queueRequest() is also not the best place, but still better in the sense
> that it is not too late but maybe not the most up to date value we could
> get.

My understanding was that the active state is written after the stats
buffer comes in (process()), but it needs to be read out right before
it's time to queue the params buffer (prepare()) and that's why for
populating the params buffer in auto mode we set it in prepare().

(Unless we don't have proper synchronization yet so in practice
queueRequest() and prepare() don't make much of a difference in terms of
timing.)


Paul

> > 
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 40e5a8f481b2..46be1413a728 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -243,7 +243,10 @@ void Agc::queueRequest(IPAContext &context,
> > >  
> > >  	frameContext.agc.autoEnabled = agc.autoEnabled;
> > >  
> > > -	if (!frameContext.agc.autoEnabled) {
> > > +	if (frameContext.agc.autoEnabled) {
> > > +		frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> > > +		frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > > +	} else {
> > >  		frameContext.agc.exposure = agc.manual.exposure;
> > >  		frameContext.agc.gain = agc.manual.gain;
> > >  	}
> > > @@ -283,11 +286,6 @@ void Agc::queueRequest(IPAContext &context,
> > >  void Agc::prepare(IPAContext &context, const uint32_t frame,
> > >  		  IPAFrameContext &frameContext, RkISP1Params *params)
> > >  {
> > > -	if (frameContext.agc.autoEnabled) {
> > > -		frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> > > -		frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > > -	}
> > > -
> > >  	if (frame > 0 && !frameContext.agc.updateMetering)
> > >  		return;
> > >  
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart