Message ID | 20210212113312.239076-1-naush@raspberrypi.com |
---|---|
Headers | show |
Series |
|
Related | show |
Hi all, We think there may be still further underlying issues causing some agc oscillations. As such please do not review this patch set until I send an update. Regards, Naush On Fri, 12 Feb 2021, 11:37 am Naushir Patuck, <naush@raspberrypi.com> wrote: > Hi all, > > This patchset mainly addresses some minor issues we have encountered with > DelayedControls > when running on the Raspberry Pi platform. Apologies for the slightly > long cover letter, > but I wanted to explain the problems we are seeing in a bit of detail :-) > > 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. It is, however, mostly inconsequential at > runtime. > > 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. > > 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(). > > Hope that is all reasonably well explained :-) Any questions, please do > shout. > > Regards, > Naush > > 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 | 72 ++++++++++------ > 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 | 82 +++++++++++++++++++ > 7 files changed, 162 insertions(+), 54 deletions(-) > create mode 100644 utils/raspberrypi/delayedctrls_parse.py > > -- > 2.25.1 > >