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

[Kieran Bingham]

Quoting Stefan Klug (2025-03-05 09:51:15)
> 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

Oopps, Sorry - I missed that !

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

Great, Looking forward to this series progressing!
--
Kieran

> Best regards,
> Stefan
> 
> > --
> > Kieran
> > 
> > 
> > >         return 0;
> > >  }
> > >  
> > > -- 
> > > 2.43.0
> > >