[PATCH 3/3] ipa: rksip1: Remove setControls(0) to reduce startup oscillations

[Stefan Klug]

Hi Kieran

Thank you for the review.

On Tue, Mar 04, 2025 at 09:22:02PM +0000, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-02-28 12:55:55)
> > The call to setControls(0) is counter productive. At start() time, no
> > requests were queued and no stats were received. So setControls(0)
> > accesses a zeroed frame context and in turn sends 0 as gain, exposure
> > and vblank to the pipeline handler and DelayedControls. This leads to
> > strong oscillations on every start of the camera.
> > 
> > A proper fix for handling the startup controls still needs to be done
> > and was already started in [1] and [2].
> > 
> > From a DelayedControls point of view the call to setControls(0) is also
> > unnecessary as DelayedControls treat frame 0 as already being queued in
> > after initialization.
> > 
> > So it is save to just remove it and the removal fixes the zero
> 
> s/save/safe/
> 
> > effectiveExposureValue discussed in the previous patch for rkisp1.
> > 
> > [1]: https://patchwork.libcamera.org/patch/21708/
> > [2]: https://patchwork.libcamera.org/patch/22445/
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/rkisp1.cpp | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 5f1583e8219b..b09dd6efdf08 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -211,8 +211,6 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >  
> >  int IPARkISP1::start()
> >  {
> > -       setControls(0);
> > -
> 
> Aha, I thought I had recollection of this being added here more recently
> but this dates all the way back to 2021
> (1456efe7d51a3a0c6b57db4310f75b2a08ab1756) ... so I don't think it
> tramples on any active development of AGC.
> 
> 
> The above makes enough sense to me so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
> Though - now I check through patchwork - I see:
> 
> https://patchwork.libcamera.org/patch/21708/
> 
> from https://patchwork.libcamera.org/project/libcamera/list/?series=4728
> also touches code here. Have you reviewed that series? Does anything
> there need to be also considered or reviewed ?
> 

Yes, I'm aware of that series (that's why I mentioned it in the commit
message above). Time wise those changes overlapped with similar changes
I did to improve regulation in rkisp1. I'll continue to push on that and
will pull in changes from Mikhail as fit.

This series is just a tiny step in that direction and makes upcoming
changes easier.

Best regards,
Stefan

> --
> Kieran
> 
> 
> >         return 0;
> >  }
> >  
> > -- 
> > 2.43.0
> >