[v3,00/41] rkisp1: pipeline rework for PFC

28244 mbox series

Hi all,

This is v3 of the pipeline rework. v2 is available here:
https://patchwork.libcamera.org/project/libcamera/list/?series=5849

After the feedback on v1 it got clear that the name is not
really well chosen as PFC is not the content of the series but rather
the preparatory work needed to later implement PFC on rkisp1/imx8mp.

I'll start with the changes to v2 first, the slightly adapted cover
letter from v2 follows.

In v3 a lot of cleanup was added and the series now passes CI. As this
code is also in daily use in several places, quite some small bugfixes
and fixups accumulated. The biggest changes (see local logs for details)
in v3 are as follows:

- The FCQueue no longer exits with a fatal log on a queue overrun, and
  these corner cases are handled gracefully.
- Rebased to current master which includes the ipa rework. Due to that,
  the influence of this series on other platforms became larger and
needs testing.
- I dropped the WIP patch that replaced the FCQueue by logic in the IPA
  base classes as it should be added on top and this series is already
big enough.

The two step approch to first split buffer queues and in a followup
patch use the BufferQueue class is still kept as I didn't get much
feedback on the BufferQueue and it shows how the code evolved. I still
feel like that concept makes things a lot easier.

Now to the big picture (mostly the v1 cover-letter):

From a very high level view, this series implements the mechanics needed
to cope with unstable hardware and to to be able to implement per frame
control on the rkisp1. It does not yet touch the possible PFC mechanics
like fast-tracked controls. It was only tested on the imx8mp together
with the dewarper so there might still be issues on other rkisp1
platforms.

To do correct per-frame-control we need to synchronize multiple things:
- The sensor needs to be fed with parameters according to the specific
  delays
- The ISP parameters need to be in sync also. This is especially
  important if for example digital gain in the ISP is used to mitigate
quantization in the sensor gain and therefore need to perfectly match
the sensor timing.

The current IPA model has basically two loops:

The sensor controls loop:
- Pipeline::statsBufferReady()
- IPA::processStats()
- IPA::setSensorControls.emit()
  - Pipeline::DelayedControls.push()
- IPA::metadataReady.emit()
  - Pipeline::tryCompleteRequest()

The ISP params loop:
- Pipeline::queueRequestDevice()
- IPA::queueRequest()
- IPA::computeParams()
- Pipeline::paramsComputed()
  - Pipeline::queueParams()

This means the sensor controls and params are only indirectly coupled.
To add to the complexity, in the sensor controls loop, the frame context
for the frame that produced the stats is used as basis for the call to
setSensorControls(). That is the point where I often fail to follow the
chain of events.

The reworked model now changes the mechanics: In multiple discussions it
got clear that even though theoretically the ISP params have a 0-1 frame
delay and are therefore 1 frame faster than typical sensor delays it is
more important to ensure that sensor data and ISP params are in sync
than to optimiza for that one frame. We can still have a look at that
later. Stating that sensor and params should be in sync also means that
we can produce them at the same time. So the pipeline was reworked with
the following premises in mind:

- Pipeline and IPA now handle sensor sequence numbers instead
  of request numbers.

The stats loop only updates active state and fills metadata:
- ISP calculates stats
- IPA::processStats()
- IPA::metadataReady.emit()
- Pipeline::tryCompleteRequest()

The sensor loop triggers calculation of params + sensor controls:
- Pipeline::startFrame()
- Pipeline::DelayedControls.applyControls(n+delay)
- IPA::computeParams(n+delay+1)
  - IPA::setSensorControls.emit()
    - Pipeline::DelayedControls.push()
- Pipeline::paramsComputed()
  - Pipeline::queueParams()

The request loop:
- Pipeline::queueRequestDevice()
- IPA::queueRequest()

The main change is that sensor controls and ISP params are computed in
response to computeParams(). This has the nice effect, that we just need
to ensure that we call computeParams() early enough to send it out to
the sensor as that is usually the longest delay.

A request underrun is easy to handle as the params/stats machinery just
continues to run with scratch buffers in the kernel. The only change to
do there is to repurpose the IPA::queueRequest to a
IPA::initializeFrameContext and call that on demand when params for a
given frame are needed.

Additionally to this conceptual change, the stats, image and params
buffers were decoupled. Due to the asyncronous nature of V4L2 and the
usage of scratch buffers in the kernel it is impossible to ensure that a
grouping of (stats, image and params) stays in sync. Especially under
high load or with bad connectors, this breaks. Decoupling them makes a
few things easier but also brings a bit of complexity. This rework and
most of the loop structure change is unfortunately in one big commit
(patch 25) as I didn't see a way to split that into easy to digest
parts.

For the (re)synchronization to work properly, I needed to add
synchronization helpers.

In the current code we have a minimal resync facility that adds 1 if it
detects a sequence that is bigger than the expected one. The problem
here is the queue in the kernel. If a offset is detected when dequing a
buffer, we can compensate for that offset when queuing new buffers. But
in case there are n buffers queued when we observe the error, we will
most likely observe that error another n times until the compensation
applies. By then we have overcompensated by n and started a beautiful
oscillation. To handle that we need to track the observed error and the
applied compensation over time.

