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

22447 diff mbox series

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().

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

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.

> 
> Signed-off-by: Stefan Klug <stefan.klug@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;
>

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.

Cheers,
Stefan

> 
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@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