[libcamera-devel] [PATCH v7 12/12] libcamera: pipeline, ipa: ipu3: Support the new IPC mechanism
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Feb 12 03:36:40 CET 2021
Hi Paul and Niklas,
(and CC'ing Niklas and Kieran)
Thank you for the patch.
On Thu, Feb 11, 2021 at 04:18:46PM +0900, Paul Elder wrote:
> From: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> Add support to ipu3 pipeline handler and IPA for the new IPC mechanism.
>
> [Original version]
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> [Fixed commit message and small changes]
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> New in v7
> - was "ipu3: Translate IPA protocol to new mojo interface"
> - remove include core_ipa_interface.h from pipeline ipu3.cpp
> - change commit message
> - move removal of include ipa_interface_wrapper.h to "libcamera:
> IPAInterface: Replace C API with the new C++-only API"
> ---
> include/libcamera/ipa/ipu3.h | 23 -------
> include/libcamera/ipa/ipu3.mojom | 43 +++++++++++++
> include/libcamera/ipa/meson.build | 1 +
> src/ipa/ipu3/ipu3.cpp | 72 ++++++++-------------
> src/ipa/ipu3/meson.build | 6 +-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 95 +++++++++++-----------------
> 6 files changed, 112 insertions(+), 128 deletions(-)
> delete mode 100644 include/libcamera/ipa/ipu3.h
> create mode 100644 include/libcamera/ipa/ipu3.mojom
>
> diff --git a/include/libcamera/ipa/ipu3.h b/include/libcamera/ipa/ipu3.h
> deleted file mode 100644
> index cbaaef04..00000000
> --- a/include/libcamera/ipa/ipu3.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2020, Google Inc.
> - *
> - * ipu3.h - Image Processing Algorithm interface for IPU3
> - */
> -#ifndef __LIBCAMERA_IPA_INTERFACE_IPU3_H__
> -#define __LIBCAMERA_IPA_INTERFACE_IPU3_H__
> -
> -#ifndef __DOXYGEN__
> -
> -enum IPU3Operations {
> - IPU3_IPA_ACTION_SET_SENSOR_CONTROLS = 1,
> - IPU3_IPA_ACTION_PARAM_FILLED = 2,
> - IPU3_IPA_ACTION_METADATA_READY = 3,
> - IPU3_IPA_EVENT_PROCESS_CONTROLS = 4,
> - IPU3_IPA_EVENT_STAT_READY = 5,
> - IPU3_IPA_EVENT_FILL_PARAMS = 6,
> -};
> -
> -#endif /* __DOXYGEN__ */
> -
> -#endif /* __LIBCAMERA_IPA_INTERFACE_IPU3_H__ */
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> new file mode 100644
> index 00000000..6ee11333
> --- /dev/null
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +module ipa.ipu3;
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +enum IPU3Operations {
> + ActionSetSensorControls = 1,
> + ActionParamFilled = 2,
> + ActionMetadataReady = 3,
> + EventProcessControls = 4,
> + EventStatReady = 5,
> + EventFillParams = 6,
> +};
> +
> +struct IPU3Event {
> + IPU3Operations op;
> + uint32 frame;
> + uint32 bufferId;
> + ControlList controls;
> +};
> +
> +struct IPU3Action {
> + IPU3Operations op;
> + ControlList controls;
> +};
> +
> +interface IPAIPU3Interface {
> + init(IPASettings settings) => (int32 ret);
> + start() => (int32 ret);
> + stop();
> +
> + configure(map<uint32, ControlInfoMap> entityControls) => ();
> +
> + mapBuffers(array<IPABuffer> buffers);
> + unmapBuffers(array<uint32> ids);
> +
> + [async] processEvent(IPU3Event ev);
> +};
> +
> +interface IPAIPU3EventInterface {
> + queueFrameAction(uint32 frame, IPU3Action action);
> +};
This looks fine, but should eventually receive the same kind of brush up
as the RPi IPA protocol. It's fine to do so later, but the conversion
should happen either before implementing the IPU3 IPA, or as it gets
implemented. We shouldn't start using (and especially extending) the old
API.
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index d701bccc..fe8aa65b 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -55,6 +55,7 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',
> ])
>
> ipa_mojom_files = [
> + 'ipu3.mojom',
> 'raspberrypi.mojom',
> 'rkisp1.mojom',
> 'vimc.mojom',
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index d5bde223..fcd8889c 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -5,8 +5,6 @@
> * ipu3.cpp - IPU3 Image Processing Algorithms
> */
>
> -#include <libcamera/ipa/ipu3.h>
> -
> #include <stdint.h>
> #include <sys/mman.h>
>
> @@ -17,6 +15,7 @@
> #include <libcamera/control_ids.h>
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
> #include <libcamera/request.h>
>
> #include "libcamera/internal/buffer.h"
> @@ -26,25 +25,21 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(IPAIPU3)
>
> -class IPAIPU3 : public IPAInterface
> +class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
> {
> public:
> int init([[maybe_unused]] const IPASettings &settings) override
> {
> return 0;
> }
> - int start([[maybe_unused]] const IPAOperationData &data,
> - [[maybe_unused]] IPAOperationData *result) override { return 0; }
> + int start() override { return 0; }
> void stop() override {}
>
> - void configure(const CameraSensorInfo &info,
> - const std::map<unsigned int, IPAStream> &streamConfig,
> - const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> - const IPAOperationData &ipaConfig,
> - IPAOperationData *response) override;
> + void configure(const std::map<uint32_t, ControlInfoMap> &entityControls) override;
> +
> void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> void unmapBuffers(const std::vector<unsigned int> &ids) override;
> - void processEvent(const IPAOperationData &event) override;
> + void processEvent(const ipa::ipu3::IPU3Event &event) override;
>
> private:
> void processControls(unsigned int frame, const ControlList &controls);
> @@ -67,11 +62,7 @@ private:
> uint32_t maxGain_;
> };
>
> -void IPAIPU3::configure([[maybe_unused]] const CameraSensorInfo &info,
> - [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> - const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> - [[maybe_unused]] const IPAOperationData &ipaConfig,
> - [[maybe_unused]] IPAOperationData *result)
> +void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
> {
> if (entityControls.empty())
> return;
> @@ -121,19 +112,15 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids)
> }
> }
>
> -void IPAIPU3::processEvent(const IPAOperationData &event)
> +void IPAIPU3::processEvent(const ipa::ipu3::IPU3Event &event)
> {
> - switch (event.operation) {
> - case IPU3_IPA_EVENT_PROCESS_CONTROLS: {
> - unsigned int frame = event.data[0];
> - processControls(frame, event.controls[0]);
> + switch (event.op) {
> + case ipa::ipu3::EventProcessControls: {
> + processControls(event.frame, event.controls);
> break;
> }
> - case IPU3_IPA_EVENT_STAT_READY: {
> - unsigned int frame = event.data[0];
> - unsigned int bufferId = event.data[1];
> -
> - auto it = buffers_.find(bufferId);
> + case ipa::ipu3::EventStatReady: {
> + auto it = buffers_.find(event.bufferId);
> if (it == buffers_.end()) {
> LOG(IPAIPU3, Error) << "Could not find stats buffer!";
> return;
> @@ -143,14 +130,11 @@ void IPAIPU3::processEvent(const IPAOperationData &event)
> const ipu3_uapi_stats_3a *stats =
> reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>
> - parseStatistics(frame, stats);
> + parseStatistics(event.frame, stats);
> break;
> }
> - case IPU3_IPA_EVENT_FILL_PARAMS: {
> - unsigned int frame = event.data[0];
> - unsigned int bufferId = event.data[1];
> -
> - auto it = buffers_.find(bufferId);
> + case ipa::ipu3::EventFillParams: {
> + auto it = buffers_.find(event.bufferId);
> if (it == buffers_.end()) {
> LOG(IPAIPU3, Error) << "Could not find param buffer!";
> return;
> @@ -160,11 +144,11 @@ void IPAIPU3::processEvent(const IPAOperationData &event)
> ipu3_uapi_params *params =
> reinterpret_cast<ipu3_uapi_params *>(mem.data());
>
> - fillParams(frame, params);
> + fillParams(event.frame, params);
> break;
> }
> default:
> - LOG(IPAIPU3, Error) << "Unknown event " << event.operation;
> + LOG(IPAIPU3, Error) << "Unknown event " << event.op;
> break;
> }
> }
> @@ -182,8 +166,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>
> /* \todo Fill in parameters buffer. */
>
> - IPAOperationData op;
> - op.operation = IPU3_IPA_ACTION_PARAM_FILLED;
> + ipa::ipu3::IPU3Action op;
> + op.op = ipa::ipu3::ActionParamFilled;
>
> queueFrameAction.emit(frame, op);
>
> @@ -199,22 +183,22 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> /* \todo React to statistics and update internal state machine. */
> /* \todo Add meta-data information to ctrls. */
>
> - IPAOperationData op;
> - op.operation = IPU3_IPA_ACTION_METADATA_READY;
> - op.controls.push_back(ctrls);
> + ipa::ipu3::IPU3Action op;
> + op.op = ipa::ipu3::ActionMetadataReady;
> + op.controls = ctrls;
>
> queueFrameAction.emit(frame, op);
> }
>
> void IPAIPU3::setControls(unsigned int frame)
> {
> - IPAOperationData op;
> - op.operation = IPU3_IPA_ACTION_SET_SENSOR_CONTROLS;
> + ipa::ipu3::IPU3Action op;
> + op.op = ipa::ipu3::ActionSetSensorControls;
>
> ControlList ctrls(ctrls_);
> ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> - op.controls.push_back(ctrls);
> + op.controls = ctrls;
>
> queueFrameAction.emit(frame, op);
> }
> @@ -231,9 +215,9 @@ const struct IPAModuleInfo ipaModuleInfo = {
> "ipu3",
> };
>
> -struct ipa_context *ipaCreate()
> +IPAInterface *ipaCreate()
> {
> - return new IPAInterfaceWrapper(std::make_unique<IPAIPU3>());
> + return new IPAIPU3();
> }
> }
>
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index 444c8245..1ced00ae 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -3,10 +3,10 @@
> ipa_name = 'ipa_ipu3'
>
> mod = shared_module(ipa_name,
> - 'ipu3.cpp',
> + ['ipu3.cpp', libcamera_generated_ipa_headers],
> name_prefix : '',
> - include_directories : [ ipa_includes, libipa_includes ],
> - dependencies : [ libatomic, libcamera_dep ],
> + include_directories : [ipa_includes, libipa_includes],
> + dependencies : libcamera_dep,
> link_with : libipa,
> install : true,
> install_dir : ipa_install_dir)
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9bc3df33..5c010d05 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -14,7 +14,8 @@
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> -#include <libcamera/ipa/ipu3.h>
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
> +#include <libcamera/ipa/ipu3_ipa_proxy.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> @@ -77,8 +78,11 @@ public:
> std::unique_ptr<DelayedControls> delayedCtrls_;
> IPU3Frames frameInfos_;
>
> + std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_;
> +
> private:
> - void queueFrameAction(unsigned int id, const IPAOperationData &op);
> + void queueFrameAction(unsigned int id,
> + const ipa::ipu3::IPU3Action &action);
> };
>
> class IPU3CameraConfiguration : public CameraConfiguration
> @@ -609,18 +613,14 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>
> for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> buffer->setCookie(ipaBufferId++);
> - ipaBuffers_.push_back({
> - .id = buffer->cookie(),
> - .planes = buffer->planes()
> - });
> + ipaBuffers_.push_back(IPABuffer(buffer->cookie(),
> + buffer->planes()));
ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> }
>
> for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
> buffer->setCookie(ipaBufferId++);
> - ipaBuffers_.push_back({
> - .id = buffer->cookie(),
> - .planes = buffer->planes()
> - });
> + ipaBuffers_.push_back(IPABuffer(buffer->cookie(),
> + buffer->planes()));
Same here.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> }
>
> data->ipa_->mapBuffers(ipaBuffers_);
> @@ -650,16 +650,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>
> int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> {
> + std::map<uint32_t, ControlInfoMap> entityControls;
> IPU3CameraData *data = cameraData(camera);
> CIO2Device *cio2 = &data->cio2_;
> ImgUDevice *imgu = data->imgu_;
> -
> - CameraSensorInfo sensorInfo = {};
> - std::map<unsigned int, IPAStream> streamConfig;
> - std::map<unsigned int, const ControlInfoMap &> entityControls;
> - IPAOperationData ipaConfig;
> - IPAOperationData result = {};
> -
> int ret;
>
> /* Allocate buffers for internal pipeline usage. */
> @@ -667,8 +661,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
> if (ret)
> return ret;
>
> - IPAOperationData ipaData = {};
> - ret = data->ipa_->start(ipaData, nullptr);
> + ret = data->ipa_->start();
> if (ret)
> goto error;
>
> @@ -684,24 +677,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con
> if (ret)
> goto error;
>
> - /* Inform IPA of stream configuration and sensor controls. */
> - ret = data->cio2_.sensor()->sensorInfo(&sensorInfo);
> - if (ret)
> - goto error;
> -
> - streamConfig[0] = {
> - .pixelFormat = data->outStream_.configuration().pixelFormat,
> - .size = data->outStream_.configuration().size,
> - };
> - streamConfig[1] = {
> - .pixelFormat = data->vfStream_.configuration().pixelFormat,
> - .size = data->vfStream_.configuration().size,
> - };
> -
> entityControls.emplace(0, data->cio2_.sensor()->controls());
> -
> - data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> - ipaConfig, &result);
> + data->ipa_->configure(entityControls);
>
> return 0;
>
> @@ -751,11 +728,11 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>
> info->rawBuffer = rawBuffer;
>
> - IPAOperationData op;
> - op.operation = IPU3_IPA_EVENT_PROCESS_CONTROLS;
> - op.data = { info->id };
> - op.controls = { request->controls() };
> - data->ipa_->processEvent(op);
> + ipa::ipu3::IPU3Event ev;
> + ev.op = ipa::ipu3::EventProcessControls;
> + ev.frame = info->id;
> + ev.controls = request->controls();
> + data->ipa_->processEvent(ev);
>
> return 0;
> }
> @@ -1048,7 +1025,7 @@ int PipelineHandlerIPU3::registerCameras()
>
> int IPU3CameraData::loadIPA()
> {
> - ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> + ipa_ = IPAManager::createIPA<ipa::ipu3::IPAProxyIPU3>(pipe_, 1, 1);
> if (!ipa_)
> return -ENOENT;
>
> @@ -1060,15 +1037,15 @@ int IPU3CameraData::loadIPA()
> }
>
> void IPU3CameraData::queueFrameAction(unsigned int id,
> - const IPAOperationData &action)
> + const ipa::ipu3::IPU3Action &action)
> {
> - switch (action.operation) {
> - case IPU3_IPA_ACTION_SET_SENSOR_CONTROLS: {
> - const ControlList &controls = action.controls[0];
> + switch (action.op) {
> + case ipa::ipu3::ActionSetSensorControls: {
> + const ControlList &controls = action.controls;
> delayedCtrls_->push(controls);
> break;
> }
> - case IPU3_IPA_ACTION_PARAM_FILLED: {
> + case ipa::ipu3::ActionParamFilled: {
> IPU3Frames::Info *info = frameInfos_.find(id);
> if (!info)
> break;
> @@ -1090,13 +1067,13 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>
> break;
> }
> - case IPU3_IPA_ACTION_METADATA_READY: {
> + case ipa::ipu3::ActionMetadataReady: {
> IPU3Frames::Info *info = frameInfos_.find(id);
> if (!info)
> break;
>
> Request *request = info->request;
> - request->metadata() = action.controls[0];
> + request->metadata() = action.controls;
> info->metadataProcessed = true;
> if (frameInfos_.tryComplete(info))
> pipe_->completeRequest(request);
> @@ -1104,7 +1081,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> break;
> }
> default:
> - LOG(IPU3, Error) << "Unknown action " << action.operation;
> + LOG(IPU3, Error) << "Unknown action " << action.op;
> break;
> }
> }
> @@ -1172,10 +1149,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> if (request->findBuffer(&rawStream_))
> pipe_->completeBuffer(request, buffer);
>
> - IPAOperationData op;
> - op.operation = IPU3_IPA_EVENT_FILL_PARAMS;
> - op.data = { info->id, info->paramBuffer->cookie() };
> - ipa_->processEvent(op);
> + ipa::ipu3::IPU3Event ev;
> + ev.op = ipa::ipu3::EventFillParams;
> + ev.frame = info->id;
> + ev.bufferId = info->paramBuffer->cookie();
> + ipa_->processEvent(ev);
> }
>
> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> @@ -1202,10 +1180,11 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> return;
> }
>
> - IPAOperationData op;
> - op.operation = IPU3_IPA_EVENT_STAT_READY;
> - op.data = { info->id, info->statBuffer->cookie() };
> - ipa_->processEvent(op);
> + ipa::ipu3::IPU3Event ev;
> + ev.op = ipa::ipu3::EventStatReady;
> + ev.frame = info->id;
> + ev.bufferId = info->statBuffer->cookie();
> + ipa_->processEvent(ev);
> }
>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list