[libcamera-devel,v2,00/12] Support passing custom data to IPA configure()

8604 mbox series

Hello,

This patch series is the second version of an improvement to the
IPAInterface::configure() function to support passing IPA-specific data.

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 01/12 and 02/12. Patch 03/12 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 04/12 and 05/12 are other small drive-by fixes. Patch 06/12
follows by moving code around in the Raspberry Pi pipeline handler to
prepare for the upcoming changes, and patch 07/12 slightly simplifies
frame action handling.

Patches 08/12 and 09/12 are the main changes in v2, and move sensor
orientation handling from the Raspberry Pi IPA to the pipeline handler,
based on the rotation reported through the device tree. The
corresponding kernel patches have been posted as "[PATCH 00/12]
raspberrypi: Report sensor orientation through DT". This change
simplifies the Raspberry Pi IPA API, addressing a concern on the
remaining patches from v1.

Patches 10/12 and 11/12 replace custom events and frame actions with the
new IPA-specific data passed to configure(). Patch 12/12 finally 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 11/12 is now cleaner, although the concern I had in v1 are still
theoretically valid. 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.

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. Those two changes would lower the number of cross-thread
calls, improving performances.

The code has been tested with the imx219 camera module.

Laurent Pinchart (12):
  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: Drop unused local variable
  libcamera: pipeline: raspberrypi: Constify parameter to
    StaggeredCtrl::set()
  libcamera: pipeline: raspberrypi: Move configureIPA() to RPiCameraData
  libcamera: pipeline: raspberrypi: Don't call handleState() in stop
    state
  libcamera: pipeline: raspberrypi: Set sensor flip based on rotation
  ipa: raspberrypi: Drop CameraHelper::GetOrientation()
  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/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/cam_helper.cpp            |   6 -
 src/ipa/raspberrypi/cam_helper.hpp            |   7 -
 src/ipa/raspberrypi/cam_helper_imx219.cpp     |   7 -
 src/ipa/raspberrypi/cam_helper_imx477.cpp     |   7 -
 src/ipa/raspberrypi/raspberrypi.cpp           |  68 +++---
 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      | 213 +++++++++---------
 .../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 +-
 20 files changed, 214 insertions(+), 180 deletions(-)