[libcamera-devel,v1,0/9] Support passing custom data to IPA configure()

8484 mbox series

The IPAInterface::configure() function passes miscellaneous
configuration data to the IPA, but offers no way for pipeline handlers
to pass custom data, or to receive custom data back. This has led to the
Raspberry Pi IPA protocol using custom frame actions and events to pass
initialization data to the IPA and to receive initial sensor
configuration back. To support this, the Raspberry Pi IPA needs to start
the IPA thread before configure() time, while it was meant to be started
when starting the camera.

This patch series addresses this issue by adding a way to pass custom
configuration data to the IPA and receive data back. It starts with two
small drive-by cleanups in patches 1/9 and 2/9. Patch 3/9 performs the
bulk of the work by modifying IPAInterface::configure(). The C API isn't
addressed yet, as larger refactoring is considered to handle the C <->
C++ translation in a better way.

Patch 4/9 is another small drive-by fix. Patch 5/9 follows by moving
code around in the Raspberry Pi pipeline handler to prepare for the
upcoming changes, and patches 6/9 and 7/9 replace custom events and
frame actions with the new IPA-specific data passed to configure().
Patch 8/9 moves the IPA start and stop to the camera start and stop,
aligning the implementation with how the IPAInterface API is meant to be
used. Patch 9/9 is an additional simplification made possible by the
rest of the series.

I'm not entirely happy with patch 7/9 as it handles the data received
from the IPA manually. The Raspberry Pi IPA can send both staggered
write configuration data and sensor gain and exposure controls, with
both set of parameters being optional. Better code is needed to
construct a protocol on top of the IPAOperationData passed to
IPAInterface::configure(). This also applies to processEvent and
queueFrameAction, and I think we should investigate usage of something
similar to protobuf to handle both the IPA C <-> C++ translation and the
IPA protocol built on top of IPAOperationData. Clever ideas are welcome,
but this will hopefully not be a blocker for this patch series.

Naush, I'm not entirely sure about 9/9, could you confirm that
RPI_IPA_ACTION_V4L2_SET_ISP never occurs in the stop state ?

An additional improvement that I believe could be done on top of this
would be to merge the RPI_IPA_ACTION_RUN_ISP,
RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME and RPI_IPA_ACTION_V4L2_SET_ISP
actions. The first two are essentially the same with an option, which
could be passed to IPAOperationData::data, and the last one carries a
set of controls that could be passed in one go.
RPI_IPA_ACTION_EMBEDDED_COMPLETE could also be merged, as it's always
sent with RPI_IPA_ACTION_RUN_ISP(_AND_DROP_FRAME), but there may be a
future use case to keep it separate that I can't think about now.

Similarly, RPI_IPA_ACTION_V4L2_SET_STAGGERED and
RPI_IPA_ACTION_STATS_METADATA_COMPLETE could also be merged into a
single action.

This would lower the number of cross-thread calls, improving
performances.

Laurent Pinchart (9):

  libcamera: ipa: Document the parameters of the IPA C configure
    function
  libcamera: ipa_context_wrapper: Fix bad copy&paste in comment
  libcamera: ipa_interface: Add support for custom IPA data to
    configure()
  libcamera: pipeline: raspberrypi: Constify parameter to
    StaggeredCtrl::set()
  libcamera: pipeline: raspberrypi: Move configureIPA() to RPiCameraData
  ipa: raspberrypi: Pass lens shading table through configure() function
  ipa: raspberrypi: Pass sensor config back from configure()
  libcamera: pipeline: raspberrypi: Start IPA when starting camera
  libcamera: pipeline: raspberrypi: Don't handle any actions in stop
    state

 .../libcamera/internal/ipa_context_wrapper.h  |   4 +-
 include/libcamera/ipa/ipa_interface.h         |   4 +-
 include/libcamera/ipa/raspberrypi.h           |   8 +-
 src/ipa/libipa/ipa_interface_wrapper.cpp      |   5 +-
 src/ipa/raspberrypi/raspberrypi.cpp           |  64 +++---
 src/ipa/rkisp1/rkisp1.cpp                     |   8 +-
 src/ipa/vimc/vimc.cpp                         |   4 +-
 src/libcamera/ipa_context_wrapper.cpp         |  10 +-
 src/libcamera/ipa_interface.cpp               |  12 +
 .../pipeline/raspberrypi/raspberrypi.cpp      | 205 +++++++++---------
 .../pipeline/raspberrypi/staggered_ctrl.cpp   |   2 +-
 .../pipeline/raspberrypi/staggered_ctrl.h     |   2 +-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   4 +-
 src/libcamera/proxy/ipa_proxy_linux.cpp       |   4 +-
 src/libcamera/proxy/ipa_proxy_thread.cpp      |  11 +-
 test/ipa/ipa_wrappers_test.cpp                |   8 +-
 16 files changed, 207 insertions(+), 148 deletions(-)