| Message ID | 20250228125600.3241397-4-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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 ? -- Kieran > return 0; > } > > -- > 2.43.0 >
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@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@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 > >
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@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@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 > > >
Hi Stefan, Thank you for the patch. On Fri, Feb 28, 2025 at 01:55:55PM +0100, Stefan Klug wrote: > 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 > 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@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); > - Could you add a \todo comment ? With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > return 0; > } >
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); - return 0; }
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 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@ideasonboard.com> --- src/ipa/rkisp1/rkisp1.cpp | 2 -- 1 file changed, 2 deletions(-)