| Message ID | 20241220162724.756494-8-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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
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@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
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;
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(-)