[libcamera-devel] [PATCH v2 0/5] DelayedControls updates and fixes
David Plowman
david.plowman at raspberrypi.com
Tue Feb 16 13:19:39 CET 2021
Hi Naush
Don't feel terribly well qualified to review all this, however, as
I've been using it, for the whole set:
Tested-by: David Plowman <david.plowman at raspberrypi.com>
Thanks!
David
On Tue, 16 Feb 2021 at 08:53, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi,
>
> Here is a revised version of the DelayedControls patch. The only difference between v2 and v1 is in patch 4/5 where the fix was incomplete. To reiterate, here is a summary of the patches:
>
> Patch 1/5
> This adds the notion of priority write to fixup the known issue of settings VBLANK and
> EXPOSURE in a single batch. It is simply a port of the fix applied to StaggeredCtrl
> that had been reviewed and subsequently deprecated, so hopefully nothing too controversial.
>
> Patch 2/5
> This is simply a python script that I am using to debug the small problems (more details
> below) that we have encountered. It parses the DelayedControls logs and nicely tabulates
> the results to show what controls and values have been set/queued/get on each frame.
> This has helped me tremendously in identifying problems and fixing them. However, this
> may not be useful to others, so I am happy to not have this merged if folks do not think
> it is the right place to put it.
>
> Patch 3/5
> Fixes a spurious write to the device on startup. The following is an extract from using
> the script to parse the logs:
>
> Frame Action Gain Exposure Vblank
> 0 Write 0 [0] 52 [0] 531 [0] <<<<<
> 0 Get 0 [0] 52 [0] 531 [0]
> 0 Queue --- [-] --- [-] --- [-]
> 1 Write 0 [0] --- [-] --- [-]
> 1 Get 0 [0] 52 [0] 531 [0]
> 1 Queue --- [-] --- [-] --- [-]
> 2 Write --- [-] --- [-] --- [-]
> 2 Get 0 [1] 52 [1] 531 [1]
> 2 Queue 192 [4] 1664 [4] 531 [4]
>
> You can see above, on frame 0 we are writing controls to the sensor, but this is unneeded.
> This spurious write should really not happen as there is no controls queued by the
> pipeline_handler at this point. This problem is mostly inconsequential.
>
> Patch 4/5
> This fixes an issue where controls queued by the pipeline handler are delayed by and
> additional frame when writing. You can see better in the parsed log:
>
> Frame Action Gain Exposure Vblank
> 2 Write --- [-] --- [-] --- [-]
> 2 Get 0 [1] 52 [1] 531 [1]
> 2 Queue 192 [4] 1664 [4] 531 [4] <<<<<
> 3 Write --- [-] --- [-] --- [-] <<<<<
> 3 Get 0 [2] 52 [2] 531 [2]
> 3 Queue 192 [5] 1664 [5] 531 [5]
> 4 Write --- [-] 1664 [4] 531 [4] <<<<<
> 4 Get 0 [3] 52 [3] 531 [3]
> 4 Queue 192 [6] 1664 [6] 531 [6]
>
> On frame 2, we queue controls from the pipeline handler. Exposure and Vblank must be
> written one frame before gain, so you would expect them to be written on frame 3 as
> nothing else is in the queue. However, they only get written on frame 4, one frame
> later than expected.
>
> This is because of how DelayedControls handles "no-op" queue items, i.e. frames where
> we do not provide the helper with controls to use. It was adding one more no-op than
> needed, and causing an extra frame delay when setting the control on the device.
>
> As a consequence of this change, we must also ensure that controls that have been sent
> to the device must have the update flag cleared or there is a change it may be re-used
> when going around the ring buffer.
>
> Patch 5/5
> We had an off-by-one error when reading back values from the queues. See the parsed
> logs below:
>
> Frame Action Gain Exposure Vblank
> 7 Write 192 [6] 1664 [7] 531 [7] <<<<<
> 7 Get 192 [6] 1664 [6] 531 [6]
> 7 Queue 210 [9] 3174 [9] 1946 [9]
> 8 Write 192 [7] 3526 [8] 2298 [8]
> 8 Get 192 [7] 1664 [7] 531 [7] <<<<<
> 8 Queue 210 [10] 3174 [10] 1946 [10]
> 9 Write 213 [8] 3174 [9] 1946 [9]
> 9 Get 213 [8] 3526 [8] 2298 [8] <<<<<
> 9 Queue 213 [12] 3526 [12] 2298 [12]
>
> On frame 7, we write exposure and vblank with values 1664 and 531 respectively. These
> values take 2 frames to consume, so should be retuned to the pipeline handler by the
> DelayedControls::get() on frame 9. However, it returns on frame 8 instead.
>
> This only causes slight (but visible) oscillations in brightness in the AGC loop as
> the values used by the sensor are not in lock-step to what is reported by DelayedControls::get().
>
> Naushir Patuck (5):
> 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()
>
> include/libcamera/internal/delayed_controls.h | 13 ++-
> src/libcamera/delayed_controls.cpp | 79 ++++++++++-----
> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +-
> .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++-
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-
> test/delayed_contols.cpp | 20 ++--
> utils/raspberrypi/delayedctrls_parse.py | 98 +++++++++++++++++++
> 7 files changed, 183 insertions(+), 56 deletions(-)
> create mode 100644 utils/raspberrypi/delayedctrls_parse.py
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list