<div dir="ltr">Hi all,<div><br></div><div>Gentle ping, would appreciate any feedback for these changes.</div><div><br></div><div>Regards,</div><div>Naush</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 16 Feb 2021 at 08:53, Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
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:<br>
<br>
Patch 1/5<br>
This adds the notion of priority write to fixup the known issue of settings VBLANK and<br>
EXPOSURE in a single batch. It is simply a port of the fix applied to StaggeredCtrl<br>
that had been reviewed and subsequently deprecated, so hopefully nothing too controversial.<br>
<br>
Patch 2/5<br>
This is simply a python script that I am using to debug the small problems (more details<br>
below) that we have encountered. It parses the DelayedControls logs and nicely tabulates<br>
the results to show what controls and values have been set/queued/get on each frame.<br>
This has helped me tremendously in identifying problems and fixing them. However, this<br>
may not be useful to others, so I am happy to not have this merged if folks do not think<br>
it is the right place to put it.<br>
<br>
Patch 3/5<br>
Fixes a spurious write to the device on startup. The following is an extract from using<br>
the script to parse the logs:<br>
<br>
Frame Action Gain Exposure Vblank<br>
0 Write 0 [0] 52 [0] 531 [0] <<<<<<br>
0 Get 0 [0] 52 [0] 531 [0]<br>
0 Queue --- [-] --- [-] --- [-]<br>
1 Write 0 [0] --- [-] --- [-]<br>
1 Get 0 [0] 52 [0] 531 [0]<br>
1 Queue --- [-] --- [-] --- [-]<br>
2 Write --- [-] --- [-] --- [-]<br>
2 Get 0 [1] 52 [1] 531 [1]<br>
2 Queue 192 [4] 1664 [4] 531 [4]<br>
<br>
You can see above, on frame 0 we are writing controls to the sensor, but this is unneeded.<br>
This spurious write should really not happen as there is no controls queued by the<br>
pipeline_handler at this point. This problem is mostly inconsequential.<br>
<br>
Patch 4/5<br>
This fixes an issue where controls queued by the pipeline handler are delayed by and<br>
additional frame when writing. You can see better in the parsed log:<br>
<br>
Frame Action Gain Exposure Vblank<br>
2 Write --- [-] --- [-] --- [-]<br>
2 Get 0 [1] 52 [1] 531 [1]<br>
2 Queue 192 [4] 1664 [4] 531 [4] <<<<<<br>
3 Write --- [-] --- [-] --- [-] <<<<<<br>
3 Get 0 [2] 52 [2] 531 [2]<br>
3 Queue 192 [5] 1664 [5] 531 [5]<br>
4 Write --- [-] 1664 [4] 531 [4] <<<<<<br>
4 Get 0 [3] 52 [3] 531 [3]<br>
4 Queue 192 [6] 1664 [6] 531 [6]<br>
<br>
On frame 2, we queue controls from the pipeline handler. Exposure and Vblank must be<br>
written one frame before gain, so you would expect them to be written on frame 3 as<br>
nothing else is in the queue. However, they only get written on frame 4, one frame<br>
later than expected.<br>
<br>
This is because of how DelayedControls handles "no-op" queue items, i.e. frames where<br>
we do not provide the helper with controls to use. It was adding one more no-op than<br>
needed, and causing an extra frame delay when setting the control on the device.<br>
<br>
As a consequence of this change, we must also ensure that controls that have been sent<br>
to the device must have the update flag cleared or there is a change it may be re-used<br>
when going around the ring buffer. <br>
<br>
Patch 5/5<br>
We had an off-by-one error when reading back values from the queues. See the parsed<br>
logs below:<br>
<br>
Frame Action Gain Exposure Vblank<br>
7 Write 192 [6] 1664 [7] 531 [7] <<<<<<br>
7 Get 192 [6] 1664 [6] 531 [6]<br>
7 Queue 210 [9] 3174 [9] 1946 [9]<br>
8 Write 192 [7] 3526 [8] 2298 [8]<br>
8 Get 192 [7] 1664 [7] 531 [7] <<<<<<br>
8 Queue 210 [10] 3174 [10] 1946 [10]<br>
9 Write 213 [8] 3174 [9] 1946 [9]<br>
9 Get 213 [8] 3526 [8] 2298 [8] <<<<<<br>
9 Queue 213 [12] 3526 [12] 2298 [12]<br>
<br>
On frame 7, we write exposure and vblank with values 1664 and 531 respectively. These<br>
values take 2 frames to consume, so should be retuned to the pipeline handler by the<br>
DelayedControls::get() on frame 9. However, it returns on frame 8 instead.<br>
<br>
This only causes slight (but visible) oscillations in brightness in the AGC loop as<br>
the values used by the sensor are not in lock-step to what is reported by DelayedControls::get().<br>
<br>
Naushir Patuck (5):<br>
libcamera: delayed_controls: Add notion of priority write<br>
utils: raspberrypi: Add a DelayedControls log parser<br>
libcamera: delayed_controls: Remove unneeded write when starting up<br>
libcamera: delayed_controls: Remove spurious no-op queued controls<br>
libcamera: delayed_controls: Fix off-by-one error in get()<br>
<br>
include/libcamera/internal/delayed_controls.h | 13 ++-<br>
src/libcamera/delayed_controls.cpp | 79 ++++++++++-----<br>
src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +-<br>
.../pipeline/raspberrypi/raspberrypi.cpp | 13 ++-<br>
src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +-<br>
test/delayed_contols.cpp | 20 ++--<br>
utils/raspberrypi/delayedctrls_parse.py | 98 +++++++++++++++++++<br>
7 files changed, 183 insertions(+), 56 deletions(-)<br>
create mode 100644 utils/raspberrypi/delayedctrls_parse.py<br>
<br>
-- <br>
2.25.1<br>
<br>
</blockquote></div>