| Message ID | 20210304081728.1058394-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi, Another nudge for some feedback on this patch series. Regards, Naush On Thu, 4 Mar 2021 at 08:17, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi, > > This version (v4) of the patch series adds two further commits to v3. > > Patches 1 to 5 are identical to v3, except that I have added a line to the > commit > messages saying the change would break the unit tests because of a change > in the > logic of the helper. > > Patch 6 updates the unit tests with the correct behavior. Unfortunately, > my > attempts to get vimc working on my platform did not end well, so I made a > local > mod to run the tests with a physical camera device. All tests do pass, but > I > would appreciate if somebody else could run this again with the vimc > device to > verify everything is passing. > > Patch 7 simply fixes a typo in the unit test filename. > > Thanks, > Naush > > Naushir Patuck (7): > libcamera: delayed_controls: Add notion of priority write > utils: raspberrypi: Add a DelayedControls log parser > libcamera: delayed_controls: Remove unneeded write when starting up > libcamera: delayed_controls: Remove spurious no-op queued controls > libcamera: delayed_controls: Fix off-by-one error in get() > test: delayed_controls: Fixup tests after recent DelayedControls > changes > test: delayed_controls: Rename delayed_contols.cpp to > delayed_controls.cpp > > include/libcamera/internal/delayed_controls.h | 13 +- > src/libcamera/delayed_controls.cpp | 74 ++++++++---- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 13 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > ...layed_contols.cpp => delayed_controls.cpp} | 51 +++++--- > test/meson.build | 2 +- > utils/raspberrypi/delayedctrls_parse.py | 111 ++++++++++++++++++ > 8 files changed, 219 insertions(+), 61 deletions(-) > rename test/{delayed_contols.cpp => delayed_controls.cpp} (84%) > create mode 100644 utils/raspberrypi/delayedctrls_parse.py > > -- > 2.25.1 > >
Hi Naush, On Thu, Mar 04, 2021 at 08:17:28AM +0000, Naushir Patuck wrote: > There was a typo in the unit test filename. Fix this typo, no other > functional change in this commit. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > test/{delayed_contols.cpp => delayed_controls.cpp} | 0 > test/meson.build | 2 +- > 2 files changed, 1 insertion(+), 1 deletion(-) > rename test/{delayed_contols.cpp => delayed_controls.cpp} (100%) > > diff --git a/test/delayed_contols.cpp b/test/delayed_controls.cpp > similarity index 100% > rename from test/delayed_contols.cpp > rename to test/delayed_controls.cpp > diff --git a/test/meson.build b/test/meson.build > index 89e6ebff5f6b..310b7cad0600 100644 > --- a/test/meson.build > +++ b/test/meson.build > @@ -30,7 +30,7 @@ internal_tests = [ > ['bayer-format', 'bayer-format.cpp'], > ['byte-stream-buffer', 'byte-stream-buffer.cpp'], > ['camera-sensor', 'camera-sensor.cpp'], > - ['delayed_contols', 'delayed_contols.cpp'], > + ['delayed_controls', 'delayed_controls.cpp'], > ['event', 'event.cpp'], > ['event-dispatcher', 'event-dispatcher.cpp'], > ['event-thread', 'event-thread.cpp'], > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On Thu, Mar 04, 2021 at 08:17:21AM +0000, Naushir Patuck wrote: > Hi, > > This version (v4) of the patch series adds two further commits to v3. > > Patches 1 to 5 are identical to v3, except that I have added a line to the commit > messages saying the change would break the unit tests because of a change in the > logic of the helper. > > Patch 6 updates the unit tests with the correct behavior. Unfortunately, my > attempts to get vimc working on my platform did not end well, so I made a local > mod to run the tests with a physical camera device. All tests do pass, but I > would appreciate if somebody else could run this again with the vimc device to > verify everything is passing. > > Patch 7 simply fixes a typo in the unit test filename. Just for my information, I'd like to get a clear understanding of the issues that this series fixes. As far as I know, it addresses oscillations in the algorithms for Raspberry Pi. Were those oscillations introduced by the switch from the StaggeredCtrls implementation to DelayedControls but only noticed later, or are there other changes that are involved here ? > Naushir Patuck (7): > libcamera: delayed_controls: Add notion of priority write > utils: raspberrypi: Add a DelayedControls log parser > libcamera: delayed_controls: Remove unneeded write when starting up > libcamera: delayed_controls: Remove spurious no-op queued controls > libcamera: delayed_controls: Fix off-by-one error in get() > test: delayed_controls: Fixup tests after recent DelayedControls > changes > test: delayed_controls: Rename delayed_contols.cpp to > delayed_controls.cpp > > include/libcamera/internal/delayed_controls.h | 13 +- > src/libcamera/delayed_controls.cpp | 74 ++++++++---- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 13 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > ...layed_contols.cpp => delayed_controls.cpp} | 51 +++++--- > test/meson.build | 2 +- > utils/raspberrypi/delayedctrls_parse.py | 111 ++++++++++++++++++ > 8 files changed, 219 insertions(+), 61 deletions(-) > rename test/{delayed_contols.cpp => delayed_controls.cpp} (84%) > create mode 100644 utils/raspberrypi/delayedctrls_parse.py
Hi Laurent, On Tue, 9 Mar 2021 at 10:33, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Thu, Mar 04, 2021 at 08:17:21AM +0000, Naushir Patuck wrote: > > Hi, > > > > This version (v4) of the patch series adds two further commits to v3. > > > > Patches 1 to 5 are identical to v3, except that I have added a line to > the commit > > messages saying the change would break the unit tests because of a > change in the > > logic of the helper. > > > > Patch 6 updates the unit tests with the correct behavior. > Unfortunately, my > > attempts to get vimc working on my platform did not end well, so I made > a local > > mod to run the tests with a physical camera device. All tests do pass, > but I > > would appreciate if somebody else could run this again with the vimc > device to > > verify everything is passing. > > > > Patch 7 simply fixes a typo in the unit test filename. > > Just for my information, I'd like to get a clear understanding of the > issues that this series fixes. As far as I know, it addresses > oscillations in the algorithms for Raspberry Pi. Were those oscillations > introduced by the switch from the StaggeredCtrls implementation to > DelayedControls but only noticed later, or are there other changes that > are involved here ? > This series addresses the AGC oscillations observed on the Raspberry Pi using our algorithm. The key difference between DelayedControls and StaggeredCtrls is that the former allows us to queue multiple batches of ctrls, whereas the latter would only apply the last queued value. Hence there is a pair of indexes to track in DelayedControls, vs a single index in StaggeredCtrl. Because of this difference, the behavior between the two was slightly different (e.g. an additional frame needed to queue a control to the device by DelayedControls, causing a delay mismatch). This patch aligns the behavior of the two while keeping the new functionality of allowing us to queue multiple batches of ctrls. This effectively fixes our AGC oscillations as well. This change should work equally well for all other vendor AGC algorithms, there is nothing specific to Raspberry Pi in these commits. Hope that makes things clearer. Regards, Naush > > > Naushir Patuck (7): > > libcamera: delayed_controls: Add notion of priority write > > utils: raspberrypi: Add a DelayedControls log parser > > libcamera: delayed_controls: Remove unneeded write when starting up > > libcamera: delayed_controls: Remove spurious no-op queued controls > > libcamera: delayed_controls: Fix off-by-one error in get() > > test: delayed_controls: Fixup tests after recent DelayedControls > > changes > > test: delayed_controls: Rename delayed_contols.cpp to > > delayed_controls.cpp > > > > include/libcamera/internal/delayed_controls.h | 13 +- > > src/libcamera/delayed_controls.cpp | 74 ++++++++---- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 13 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- > > ...layed_contols.cpp => delayed_controls.cpp} | 51 +++++--- > > test/meson.build | 2 +- > > utils/raspberrypi/delayedctrls_parse.py | 111 ++++++++++++++++++ > > 8 files changed, 219 insertions(+), 61 deletions(-) > > rename test/{delayed_contols.cpp => delayed_controls.cpp} (84%) > > create mode 100644 utils/raspberrypi/delayedctrls_parse.py > > -- > Regards, > > Laurent Pinchart >
On 04/03/2021 08:17, Naushir Patuck wrote: > There was a typo in the unit test filename. Fix this typo, no other > functional change in this commit. > Phew, an easy one.... ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > test/{delayed_contols.cpp => delayed_controls.cpp} | 0 > test/meson.build | 2 +- > 2 files changed, 1 insertion(+), 1 deletion(-) > rename test/{delayed_contols.cpp => delayed_controls.cpp} (100%) > > diff --git a/test/delayed_contols.cpp b/test/delayed_controls.cpp > similarity index 100% > rename from test/delayed_contols.cpp > rename to test/delayed_controls.cpp > diff --git a/test/meson.build b/test/meson.build > index 89e6ebff5f6b..310b7cad0600 100644 > --- a/test/meson.build > +++ b/test/meson.build > @@ -30,7 +30,7 @@ internal_tests = [ > ['bayer-format', 'bayer-format.cpp'], > ['byte-stream-buffer', 'byte-stream-buffer.cpp'], > ['camera-sensor', 'camera-sensor.cpp'], > - ['delayed_contols', 'delayed_contols.cpp'], > + ['delayed_controls', 'delayed_controls.cpp'], > ['event', 'event.cpp'], > ['event-dispatcher', 'event-dispatcher.cpp'], > ['event-thread', 'event-thread.cpp'], >
Hi, This version (v4) of the patch series adds two further commits to v3. Patches 1 to 5 are identical to v3, except that I have added a line to the commit messages saying the change would break the unit tests because of a change in the logic of the helper. Patch 6 updates the unit tests with the correct behavior. Unfortunately, my attempts to get vimc working on my platform did not end well, so I made a local mod to run the tests with a physical camera device. All tests do pass, but I would appreciate if somebody else could run this again with the vimc device to verify everything is passing. Patch 7 simply fixes a typo in the unit test filename. Thanks, Naush Naushir Patuck (7): libcamera: delayed_controls: Add notion of priority write utils: raspberrypi: Add a DelayedControls log parser libcamera: delayed_controls: Remove unneeded write when starting up libcamera: delayed_controls: Remove spurious no-op queued controls libcamera: delayed_controls: Fix off-by-one error in get() test: delayed_controls: Fixup tests after recent DelayedControls changes test: delayed_controls: Rename delayed_contols.cpp to delayed_controls.cpp include/libcamera/internal/delayed_controls.h | 13 +- src/libcamera/delayed_controls.cpp | 74 ++++++++---- src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +- .../pipeline/raspberrypi/raspberrypi.cpp | 13 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +- ...layed_contols.cpp => delayed_controls.cpp} | 51 +++++--- test/meson.build | 2 +- utils/raspberrypi/delayedctrls_parse.py | 111 ++++++++++++++++++ 8 files changed, 219 insertions(+), 61 deletions(-) rename test/{delayed_contols.cpp => delayed_controls.cpp} (84%) create mode 100644 utils/raspberrypi/delayedctrls_parse.py