Another reason for this series was to get started faster. Therefor
controls passed to Camera::start() are now handled properly. In that
context a initial params buffer is also filled. This ensures that the
stats on frame 0 are calculated based on the correct settings and
prevents large oscillations at the beginning of the stream.

Open Todos:
- The semantics of delayed controls was modified to not delay, but more
  to a record and query. Maybe we should rename it?
- Testing on more platforms (ipu3/simple/mali)

Best regards,
Stefan

Jacopo Mondi (1):
  libcamera: v4l2_videodevice: Do not hide frame drops

Stefan Klug (40):
  libcamera: delayed_controls: Add push() function that accepts a
    sequence number
  libcamera: delayed_controls: Handle missed pushes
  libcamera: delayed_controls: Increase log level for dummy pushes
  libcamera: delayed_controls: Queue noop when needed, not before
  libcamera: delayed_controls: Add maxDelay() function
  pipeline: rkisp1: Add a frameStart function to handle
    DelayedControls::applyControls
  pipeline: rkisp1: Include frame number when pushing to delayed
    controls
  libcamera: delayed_controls: Change semantics of sequence numbers
  libcamera: delayed_controls: Ignore double pushes for the same frame
    number
  libipa: fc_queue: Rename template argument to FC
  libipa: fc_queue: Add trailing underscore to private members of
    FrameContext
  ipa: rkisp1: Refactor setControls()
  ipa: rkisp1: Move setSensorControls signal to computeParams
  ipa: rkisp1: Add initializeFrameContext() function
  pipeline: rkisp1: Apply initial controls
  libipa: agc: Pass agc::Session to prepare()
  libipa: agc: Process frame duration at the right time
  ipa: rkisp1: Allow processStats() to be called without stats buffer
  libipa: awb: Populate metadata when stats are invalid
  ipa: rksip1: Call libipa::Awb::process() when stats are null
  libipa: agc: Populate frameContext in queueRequest() in auto mode
  libipa: awb: Populate frameContext in queueRequest() in auto mode
  pipeline: rkisp1: Pass bufferId to metadataReady()
  ipa: rkisp1: Lazy initialise frame context
  libcamera: internal: Add SequenceSyncHelper class
  libcamera: internal: Add a BufferQueue class to handle buffer queues
  pipeline: rkisp1: Decouple image, stats and param buffers
  pipeline: rkisp1: Reinstantiate maxQueuedRequestsDevice limit
  pipeline: rkisp1: Correctly handle params buffer for frame 0
  pipeline: rkisp1: Fix buffer metadata when using the dewarper
  pipeline: rkisp1: rkisp1_path: Modify interface to be compatible with
    BufferQueue
  pipeline: rkisp1: Use BufferQueue for buffer handling
  libipa: algorithm: Update documentation
  libipa: fc_queue: Early out on first context
  libipa: fc_queue: Return nullptr instead of crashing
  libipa: algorithm: Add a initialize parameter to prepare
  ipa: rkisp1: Gracefully handle FCQueue overruns
  ipa: rkisp1: Handle frame jumps in calls to computeParams()
  libipa: agc: Make startup frames and regulations speed configurable
  ipa: rkisp1: Increase regulation speed

 include/libcamera/internal/buffer_queue.h     | 136 +++
 include/libcamera/internal/delayed_controls.h |   3 +
 include/libcamera/internal/meson.build        |   2 +
 .../libcamera/internal/sequence_sync_helper.h |  31 +
 include/libcamera/internal/v4l2_videodevice.h |   1 -
 include/libcamera/ipa/rkisp1.mojom            |  14 +-
 src/ipa/ipu3/algorithms/af.cpp                |   3 +-
 src/ipa/ipu3/algorithms/af.h                  |   3 +-
 src/ipa/ipu3/algorithms/agc.cpp               |   5 +-
 src/ipa/ipu3/algorithms/agc.h                 |   3 +-
 src/ipa/ipu3/algorithms/awb.cpp               |   3 +-
 src/ipa/ipu3/algorithms/awb.h                 |   3 +-
 src/ipa/ipu3/algorithms/blc.cpp               |   4 +-
 src/ipa/ipu3/algorithms/blc.h                 |   3 +-
 src/ipa/ipu3/algorithms/tone_mapping.cpp      |   4 +-
 src/ipa/ipu3/algorithms/tone_mapping.h        |   3 +-
 src/ipa/ipu3/ipu3.cpp                         |  31 +-
 src/ipa/libipa/agc.cpp                        |  53 +-
 src/ipa/libipa/agc.h                          |   8 +-
 src/ipa/libipa/agc_mean_luminance.cpp         |  25 +-
 src/ipa/libipa/agc_mean_luminance.h           |   3 +
 src/ipa/libipa/algorithm.cpp                  |  52 +-
 src/ipa/libipa/algorithm.h                    |   3 +-
 src/ipa/libipa/awb.cpp                        |  14 +-
 src/ipa/libipa/fc_queue.cpp                   |  47 +-
 src/ipa/libipa/fc_queue.h                     | 143 ++-
 src/ipa/mali-c55/algorithms/agc.cpp           |   9 +-
 src/ipa/mali-c55/algorithms/agc.h             |   3 +-
 src/ipa/mali-c55/algorithms/awb.cpp           |   7 +-
 src/ipa/mali-c55/algorithms/awb.h             |   3 +-
 src/ipa/mali-c55/algorithms/blc.cpp           |   6 +-
 src/ipa/mali-c55/algorithms/blc.h             |   3 +-
 src/ipa/mali-c55/algorithms/ccm.cpp           |   5 +-
 src/ipa/mali-c55/algorithms/ccm.h             |   3 +-
 src/ipa/mali-c55/algorithms/lsc.cpp           |   4 +-
 src/ipa/mali-c55/algorithms/lsc.h             |   3 +-
 src/ipa/mali-c55/mali-c55.cpp                 |  33 +-
 src/ipa/rkisp1/algorithms/agc.cpp             |  21 +-
 src/ipa/rkisp1/algorithms/agc.h               |   3 +-
 src/ipa/rkisp1/algorithms/awb.cpp             |  12 +-
 src/ipa/rkisp1/algorithms/awb.h               |   3 +-
 src/ipa/rkisp1/algorithms/blc.cpp             |   7 +-
 src/ipa/rkisp1/algorithms/blc.h               |   3 +-
 src/ipa/rkisp1/algorithms/ccm.cpp             |   3 +-
 src/ipa/rkisp1/algorithms/ccm.h               |   3 +-
 src/ipa/rkisp1/algorithms/compress.cpp        |   3 +-
 src/ipa/rkisp1/algorithms/compress.h          |   3 +-
 src/ipa/rkisp1/algorithms/cproc.cpp           |   5 +-
 src/ipa/rkisp1/algorithms/cproc.h             |   3 +-
 src/ipa/rkisp1/algorithms/dpcc.cpp            |   7 +-
 src/ipa/rkisp1/algorithms/dpcc.h              |   3 +-
 src/ipa/rkisp1/algorithms/dpf.cpp             |   7 +-
 src/ipa/rkisp1/algorithms/dpf.h               |   3 +-
 src/ipa/rkisp1/algorithms/filter.cpp          |   5 +-
 src/ipa/rkisp1/algorithms/filter.h            |   3 +-
 src/ipa/rkisp1/algorithms/goc.cpp             |   5 +-
 src/ipa/rkisp1/algorithms/goc.h               |   3 +-
 src/ipa/rkisp1/algorithms/gsl.cpp             |   7 +-
 src/ipa/rkisp1/algorithms/gsl.h               |   3 +-
 src/ipa/rkisp1/algorithms/lsc.cpp             |   5 +-
 src/ipa/rkisp1/algorithms/lsc.h               |   3 +-
 src/ipa/rkisp1/algorithms/lux.cpp             |   3 +-
 src/ipa/rkisp1/algorithms/lux.h               |   3 +-
 src/ipa/rkisp1/algorithms/wdr.cpp             |   3 +-
 src/ipa/rkisp1/algorithms/wdr.h               |   3 +-
 src/ipa/rkisp1/rkisp1.cpp                     | 151 ++-
 src/ipa/softisp/algorithms/adjust.cpp         |   3 +-
 src/ipa/softisp/algorithms/adjust.h           |   3 +-
 src/ipa/softisp/algorithms/agc.cpp            |   6 +-
 src/ipa/softisp/algorithms/agc.h              |   3 +-
 src/ipa/softisp/algorithms/awb.cpp            |   3 +-
 src/ipa/softisp/algorithms/awb.h              |   3 +-
 src/ipa/softisp/algorithms/blc.cpp            |   3 +-
 src/ipa/softisp/algorithms/blc.h              |   3 +-
 src/ipa/softisp/algorithms/ccm.cpp            |   4 +-
 src/ipa/softisp/algorithms/ccm.h              |   3 +-
 src/ipa/softisp/softisp.cpp                   |  30 +-
 src/libcamera/buffer_queue.cpp                | 541 +++++++++++
 src/libcamera/delayed_controls.cpp            |  89 +-
 src/libcamera/meson.build                     |   2 +
 src/libcamera/pipeline/ipu3/ipu3.cpp          |   2 +-
 src/libcamera/pipeline/mali-c55/mali-c55.cpp  |   6 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 891 +++++++++++-------
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  42 +-
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  23 +-
 src/libcamera/pipeline/simple/simple.cpp      |  15 +-
 src/libcamera/sequence_sync_helper.cpp        | 113 +++
 src/libcamera/v4l2_videodevice.cpp            |  15 -
 test/delayed_controls.cpp                     |  57 +-
 89 files changed, 2081 insertions(+), 744 deletions(-)
 create mode 100644 include/libcamera/internal/buffer_queue.h
 create mode 100644 include/libcamera/internal/sequence_sync_helper.h
 create mode 100644 src/libcamera/buffer_queue.cpp
 create mode 100644 src/libcamera/sequence_sync_helper.cpp