| Message ID | 20240712143227.3036702-2-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, thanks for the set On 12/07/2024 15:32, Stefan Klug wrote: > When the color gains where set manually it was possible to specify a > gain that wrapped the hardware limits. It would also be possible to > further tune the floating point limits, but that is an error prone > approach. So the limits are imposed on the integers, just before writing > to the hardware. This noticeably reduces some oscillations in the awb > regulation. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- The code change is fine: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> But how's it possible that you get values larger than 1023 here? The control value is declared with a range of 0 to 3.996; is it that the limits aren't checked when controls are set, or is there some floating point magic going on? > src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index a01fe5d90973..1a5d4776970a 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -120,10 +120,14 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue; > } > > - params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green; > - params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue; > - params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red; > - params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green; > + params->others.awb_gain_config.gain_green_b = > + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); > + params->others.awb_gain_config.gain_blue = > + std::clamp<int>(256 * frameContext.awb.gains.blue, 0, 0x3ff); > + params->others.awb_gain_config.gain_red = > + std::clamp<int>(256 * frameContext.awb.gains.red, 0, 0x3ff); > + params->others.awb_gain_config.gain_green_r = > + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); > > /* Update the gains. */ > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN;
Quoting Stefan Klug (2024-07-12 15:32:02) > When the color gains where set manually it was possible to specify a > gain that wrapped the hardware limits. It would also be possible to > further tune the floating point limits, but that is an error prone > approach. So the limits are imposed on the integers, just before writing > to the hardware. This noticeably reduces some oscillations in the awb > regulation. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index a01fe5d90973..1a5d4776970a 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -120,10 +120,14 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue; > } > > - params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green; > - params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue; > - params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red; > - params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green; > + params->others.awb_gain_config.gain_green_b = Are these parameters clamped/verified by the kernel ? Is that where the oscillations come from (if the kernel applies different values than we track/believe/expect?) Should we be clamping frameContext.awb.gains.green so we track what value actually is applied in the frame context? > + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); > + params->others.awb_gain_config.gain_blue = > + std::clamp<int>(256 * frameContext.awb.gains.blue, 0, 0x3ff); > + params->others.awb_gain_config.gain_red = > + std::clamp<int>(256 * frameContext.awb.gains.red, 0, 0x3ff); > + params->others.awb_gain_config.gain_green_r = > + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); > Though - I don't object to ensuring we clamp before setting in the param buffer - but it scares me thinking there are a lot more values we need to perhaps validate before storing in the parameter buffers? Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > /* Update the gains. */ > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN; > -- > 2.43.0 >
On Fri, Jul 12, 2024 at 04:32:02PM +0200, Stefan Klug wrote: > When the color gains where set manually it was possible to specify a s/where/are/ > gain that wrapped the hardware limits. It would also be possible to > further tune the floating point limits, but that is an error prone > approach. So the limits are imposed on the integers, just before writing > to the hardware. This noticeably reduces some oscillations in the awb > regulation. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> tbh these should be protected against once in the kernel and against in the ControlInfo but until those are fixed we need this. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index a01fe5d90973..1a5d4776970a 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -120,10 +120,14 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, > frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue; > } > > - params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green; > - params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue; > - params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red; > - params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green; > + params->others.awb_gain_config.gain_green_b = > + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); > + params->others.awb_gain_config.gain_blue = > + std::clamp<int>(256 * frameContext.awb.gains.blue, 0, 0x3ff); > + params->others.awb_gain_config.gain_red = > + std::clamp<int>(256 * frameContext.awb.gains.red, 0, 0x3ff); > + params->others.awb_gain_config.gain_green_r = > + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); > > /* Update the gains. */ > params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN; > -- > 2.43.0 >
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index a01fe5d90973..1a5d4776970a 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -120,10 +120,14 @@ void Awb::prepare(IPAContext &context, const uint32_t frame, frameContext.awb.gains.blue = context.activeState.awb.gains.automatic.blue; } - params->others.awb_gain_config.gain_green_b = 256 * frameContext.awb.gains.green; - params->others.awb_gain_config.gain_blue = 256 * frameContext.awb.gains.blue; - params->others.awb_gain_config.gain_red = 256 * frameContext.awb.gains.red; - params->others.awb_gain_config.gain_green_r = 256 * frameContext.awb.gains.green; + params->others.awb_gain_config.gain_green_b = + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); + params->others.awb_gain_config.gain_blue = + std::clamp<int>(256 * frameContext.awb.gains.blue, 0, 0x3ff); + params->others.awb_gain_config.gain_red = + std::clamp<int>(256 * frameContext.awb.gains.red, 0, 0x3ff); + params->others.awb_gain_config.gain_green_r = + std::clamp<int>(256 * frameContext.awb.gains.green, 0, 0x3ff); /* Update the gains. */ params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN;
When the color gains where set manually it was possible to specify a gain that wrapped the hardware limits. It would also be possible to further tune the floating point limits, but that is an error prone approach. So the limits are imposed on the integers, just before writing to the hardware. This noticeably reduces some oscillations in the awb regulation. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)