| Message ID | 20210211071846.35161-1-paul.elder@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi, Am 11.02.21 um 08:18 schrieb Paul Elder: > Add support to the rkisp1 pipeline handler and IPA for the new IPC > mechanism. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > No change in v7 > > Changes in v6: > - move the enum and const from the header to the mojom file > - remove the per-pipeline ControlInfoMap > - since rkisp1.h has nothing in it anymore, remove it > - use the namespace in the pipeline handler and IPA > > Changes in v5: > - import the generated header with the new file name > > Changes in v4: > - rename Controls to controls > - rename IPARkISP1CallbackInterface to IPARkISP1EventInterface > - rename libcamera_generated_headers to libcamera_generated_ipa_headers > in meson > - use the new header name, rkisp1_ipa_interface.h > > Changes in v3: > - change namespacing of base ControlInfoMap structure > > New in v2 > --- > include/libcamera/ipa/meson.build | 1 + > include/libcamera/ipa/rkisp1.h | 22 ------- > include/libcamera/ipa/rkisp1.mojom | 44 +++++++++++++ > src/ipa/rkisp1/meson.build | 2 +- > src/ipa/rkisp1/rkisp1.cpp | 61 +++++++++--------- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 ++++++++++++------------ > 6 files changed, 115 insertions(+), 94 deletions(-) > delete mode 100644 include/libcamera/ipa/rkisp1.h > create mode 100644 include/libcamera/ipa/rkisp1.mojom > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > index 67e3f049..d701bccc 100644 > --- a/include/libcamera/ipa/meson.build > +++ b/include/libcamera/ipa/meson.build > @@ -56,6 +56,7 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h', > > ipa_mojom_files = [ > 'raspberrypi.mojom', > + 'rkisp1.mojom', > 'vimc.mojom', > ] > > diff --git a/include/libcamera/ipa/rkisp1.h b/include/libcamera/ipa/rkisp1.h > deleted file mode 100644 > index bb824f29..00000000 > --- a/include/libcamera/ipa/rkisp1.h > +++ /dev/null > @@ -1,22 +0,0 @@ > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > -/* > - * Copyright (C) 2019, Google Inc. > - * > - * rkisp1.h - Image Processing Algorithm interface for RkISP1 > - */ > -#ifndef __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ > -#define __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ > - > -#ifndef __DOXYGEN__ > - > -enum RkISP1Operations { > - RKISP1_IPA_ACTION_V4L2_SET = 1, > - RKISP1_IPA_ACTION_PARAM_FILLED = 2, > - RKISP1_IPA_ACTION_METADATA = 3, > - RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER = 4, > - RKISP1_IPA_EVENT_QUEUE_REQUEST = 5, > -}; > - > -#endif /* __DOXYGEN__ */ > - > -#endif /* __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ */ > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > new file mode 100644 > index 00000000..9270f9c7 > --- /dev/null > +++ b/include/libcamera/ipa/rkisp1.mojom > @@ -0,0 +1,44 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > + > +module ipa.rkisp1; > + > +import "include/libcamera/ipa/core.mojom"; > + > +enum RkISP1Operations { > + ActionV4L2Set = 1, > + ActionParamFilled = 2, > + ActionMetadata = 3, > + EventSignalStatBuffer = 4, > + EventQueueRequest = 5, > +}; > + > +struct RkISP1Event { > + RkISP1Operations op; > + uint32 frame; > + uint32 bufferId; > + ControlList controls; > +}; > + > +struct RkISP1Action { > + RkISP1Operations op; > + ControlList controls; > +}; > + > +interface IPARkISP1Interface { > + init(IPASettings settings) => (int32 ret); > + start() => (int32 ret); > + stop(); > + > + configure(CameraSensorInfo sensorInfo, > + map<uint32, IPAStream> streamConfig, > + map<uint32, ControlInfoMap> entityControls) => (); > + > + mapBuffers(array<IPABuffer> buffers); > + unmapBuffers(array<uint32> ids); > + > + [async] processEvent(RkISP1Event ev); > +}; > + > +interface IPARkISP1EventInterface { > + queueFrameAction(uint32 frame, RkISP1Action action); > +}; > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build > index ed9a6b6b..3061d53c 100644 > --- a/src/ipa/rkisp1/meson.build > +++ b/src/ipa/rkisp1/meson.build > @@ -3,7 +3,7 @@ > ipa_name = 'ipa_rkisp1' > > mod = shared_module(ipa_name, > - 'rkisp1.cpp', > + ['rkisp1.cpp', libcamera_generated_ipa_headers], > name_prefix : '', > include_directories : [ipa_includes, libipa_includes], > dependencies : libcamera_dep, > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index f4812d8d..67bac986 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -19,7 +19,7 @@ > #include <libcamera/control_ids.h> > #include <libcamera/ipa/ipa_interface.h> > #include <libcamera/ipa/ipa_module_info.h> > -#include <libcamera/ipa/rkisp1.h> > +#include <libcamera/ipa/rkisp1_ipa_interface.h> > #include <libcamera/request.h> > > #include "libcamera/internal/log.h" > @@ -28,25 +28,22 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPARkISP1) > > -class IPARkISP1 : public IPAInterface > +class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface > { > 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; > + const std::map<uint32_t, IPAStream> &streamConfig, > + 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::rkisp1::RkISP1Event &event) override; > > private: > void queueRequest(unsigned int frame, rkisp1_params_cfg *params, > @@ -79,10 +76,8 @@ private: > * before accessing them. > */ > void IPARkISP1::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) > + [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig, > + const std::map<uint32_t, ControlInfoMap> &entityControls) > { > if (entityControls.empty()) > return; > @@ -158,12 +153,12 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > } > } > > -void IPARkISP1::processEvent(const IPAOperationData &event) > +void IPARkISP1::processEvent(const ipa::rkisp1::RkISP1Event &event) > { > - switch (event.operation) { > - case RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER: { > - unsigned int frame = event.data[0]; > - unsigned int bufferId = event.data[1]; > + switch (event.op) { > + case ipa::rkisp1::EventSignalStatBuffer: { > + unsigned int frame = event.frame; > + unsigned int bufferId = event.bufferId; > > const rkisp1_stat_buffer *stats = > static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); > @@ -171,18 +166,18 @@ void IPARkISP1::processEvent(const IPAOperationData &event) > updateStatistics(frame, stats); > break; > } > - case RKISP1_IPA_EVENT_QUEUE_REQUEST: { > - unsigned int frame = event.data[0]; > - unsigned int bufferId = event.data[1]; > + case ipa::rkisp1::EventQueueRequest: { > + unsigned int frame = event.frame; > + unsigned int bufferId = event.bufferId; > > rkisp1_params_cfg *params = > static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); > > - queueRequest(frame, params, event.controls[0]); > + queueRequest(frame, params, event.controls); > break; > } > default: > - LOG(IPARkISP1, Error) << "Unknown event " << event.operation; > + LOG(IPARkISP1, Error) << "Unknown event " << event.op; > break; > } > } > @@ -202,8 +197,8 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params, > params->module_en_update = RKISP1_CIF_ISP_MODULE_AEC; > } > > - IPAOperationData op; > - op.operation = RKISP1_IPA_ACTION_PARAM_FILLED; > + ipa::rkisp1::RkISP1Action op; > + op.op = ipa::rkisp1::ActionParamFilled; > > queueFrameAction.emit(frame, op); > } > @@ -255,13 +250,13 @@ void IPARkISP1::updateStatistics(unsigned int frame, > > void IPARkISP1::setControls(unsigned int frame) > { > - IPAOperationData op; > - op.operation = RKISP1_IPA_ACTION_V4L2_SET; > + ipa::rkisp1::RkISP1Action op; > + op.op = ipa::rkisp1::ActionV4L2Set; > > 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); > } > @@ -273,9 +268,9 @@ void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState) > if (aeState) > ctrls.set(controls::AeLocked, aeState == 2); > > - IPAOperationData op; > - op.operation = RKISP1_IPA_ACTION_METADATA; > - op.controls.push_back(ctrls); > + ipa::rkisp1::RkISP1Action op; > + op.op = ipa::rkisp1::ActionMetadata; > + op.controls = ctrls; > > queueFrameAction.emit(frame, op); > } > @@ -292,9 +287,9 @@ const struct IPAModuleInfo ipaModuleInfo = { > "rkisp1", > }; > > -struct ipa_context *ipaCreate() > +IPAInterface *ipaCreate() > { > - return new IPAInterfaceWrapper(std::make_unique<IPARkISP1>()); > + return new IPARkISP1(); > } > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index e85979a7..23c95100 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -18,7 +18,9 @@ > #include <libcamera/camera.h> > #include <libcamera/control_ids.h> > #include <libcamera/formats.h> > -#include <libcamera/ipa/rkisp1.h> > +#include <libcamera/ipa/core_ipa_interface.h> > +#include <libcamera/ipa/rkisp1_ipa_interface.h> > +#include <libcamera/ipa/rkisp1_ipa_proxy.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > @@ -96,9 +98,11 @@ public: > RkISP1MainPath *mainPath_; > RkISP1SelfPath *selfPath_; > > + std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_; > + > private: > void queueFrameAction(unsigned int frame, > - const IPAOperationData &action); > + const ipa::rkisp1::RkISP1Action &action); > > void metadataReady(unsigned int frame, const ControlList &metadata); > }; > @@ -298,7 +302,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) > > int RkISP1CameraData::loadIPA() > { > - ipa_ = IPAManager::createIPA(pipe_, 1, 1); > + ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); > if (!ipa_) > return -ENOENT; > > @@ -311,15 +315,15 @@ int RkISP1CameraData::loadIPA() > } > > void RkISP1CameraData::queueFrameAction(unsigned int frame, > - const IPAOperationData &action) > + const ipa::rkisp1::RkISP1Action &action) > { > - switch (action.operation) { > - case RKISP1_IPA_ACTION_V4L2_SET: { > - const ControlList &controls = action.controls[0]; > + switch (action.op) { > + case ipa::rkisp1::ActionV4L2Set: { > + const ControlList &controls = action.controls; > delayedCtrls_->push(controls); > break; > } > - case RKISP1_IPA_ACTION_PARAM_FILLED: { > + case ipa::rkisp1::ActionParamFilled: { > PipelineHandlerRkISP1 *pipe = static_cast<PipelineHandlerRkISP1 *>(pipe_); > RkISP1FrameInfo *info = frameInfo_.find(frame); > if (!info) > @@ -336,11 +340,11 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame, > > break; > } > - case RKISP1_IPA_ACTION_METADATA: > - metadataReady(frame, action.controls[0]); > + case ipa::rkisp1::ActionMetadata: > + metadataReady(frame, action.controls); > break; > default: > - LOG(RkISP1, Error) << "Unknown action " << action.operation; > + LOG(RkISP1, Error) << "Unknown action " << action.op; > break; > } > } > @@ -667,15 +671,15 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > > for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) { > buffer->setCookie(ipaBufferId++); > - data->ipaBuffers_.push_back({ .id = buffer->cookie(), > - .planes = buffer->planes() }); > + data->ipaBuffers_.push_back(IPABuffer(buffer->cookie(), > + buffer->planes())); > availableParamBuffers_.push(buffer.get()); > } > > for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) { > buffer->setCookie(ipaBufferId++); > - data->ipaBuffers_.push_back({ .id = buffer->cookie(), > - .planes = buffer->planes() }); > + data->ipaBuffers_.push_back(IPABuffer(buffer->cookie(), > + buffer->planes())); > availableStatBuffers_.push(buffer.get()); > } > > @@ -729,8 +733,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > if (ret) > return ret; > > - IPAOperationData ipaData = {}; > - ret = data->ipa_->start(ipaData, nullptr); > + ret = data->ipa_->start(); > if (ret) { > freeBuffers(camera); > LOG(RkISP1, Error) > @@ -771,10 +774,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > return ret; > } > > - streamConfig[0] = { > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > - .size = data->mainPathStream_.configuration().size, > - }; > + streamConfig[0] = IPAStream( > + data->mainPathStream_.configuration().pixelFormat, > + data->mainPathStream_.configuration().size > + ); > } > > if (data->selfPath_->isEnabled()) { > @@ -788,10 +791,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > return ret; > } > > - streamConfig[1] = { > - .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > - .size = data->selfPathStream_.configuration().size, > - }; > + streamConfig[1] = IPAStream( > + data->selfPathStream_.configuration().pixelFormat, > + data->selfPathStream_.configuration().size > + ); > } > > isp_->setFrameStartEnabled(true); > @@ -808,12 +811,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > ret = 0; > } > > - std::map<unsigned int, const ControlInfoMap &> entityControls; > + std::map<uint32_t, ControlInfoMap> entityControls; > entityControls.emplace(0, data->sensor_->controls()); > > - IPAOperationData ipaConfig; > - data->ipa_->configure(sensorInfo, streamConfig, entityControls, > - ipaConfig, nullptr); > + data->ipa_->configure(sensorInfo, streamConfig, entityControls); It looks like there is a wrong calling order in rkisp1. The call to data->ipa_->configure is done upon the "start" method of the rkisp1 pipline-handler instead of the "configure" method. This is bug not related to this patchset. But with this patchset, the call to data->ipa_->configure is after the call to data->ipa_->start when only async calls are allowed. I can send a patch fixing that. Thanks, Dafna > > return ret; > } > @@ -855,11 +856,12 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > if (!info) > return -ENOENT; > > - IPAOperationData op; > - op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST; > - op.data = { data->frame_, info->paramBuffer->cookie() }; > - op.controls = { request->controls() }; > - data->ipa_->processEvent(op); > + ipa::rkisp1::RkISP1Event ev; > + ev.op = ipa::rkisp1::EventQueueRequest; > + ev.frame = data->frame_; > + ev.bufferId = info->paramBuffer->cookie(); > + ev.controls = request->controls(); > + data->ipa_->processEvent(ev); > > data->frame_++; > > @@ -1090,10 +1092,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer) > if (data->frame_ <= buffer->metadata().sequence) > data->frame_ = buffer->metadata().sequence + 1; > > - IPAOperationData op; > - op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER; > - op.data = { info->frame, info->statBuffer->cookie() }; > - data->ipa_->processEvent(op); > + ipa::rkisp1::RkISP1Event ev; > + ev.op = ipa::rkisp1::EventSignalStatBuffer; > + ev.frame = info->frame; > + ev.bufferId = info->statBuffer->cookie(); > + data->ipa_->processEvent(ev); > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1) >
Hi Dafna, On Fri, Feb 12, 2021 at 01:32:48PM +0100, Dafna Hirschfeld wrote: > Am 11.02.21 um 08:18 schrieb Paul Elder: > > Add support to the rkisp1 pipeline handler and IPA for the new IPC > > mechanism. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > No change in v7 > > > > Changes in v6: > > - move the enum and const from the header to the mojom file > > - remove the per-pipeline ControlInfoMap > > - since rkisp1.h has nothing in it anymore, remove it > > - use the namespace in the pipeline handler and IPA > > > > Changes in v5: > > - import the generated header with the new file name > > > > Changes in v4: > > - rename Controls to controls > > - rename IPARkISP1CallbackInterface to IPARkISP1EventInterface > > - rename libcamera_generated_headers to libcamera_generated_ipa_headers > > in meson > > - use the new header name, rkisp1_ipa_interface.h > > > > Changes in v3: > > - change namespacing of base ControlInfoMap structure > > > > New in v2 > > --- > > include/libcamera/ipa/meson.build | 1 + > > include/libcamera/ipa/rkisp1.h | 22 ------- > > include/libcamera/ipa/rkisp1.mojom | 44 +++++++++++++ > > src/ipa/rkisp1/meson.build | 2 +- > > src/ipa/rkisp1/rkisp1.cpp | 61 +++++++++--------- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 ++++++++++++------------ > > 6 files changed, 115 insertions(+), 94 deletions(-) > > delete mode 100644 include/libcamera/ipa/rkisp1.h > > create mode 100644 include/libcamera/ipa/rkisp1.mojom > > > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > > index 67e3f049..d701bccc 100644 > > --- a/include/libcamera/ipa/meson.build > > +++ b/include/libcamera/ipa/meson.build > > @@ -56,6 +56,7 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h', > > > > ipa_mojom_files = [ > > 'raspberrypi.mojom', > > + 'rkisp1.mojom', > > 'vimc.mojom', > > ] > > > > diff --git a/include/libcamera/ipa/rkisp1.h b/include/libcamera/ipa/rkisp1.h > > deleted file mode 100644 > > index bb824f29..00000000 > > --- a/include/libcamera/ipa/rkisp1.h > > +++ /dev/null > > @@ -1,22 +0,0 @@ > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > -/* > > - * Copyright (C) 2019, Google Inc. > > - * > > - * rkisp1.h - Image Processing Algorithm interface for RkISP1 > > - */ > > -#ifndef __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ > > -#define __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ > > - > > -#ifndef __DOXYGEN__ > > - > > -enum RkISP1Operations { > > - RKISP1_IPA_ACTION_V4L2_SET = 1, > > - RKISP1_IPA_ACTION_PARAM_FILLED = 2, > > - RKISP1_IPA_ACTION_METADATA = 3, > > - RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER = 4, > > - RKISP1_IPA_EVENT_QUEUE_REQUEST = 5, > > -}; > > - > > -#endif /* __DOXYGEN__ */ > > - > > -#endif /* __LIBCAMERA_IPA_INTERFACE_RKISP1_H__ */ > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > new file mode 100644 > > index 00000000..9270f9c7 > > --- /dev/null > > +++ b/include/libcamera/ipa/rkisp1.mojom > > @@ -0,0 +1,44 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > + > > +module ipa.rkisp1; > > + > > +import "include/libcamera/ipa/core.mojom"; > > + > > +enum RkISP1Operations { > > + ActionV4L2Set = 1, > > + ActionParamFilled = 2, > > + ActionMetadata = 3, > > + EventSignalStatBuffer = 4, > > + EventQueueRequest = 5, > > +}; > > + > > +struct RkISP1Event { > > + RkISP1Operations op; > > + uint32 frame; > > + uint32 bufferId; > > + ControlList controls; > > +}; > > + > > +struct RkISP1Action { > > + RkISP1Operations op; > > + ControlList controls; > > +}; > > + > > +interface IPARkISP1Interface { > > + init(IPASettings settings) => (int32 ret); > > + start() => (int32 ret); > > + stop(); > > + > > + configure(CameraSensorInfo sensorInfo, > > + map<uint32, IPAStream> streamConfig, > > + map<uint32, ControlInfoMap> entityControls) => (); > > + > > + mapBuffers(array<IPABuffer> buffers); > > + unmapBuffers(array<uint32> ids); > > + > > + [async] processEvent(RkISP1Event ev); > > +}; > > + > > +interface IPARkISP1EventInterface { > > + queueFrameAction(uint32 frame, RkISP1Action action); > > +}; > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build > > index ed9a6b6b..3061d53c 100644 > > --- a/src/ipa/rkisp1/meson.build > > +++ b/src/ipa/rkisp1/meson.build > > @@ -3,7 +3,7 @@ > > ipa_name = 'ipa_rkisp1' > > > > mod = shared_module(ipa_name, > > - 'rkisp1.cpp', > > + ['rkisp1.cpp', libcamera_generated_ipa_headers], > > name_prefix : '', > > include_directories : [ipa_includes, libipa_includes], > > dependencies : libcamera_dep, > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index f4812d8d..67bac986 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -19,7 +19,7 @@ > > #include <libcamera/control_ids.h> > > #include <libcamera/ipa/ipa_interface.h> > > #include <libcamera/ipa/ipa_module_info.h> > > -#include <libcamera/ipa/rkisp1.h> > > +#include <libcamera/ipa/rkisp1_ipa_interface.h> > > #include <libcamera/request.h> > > > > #include "libcamera/internal/log.h" > > @@ -28,25 +28,22 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPARkISP1) > > > > -class IPARkISP1 : public IPAInterface > > +class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface > > { > > 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; > > + const std::map<uint32_t, IPAStream> &streamConfig, > > + 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::rkisp1::RkISP1Event &event) override; > > > > private: > > void queueRequest(unsigned int frame, rkisp1_params_cfg *params, > > @@ -79,10 +76,8 @@ private: > > * before accessing them. > > */ > > void IPARkISP1::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) > > + [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig, > > + const std::map<uint32_t, ControlInfoMap> &entityControls) > > { > > if (entityControls.empty()) > > return; > > @@ -158,12 +153,12 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > > } > > } > > > > -void IPARkISP1::processEvent(const IPAOperationData &event) > > +void IPARkISP1::processEvent(const ipa::rkisp1::RkISP1Event &event) > > { > > - switch (event.operation) { > > - case RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER: { > > - unsigned int frame = event.data[0]; > > - unsigned int bufferId = event.data[1]; > > + switch (event.op) { > > + case ipa::rkisp1::EventSignalStatBuffer: { > > + unsigned int frame = event.frame; > > + unsigned int bufferId = event.bufferId; > > > > const rkisp1_stat_buffer *stats = > > static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); > > @@ -171,18 +166,18 @@ void IPARkISP1::processEvent(const IPAOperationData &event) > > updateStatistics(frame, stats); > > break; > > } > > - case RKISP1_IPA_EVENT_QUEUE_REQUEST: { > > - unsigned int frame = event.data[0]; > > - unsigned int bufferId = event.data[1]; > > + case ipa::rkisp1::EventQueueRequest: { > > + unsigned int frame = event.frame; > > + unsigned int bufferId = event.bufferId; > > > > rkisp1_params_cfg *params = > > static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); > > > > - queueRequest(frame, params, event.controls[0]); > > + queueRequest(frame, params, event.controls); > > break; > > } > > default: > > - LOG(IPARkISP1, Error) << "Unknown event " << event.operation; > > + LOG(IPARkISP1, Error) << "Unknown event " << event.op; > > break; > > } > > } > > @@ -202,8 +197,8 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params, > > params->module_en_update = RKISP1_CIF_ISP_MODULE_AEC; > > } > > > > - IPAOperationData op; > > - op.operation = RKISP1_IPA_ACTION_PARAM_FILLED; > > + ipa::rkisp1::RkISP1Action op; > > + op.op = ipa::rkisp1::ActionParamFilled; > > > > queueFrameAction.emit(frame, op); > > } > > @@ -255,13 +250,13 @@ void IPARkISP1::updateStatistics(unsigned int frame, > > > > void IPARkISP1::setControls(unsigned int frame) > > { > > - IPAOperationData op; > > - op.operation = RKISP1_IPA_ACTION_V4L2_SET; > > + ipa::rkisp1::RkISP1Action op; > > + op.op = ipa::rkisp1::ActionV4L2Set; > > > > 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); > > } > > @@ -273,9 +268,9 @@ void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState) > > if (aeState) > > ctrls.set(controls::AeLocked, aeState == 2); > > > > - IPAOperationData op; > > - op.operation = RKISP1_IPA_ACTION_METADATA; > > - op.controls.push_back(ctrls); > > + ipa::rkisp1::RkISP1Action op; > > + op.op = ipa::rkisp1::ActionMetadata; > > + op.controls = ctrls; > > > > queueFrameAction.emit(frame, op); > > } > > @@ -292,9 +287,9 @@ const struct IPAModuleInfo ipaModuleInfo = { > > "rkisp1", > > }; > > > > -struct ipa_context *ipaCreate() > > +IPAInterface *ipaCreate() > > { > > - return new IPAInterfaceWrapper(std::make_unique<IPARkISP1>()); > > + return new IPARkISP1(); > > } > > } > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index e85979a7..23c95100 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -18,7 +18,9 @@ > > #include <libcamera/camera.h> > > #include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > -#include <libcamera/ipa/rkisp1.h> > > +#include <libcamera/ipa/core_ipa_interface.h> > > +#include <libcamera/ipa/rkisp1_ipa_interface.h> > > +#include <libcamera/ipa/rkisp1_ipa_proxy.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > @@ -96,9 +98,11 @@ public: > > RkISP1MainPath *mainPath_; > > RkISP1SelfPath *selfPath_; > > > > + std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_; > > + > > private: > > void queueFrameAction(unsigned int frame, > > - const IPAOperationData &action); > > + const ipa::rkisp1::RkISP1Action &action); > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > }; > > @@ -298,7 +302,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) > > > > int RkISP1CameraData::loadIPA() > > { > > - ipa_ = IPAManager::createIPA(pipe_, 1, 1); > > + ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); > > if (!ipa_) > > return -ENOENT; > > > > @@ -311,15 +315,15 @@ int RkISP1CameraData::loadIPA() > > } > > > > void RkISP1CameraData::queueFrameAction(unsigned int frame, > > - const IPAOperationData &action) > > + const ipa::rkisp1::RkISP1Action &action) > > { > > - switch (action.operation) { > > - case RKISP1_IPA_ACTION_V4L2_SET: { > > - const ControlList &controls = action.controls[0]; > > + switch (action.op) { > > + case ipa::rkisp1::ActionV4L2Set: { > > + const ControlList &controls = action.controls; > > delayedCtrls_->push(controls); > > break; > > } > > - case RKISP1_IPA_ACTION_PARAM_FILLED: { > > + case ipa::rkisp1::ActionParamFilled: { > > PipelineHandlerRkISP1 *pipe = static_cast<PipelineHandlerRkISP1 *>(pipe_); > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > if (!info) > > @@ -336,11 +340,11 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame, > > > > break; > > } > > - case RKISP1_IPA_ACTION_METADATA: > > - metadataReady(frame, action.controls[0]); > > + case ipa::rkisp1::ActionMetadata: > > + metadataReady(frame, action.controls); > > break; > > default: > > - LOG(RkISP1, Error) << "Unknown action " << action.operation; > > + LOG(RkISP1, Error) << "Unknown action " << action.op; > > break; > > } > > } > > @@ -667,15 +671,15 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > > > > for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) { > > buffer->setCookie(ipaBufferId++); > > - data->ipaBuffers_.push_back({ .id = buffer->cookie(), > > - .planes = buffer->planes() }); > > + data->ipaBuffers_.push_back(IPABuffer(buffer->cookie(), > > + buffer->planes())); > > availableParamBuffers_.push(buffer.get()); > > } > > > > for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) { > > buffer->setCookie(ipaBufferId++); > > - data->ipaBuffers_.push_back({ .id = buffer->cookie(), > > - .planes = buffer->planes() }); > > + data->ipaBuffers_.push_back(IPABuffer(buffer->cookie(), > > + buffer->planes())); > > availableStatBuffers_.push(buffer.get()); > > } > > > > @@ -729,8 +733,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > if (ret) > > return ret; > > > > - IPAOperationData ipaData = {}; > > - ret = data->ipa_->start(ipaData, nullptr); > > + ret = data->ipa_->start(); > > if (ret) { > > freeBuffers(camera); > > LOG(RkISP1, Error) > > @@ -771,10 +774,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > return ret; > > } > > > > - streamConfig[0] = { > > - .pixelFormat = data->mainPathStream_.configuration().pixelFormat, > > - .size = data->mainPathStream_.configuration().size, > > - }; > > + streamConfig[0] = IPAStream( > > + data->mainPathStream_.configuration().pixelFormat, > > + data->mainPathStream_.configuration().size > > + ); > > } > > > > if (data->selfPath_->isEnabled()) { > > @@ -788,10 +791,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > return ret; > > } > > > > - streamConfig[1] = { > > - .pixelFormat = data->selfPathStream_.configuration().pixelFormat, > > - .size = data->selfPathStream_.configuration().size, > > - }; > > + streamConfig[1] = IPAStream( > > + data->selfPathStream_.configuration().pixelFormat, > > + data->selfPathStream_.configuration().size > > + ); > > } > > > > isp_->setFrameStartEnabled(true); > > @@ -808,12 +811,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c > > ret = 0; > > } > > > > - std::map<unsigned int, const ControlInfoMap &> entityControls; > > + std::map<uint32_t, ControlInfoMap> entityControls; > > entityControls.emplace(0, data->sensor_->controls()); > > > > - IPAOperationData ipaConfig; > > - data->ipa_->configure(sensorInfo, streamConfig, entityControls, > > - ipaConfig, nullptr); > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls); > > It looks like there is a wrong calling order in rkisp1. The call to data->ipa_->configure > is done upon the "start" method of the rkisp1 pipline-handler instead of the "configure" method. > This is bug not related to this patchset. But with this patchset, the call to data->ipa_->configure > is after the call to data->ipa_->start when only async calls are allowed. Good catch ! > I can send a patch fixing that. That would be nice, thanks. > > > > return ret; > > } > > @@ -855,11 +856,12 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > > if (!info) > > return -ENOENT; > > > > - IPAOperationData op; > > - op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST; > > - op.data = { data->frame_, info->paramBuffer->cookie() }; > > - op.controls = { request->controls() }; > > - data->ipa_->processEvent(op); > > + ipa::rkisp1::RkISP1Event ev; > > + ev.op = ipa::rkisp1::EventQueueRequest; > > + ev.frame = data->frame_; > > + ev.bufferId = info->paramBuffer->cookie(); > > + ev.controls = request->controls(); > > + data->ipa_->processEvent(ev); > > > > data->frame_++; > > > > @@ -1090,10 +1092,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer) > > if (data->frame_ <= buffer->metadata().sequence) > > data->frame_ = buffer->metadata().sequence + 1; > > > > - IPAOperationData op; > > - op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER; > > - op.data = { info->frame, info->statBuffer->cookie() }; > > - data->ipa_->processEvent(op); > > + ipa::rkisp1::RkISP1Event ev; > > + ev.op = ipa::rkisp1::EventSignalStatBuffer; > > + ev.frame = info->frame; > > + ev.bufferId = info->statBuffer->cookie(); > > + data->ipa_->processEvent(ev); > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
v7 is split in three parts, core components, conversion + plumbing, and tests + documentation. This is part 2, and implements the plumbing for IPA isolation. Patches 4/12 to 11/12 must be squashed together to avoid bisection breakage. Patch 8/12 may be of interest, as it shows how one would write the interface and data definition. In v2 I have customized the raspberrypi mojom file to take advantage of the freedom in definition the IPA interface and data structures (as opposed to v1, where it was practically a direct translation of IPAOperationData). In v3 I have added namespace support to the data definition, and this is reflected in its usage. In v6 I added support for custom start(), and used it. Also in v6, support for consts was added, so I moved all the enum and const definitions to the mojom file. As of v6, namespacing in the mojom files is required, in the form of /^ipa\.[0-9A-Za-z]+/ and this is reflected in the three mojom files in this series. Patch 9/11 is also noteworthy, as it shows how one would use the interface that has been defined. Since the headers and proxies are autogenerated, there isn't a patch where the generated code can be seen, however. Patches 10/12 to 12/12 also define mojom files and use them, but are for vimc, rkisp1, and ipu3, respectively. These will also give an idea on how the mojom definition looks and how it will be used. Changes in v7: - clean up raspberrypi IPA interface while rebasing - add ipu3 support for the new IPC (from Niklas) Changes in v6: - move definitions of libcamera structs to core.mojom (in Part 1) - move enums and consts from the ipa header to the mojom file - remove vimc.h, use the ipa::vimc namespace, and use the new start() - same for rkisp1.h - remove postfix _ in generated struct fields - use the required ipa namespace - fill the LS table handle in the raspberry pi pipeline handler, instead of passing it from the IPA - remove per-pipeline ContrlInfoMap from the ipa header - remove the ControlInfoMap initializer from raspberrypi header Niklas Söderlund (1): libcamera: pipeline, ipa: ipu3: Support the new IPC mechanism Paul Elder (11): meson: ipa, proxy: Generate headers and proxy with mojo tests: Remove IPA wrappers test ipa: raspberrypi: meson: Add dependency on generated headers libcamera: IPAInterface: Replace C API with the new C++-only API libcamera: IPAProxy: Remove stop() override libcamera: IPAProxy, IPAManager: Switch to one-proxy-per-pipeline scheme libcamera: PipelineHandler: Remove IPA from base class ipa: raspberrypi: Add mojom data definition file libcamera: pipeline, ipa: raspberrypi: Use new data definition libcamera: pipeline, ipa: vimc: Support the new IPC mechanism libcamera: pipeline, ipa: rkisp1: Support the new IPC mechanism Documentation/Doxyfile.in | 1 - .../libcamera/internal/ipa_context_wrapper.h | 53 -- include/libcamera/internal/ipa_manager.h | 31 +- include/libcamera/internal/ipa_module.h | 4 +- include/libcamera/internal/ipa_proxy.h | 31 - include/libcamera/internal/meson.build | 1 - include/libcamera/internal/pipeline_handler.h | 1 - include/libcamera/ipa/ipa_interface.h | 147 +--- include/libcamera/ipa/ipu3.h | 23 - include/libcamera/ipa/ipu3.mojom | 43 ++ include/libcamera/ipa/meson.build | 125 ++++ include/libcamera/ipa/raspberrypi.h | 31 - include/libcamera/ipa/raspberrypi.mojom | 131 ++++ include/libcamera/ipa/rkisp1.h | 22 - include/libcamera/ipa/rkisp1.mojom | 44 ++ include/libcamera/ipa/vimc.h | 28 - include/libcamera/ipa/vimc.mojom | 24 + src/ipa/ipu3/ipu3.cpp | 74 +- src/ipa/ipu3/meson.build | 6 +- src/ipa/libipa/ipa_interface_wrapper.cpp | 287 -------- src/ipa/libipa/ipa_interface_wrapper.h | 61 -- src/ipa/libipa/meson.build | 2 - src/ipa/raspberrypi/meson.build | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 180 ++--- src/ipa/rkisp1/meson.build | 2 +- src/ipa/rkisp1/rkisp1.cpp | 63 +- src/ipa/vimc/meson.build | 2 +- src/ipa/vimc/vimc.cpp | 39 +- src/libcamera/ipa_context_wrapper.cpp | 298 -------- src/libcamera/ipa_interface.cpp | 650 +----------------- src/libcamera/ipa_manager.cpp | 47 +- src/libcamera/ipa_module.cpp | 18 +- src/libcamera/ipa_proxy.cpp | 101 --- src/libcamera/meson.build | 2 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 95 +-- .../pipeline/raspberrypi/raspberrypi.cpp | 257 ++++--- .../pipeline/raspberrypi/rpi_stream.cpp | 6 +- .../pipeline/raspberrypi/rpi_stream.h | 5 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 ++- src/libcamera/pipeline/vimc/vimc.cpp | 10 +- src/libcamera/pipeline_handler.cpp | 8 - src/libcamera/proxy/ipa_proxy_linux.cpp | 104 --- src/libcamera/proxy/ipa_proxy_thread.cpp | 175 ----- src/libcamera/proxy/meson.build | 21 +- .../proxy/worker/ipa_proxy_linux_worker.cpp | 90 --- src/libcamera/proxy/worker/meson.build | 23 +- test/ipa/ipa_interface_test.cpp | 44 +- test/ipa/ipa_wrappers_test.cpp | 455 ------------ test/ipa/meson.build | 3 +- utils/ipc/generators/meson.build | 3 + utils/ipc/meson.build | 13 + 51 files changed, 883 insertions(+), 3082 deletions(-) delete mode 100644 include/libcamera/internal/ipa_context_wrapper.h delete mode 100644 include/libcamera/ipa/ipu3.h create mode 100644 include/libcamera/ipa/ipu3.mojom create mode 100644 include/libcamera/ipa/raspberrypi.mojom delete mode 100644 include/libcamera/ipa/rkisp1.h create mode 100644 include/libcamera/ipa/rkisp1.mojom delete mode 100644 include/libcamera/ipa/vimc.h create mode 100644 include/libcamera/ipa/vimc.mojom delete mode 100644 src/ipa/libipa/ipa_interface_wrapper.cpp delete mode 100644 src/ipa/libipa/ipa_interface_wrapper.h delete mode 100644 src/libcamera/ipa_context_wrapper.cpp delete mode 100644 src/libcamera/proxy/ipa_proxy_linux.cpp delete mode 100644 src/libcamera/proxy/ipa_proxy_thread.cpp delete mode 100644 src/libcamera/proxy/worker/ipa_proxy_linux_worker.cpp delete mode 100644 test/ipa/ipa_wrappers_test.cpp create mode 100644 utils/ipc/generators/meson.build