| Message ID | 20201224081613.41662-1-paul.elder@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thanks for your patch. On 2020-12-24 17:16:12 +0900, Paul Elder wrote: > Add support to vimc pipeline handler and IPA for the new IPC mechanism. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > Changes in v6: > - move the enum and const from the header to the mojom file > - remove the per-pipeline ControlInfoMap > - since vimc.h has nothing in it anymore, remove it > - use the namespace in the pipeline handler and IPA > > Changes in v5: > - use the new generated header naming scheme > - add dummy event, since event interfaces are now required to have at > least one event > > Changes in v4: > - rename Controls to controls > - rename IPAVimcCallbackInterface to IPAVimcEventInterface > - rename libcamera_generated_headers to libcamera_generated_ipa_headers > in meson > - use the new header name, vimc_ipa_interface.h > > Changes in v3: > - change namespacing of base ControlInfoMap structure > > New in v2 > --- > include/libcamera/ipa/meson.build | 1 + > include/libcamera/ipa/vimc.h | 28 --------------------- > include/libcamera/ipa/vimc.mojom | 24 ++++++++++++++++++ > src/ipa/vimc/meson.build | 2 +- > src/ipa/vimc/vimc.cpp | 37 ++++++++++------------------ > src/libcamera/pipeline/vimc/vimc.cpp | 10 +++++--- > 6 files changed, 46 insertions(+), 56 deletions(-) > delete mode 100644 include/libcamera/ipa/vimc.h > create mode 100644 include/libcamera/ipa/vimc.mojom > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > index 785c1241..67e3f049 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', > + 'vimc.mojom', > ] > > ipa_mojoms = [] > diff --git a/include/libcamera/ipa/vimc.h b/include/libcamera/ipa/vimc.h > deleted file mode 100644 > index 27a4a61d..00000000 > --- a/include/libcamera/ipa/vimc.h > +++ /dev/null > @@ -1,28 +0,0 @@ > -/* SPDX-License-Identifier: LGPL-2.1-or-later */ > -/* > - * Copyright (C) 2019, Google Inc. > - * > - * vimc.h - Vimc Image Processing Algorithm module > - */ > - > -#ifndef __LIBCAMERA_IPA_VIMC_H__ > -#define __LIBCAMERA_IPA_VIMC_H__ > - > -#ifndef __DOXYGEN__ > - > -namespace libcamera { > - > -#define VIMC_IPA_FIFO_PATH "/tmp/libcamera_ipa_vimc_fifo" > - > -enum IPAOperationCode { > - IPAOperationNone, > - IPAOperationInit, > - IPAOperationStart, > - IPAOperationStop, > -}; > - > -} /* namespace libcamera */ > - > -#endif /* __DOXYGEN__ */ > - > -#endif /* __LIBCAMERA_IPA_VIMC_H__ */ > diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom > new file mode 100644 > index 00000000..165d9401 > --- /dev/null > +++ b/include/libcamera/ipa/vimc.mojom > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > + > +module ipa.vimc; > + > +import "include/libcamera/ipa/core.mojom"; > + > +const string VimcIPAFIFOPath = "/tmp/libcamera_ipa_vimc_fifo"; > + > +enum IPAOperationCode { > + IPAOperationNone, > + IPAOperationInit, > + IPAOperationStart, > + IPAOperationStop, > +}; > + > +interface IPAVimcInterface { > + init(IPASettings settings) => (int32 ret); > + start() => (int32 ret); > + stop(); > +}; > + > +interface IPAVimcEventInterface { > + dummyEvent(uint32 val); > +}; > diff --git a/src/ipa/vimc/meson.build b/src/ipa/vimc/meson.build > index 8c9df854..e982ebba 100644 > --- a/src/ipa/vimc/meson.build > +++ b/src/ipa/vimc/meson.build > @@ -3,7 +3,7 @@ > ipa_name = 'ipa_vimc' > > mod = shared_module(ipa_name, > - 'vimc.cpp', > + ['vimc.cpp', libcamera_generated_ipa_headers], > name_prefix : '', > include_directories : [ipa_includes, libipa_includes], > dependencies : libcamera_dep, > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp > index 436a5559..13681d88 100644 > --- a/src/ipa/vimc/vimc.cpp > +++ b/src/ipa/vimc/vimc.cpp > @@ -5,7 +5,7 @@ > * ipa_vimc.cpp - Vimc Image Processing Algorithm module > */ > > -#include <libcamera/ipa/vimc.h> > +#include <libcamera/ipa/vimc_ipa_interface.h> > > #include <fcntl.h> > #include <string.h> > @@ -24,7 +24,7 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPAVimc) > > -class IPAVimc : public IPAInterface > +class IPAVimc : public ipa::vimc::IPAVimcInterface > { > public: > IPAVimc(); > @@ -32,22 +32,12 @@ public: > > int init(const IPASettings &settings) override; > > - int start(const IPAOperationData &data, > - IPAOperationData *result) override; > + int start() override; > void stop() override; > > - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo, > - [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig, > - [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls, > - [[maybe_unused]] const IPAOperationData &ipaConfig, > - [[maybe_unused]] IPAOperationData *result) override {} > - void mapBuffers([[maybe_unused]] const std::vector<IPABuffer> &buffers) override {} > - void unmapBuffers([[maybe_unused]] const std::vector<unsigned int> &ids) override {} > - void processEvent([[maybe_unused]] const IPAOperationData &event) override {} > - > private: > void initTrace(); > - void trace(enum IPAOperationCode operation); > + void trace(enum ipa::vimc::IPAOperationCode operation); > > int fd_; > }; > @@ -66,7 +56,7 @@ IPAVimc::~IPAVimc() > > int IPAVimc::init(const IPASettings &settings) > { > - trace(IPAOperationInit); > + trace(ipa::vimc::IPAOperationInit); > > LOG(IPAVimc, Debug) > << "initializing vimc IPA with configuration file " > @@ -81,10 +71,9 @@ int IPAVimc::init(const IPASettings &settings) > return 0; > } > > -int IPAVimc::start([[maybe_unused]] const IPAOperationData &data, > - [[maybe_unused]] IPAOperationData *result) > +int IPAVimc::start() > { > - trace(IPAOperationStart); > + trace(ipa::vimc::IPAOperationStart); > > LOG(IPAVimc, Debug) << "start vimc IPA!"; > > @@ -93,7 +82,7 @@ int IPAVimc::start([[maybe_unused]] const IPAOperationData &data, > > void IPAVimc::stop() > { > - trace(IPAOperationStop); > + trace(ipa::vimc::IPAOperationStop); > > LOG(IPAVimc, Debug) << "stop vimc IPA!"; > } > @@ -101,11 +90,11 @@ void IPAVimc::stop() > void IPAVimc::initTrace() > { > struct stat fifoStat; > - int ret = stat(VIMC_IPA_FIFO_PATH, &fifoStat); > + int ret = stat(ipa::vimc::VimcIPAFIFOPath.c_str(), &fifoStat); > if (ret) > return; > > - ret = ::open(VIMC_IPA_FIFO_PATH, O_WRONLY); > + ret = ::open(ipa::vimc::VimcIPAFIFOPath.c_str(), O_WRONLY); > if (ret < 0) { > ret = errno; > LOG(IPAVimc, Error) << "Failed to open vimc IPA test FIFO: " > @@ -116,7 +105,7 @@ void IPAVimc::initTrace() > fd_ = ret; > } > > -void IPAVimc::trace(enum IPAOperationCode operation) > +void IPAVimc::trace(enum ipa::vimc::IPAOperationCode operation) > { > if (fd_ < 0) > return; > @@ -141,9 +130,9 @@ const struct IPAModuleInfo ipaModuleInfo = { > "vimc", > }; > > -struct ipa_context *ipaCreate() > +IPAInterface *ipaCreate() > { > - return new IPAInterfaceWrapper(std::make_unique<IPAVimc>()); > + return new IPAVimc(); > } > } > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index 8bda746f..d258090e 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -34,6 +34,9 @@ > #include "libcamera/internal/v4l2_subdevice.h" > #include "libcamera/internal/v4l2_videodevice.h" > > +#include <libcamera/ipa/vimc_ipa_interface.h> > +#include <libcamera/ipa/vimc_ipa_proxy.h> > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(VIMC) > @@ -56,6 +59,8 @@ public: > std::unique_ptr<V4L2VideoDevice> video_; > std::unique_ptr<V4L2VideoDevice> raw_; > Stream stream_; > + > + std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; > }; > > class VimcCameraConfiguration : public CameraConfiguration > @@ -311,8 +316,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con > if (ret < 0) > return ret; > > - IPAOperationData ipaData = {}; > - ret = data->ipa_->start(ipaData, nullptr); > + ret = data->ipa_->start(); > if (ret) { > data->video_->releaseBuffers(); > return ret; > @@ -418,7 +422,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > std::unique_ptr<VimcCameraData> data = std::make_unique<VimcCameraData>(this, media); > > - data->ipa_ = IPAManager::createIPA(this, 0, 0); > + data->ipa_ = IPAManager::createIPA<ipa::vimc::IPAProxyVimc>(this, 0, 0); > if (data->ipa_ != nullptr) { > std::string conf = data->ipa_->configurationFile("vimc.conf"); > data->ipa_->init(IPASettings{ conf }); > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thanks for your work. On 2020-12-24 17:16:13 +0900, Paul Elder wrote: > 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> > > --- > 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 021d0ffe..15b5e16e 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> > > @@ -139,9 +141,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); > }; > @@ -406,7 +410,7 @@ private: > > int RkISP1CameraData::loadIPA() > { > - ipa_ = IPAManager::createIPA(pipe_, 1, 1); > + ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); > if (!ipa_) > return -ENOENT; > > @@ -419,27 +423,27 @@ 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; > timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame, > sensor_.get(), > controls)); > break; > } > - case RKISP1_IPA_ACTION_PARAM_FILLED: { > + case ipa::rkisp1::ActionParamFilled: { > RkISP1FrameInfo *info = frameInfo_.find(frame); > if (info) > info->paramFilled = true; > 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; > } > } > @@ -766,15 +770,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()); > } > > @@ -828,8 +832,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) > @@ -870,10 +873,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()) { > @@ -887,10 +890,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 > + ); > } > > activeCamera_ = camera; > @@ -905,12 +908,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); > > return ret; > } > @@ -952,11 +953,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->timeline_.scheduleAction(std::make_unique<RkISP1ActionQueueBuffers>(data->frame_, > data, > @@ -1178,10 +1180,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) > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thank you for the patch. On Thu, Dec 24, 2020 at 05:16:10PM +0900, Paul Elder wrote: > Add a mojom data definition for raspberrypi pipeline handler's IPAs. > This is a direct translation of what the raspberrypi pipeline handler > and IPA was using before with IPAOperationData. > > Also move the enums from raspberrypi.h to raspberrypi.mojom > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v6: > - rebase on "pipeline: ipa: raspberrypi: Handle failures during IPA > configuration" > - move enums and consts to the mojom file > - use the custom start() > - remove unnecessary ConfigParameters, and remove lsTableHandleStatic > from ConfigInput > > Changes in v5: > - rename some structs > > Changes in v4: > - rename IPARPiCallbackInterface to IPARPiEventInterface > > Changes in v3: > - remove stray comment about how our compiler will generate code > - add ipa.rpi namespace > - not ipa.RPi, because namespace conflict > - remove RPi prefix from struct and enum names > - add transform parameter to struct ConfigInput > > Changes in v2: > - rebased on "libcamera: pipeline: ipa: raspberrypi: Rework drop frame > signalling" > - add license > - move generic documentation to core.mojom > - move documentation from IPA interface > - customize the RPi IPA interface to make the pipeline and IPA code > cleaner > --- > include/libcamera/ipa/meson.build | 4 +- > include/libcamera/ipa/raspberrypi.h | 20 ---- > include/libcamera/ipa/raspberrypi.mojom | 134 ++++++++++++++++++++++++ > 3 files changed, 137 insertions(+), 21 deletions(-) > create mode 100644 include/libcamera/ipa/raspberrypi.mojom > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > index 499d1bc0..785c1241 100644 > --- a/include/libcamera/ipa/meson.build > +++ b/include/libcamera/ipa/meson.build > @@ -54,7 +54,9 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h', > './' +'@INPUT@' > ]) > > -ipa_mojom_files = [] > +ipa_mojom_files = [ > + 'raspberrypi.mojom', > +] > > ipa_mojoms = [] > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > index 01fe5abc..df30b4a2 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -16,26 +16,6 @@ namespace libcamera { > > namespace RPi { > > -enum ConfigParameters { > - IPA_CONFIG_LS_TABLE = (1 << 0), > - IPA_CONFIG_STAGGERED_WRITE = (1 << 1), > - IPA_CONFIG_SENSOR = (1 << 2), > - IPA_CONFIG_DROP_FRAMES = (1 << 3), > - IPA_CONFIG_FAILED = (1 << 4), > - IPA_CONFIG_STARTUP = (1 << 5), > -}; > - > -enum Operations { > - IPA_ACTION_V4L2_SET_STAGGERED = 1, > - IPA_ACTION_V4L2_SET_ISP, > - IPA_ACTION_STATS_METADATA_COMPLETE, > - IPA_ACTION_RUN_ISP, > - IPA_ACTION_EMBEDDED_COMPLETE, > - IPA_EVENT_SIGNAL_STAT_READY, > - IPA_EVENT_SIGNAL_ISP_PREPARE, > - IPA_EVENT_QUEUE_REQUEST, > -}; > - > enum BufferMask { > ID = 0x00ffff, > STATS = 0x010000, > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > new file mode 100644 > index 00000000..53521ce9 > --- /dev/null > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -0,0 +1,134 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > + > +module ipa.rpi; > + > +import "include/libcamera/ipa/core.mojom"; > + > +enum BufferMask { > + MaskID = 0x00ffff, > + MaskStats = 0x010000, > + MaskEmbeddedData = 0x020000, > + MaskBayerData = 0x040000, > + MaskExternalBuffer = 0x100000, > +}; > + > +/* Size of the LS grid allocation. */ > +const uint32 MaxLsGridSize = 0x8000; It's annoying that the mojom parse doesn't allow us to write 32 * 1024 :-( Should we file a bug to Google to get this fixed eventually ? > + > +enum ConfigParameters { > + ConfigLsTable = 0x01, > + ConfigStaggeredWrite = 0x02, > + ConfigSensor = 0x04, > + ConfigFailed = 0x08, > +}; Now that we have a more dynamic interface, we could rework this. The ConfigLsTable flag isn't needed, as we can instead check if the lsTableHandle is valid or not. ConfigFailed would be best returned through a dedicated status field in ConfigOutput that would return 0 on success or a negative error code on error, through a boolean field named "success" or "error", or through a ret return value for the configure() function, like done with start(). We can keep ConfigStaggeredWrite and ConfigSensor for now (until we get support for nullable values), but I would already rename ConfigParameters to ConfigOutputParameters to emphasize the fact it's used for ConfigOutput only. The ConfigOutput::op field should be renamed to params, and the ConfigInput::op field should be dropped. You can do this as part as this patch already (but it then wouldn't be a direct translation anymore, as stated in the commit message) or as a patch on top (which can be moved to a Part 4 to avoid blocking the merge of parts 1 to 3), up to you. > + > +struct SensorConfig { > + uint32 gainDelay; > + uint32 exposureDelay; > + uint32 sensorMetadata; > +}; > + > +struct ISPConfig { > + uint32 embeddedbufferId; > + uint32 bayerbufferId; > +}; > + > +struct ConfigInput { > + uint32 op; > + uint32 transform; > + FileDescriptor lsTableHandle; > +}; > + > +struct ConfigOutput { > + uint32 op; > + SensorConfig sensorConfig; > + ControlList controls; > +}; > + > +struct StartControls { > + bool hasControls; > + ControlList controls; > + int32 dropFrameCount; > +}; > + > +interface IPARPiInterface { > + init(IPASettings settings) => (int32 ret); > + start(StartControls controls) => (StartControls result, int32 ret); > + stop(); > + > + /** > + * \fn configure() > + * \brief Configure the IPA stream and sensor settings > + * \param[in] sensorInfo Camera sensor information > + * \param[in] streamConfig Configuration of all active streams > + * \param[in] entityControls Controls provided by the pipeline entities > + * \param[in] ipaConfig Pipeline-handler-specific configuration data > + * \param[out] result Pipeline-handler-specific configuration result The return value is named "results". > + * > + * This method shall be called when the camera is started to inform the IPA of "started" or "configured" ? > + * the camera's streams and the sensor settings. > + * > + * The \a sensorInfo conveys information about the camera sensor settings that > + * the pipeline handler has selected for the configuration. > + * > + * The \a ipaConfig and \a result parameters carry data passed by the s/result/results/ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * pipeline handler to the IPA and back. > + */ > + configure(CameraSensorInfo sensorInfo, > + map<uint32, IPAStream> streamConfig, > + map<uint32, ControlInfoMap> entityControls, > + ConfigInput ipaConfig) > + => (ConfigOutput results); > + > + /** > + * \fn mapBuffers() > + * \brief Map buffers shared between the pipeline handler and the IPA > + * \param[in] buffers List of buffers to map > + * > + * This method informs the IPA module of memory buffers set up by the pipeline > + * handler that the IPA needs to access. It provides dmabuf file handles for > + * each buffer, and associates the buffers with unique numerical IDs. > + * > + * IPAs shall map the dmabuf file handles to their address space and keep a > + * cache of the mappings, indexed by the buffer numerical IDs. The IDs are used > + * in all other IPA interface methods to refer to buffers, including the > + * unmapBuffers() method. > + * > + * All buffers that the pipeline handler wishes to share with an IPA shall be > + * mapped with this method. Buffers may be mapped all at once with a single > + * call, or mapped and unmapped dynamically at runtime, depending on the IPA > + * protocol. Regardless of the protocol, all buffers mapped at a given time > + * shall have unique numerical IDs. > + * > + * The numerical IDs have no meaning defined by the IPA interface, and > + * should be treated as opaque handles by IPAs, with the only exception > + * that ID zero is invalid. > + * > + * \sa unmapBuffers() > + */ > + mapBuffers(array<IPABuffer> buffers); > + > + /** > + * \fn unmapBuffers() > + * \brief Unmap buffers shared by the pipeline to the IPA > + * \param[in] ids List of buffer IDs to unmap > + * > + * This method removes mappings set up with mapBuffers(). Numerical IDs > + * of unmapped buffers may be reused when mapping new buffers. > + * > + * \sa mapBuffers() > + */ > + unmapBuffers(array<uint32> ids); > + > + [async] signalStatReady(uint32 bufferId); > + [async] signalQueueRequest(ControlList controls); > + [async] signalIspPrepare(ISPConfig data); > +}; > + > +interface IPARPiEventInterface { > + statsMetadataComplete(uint32 bufferId, ControlList controls); > + runIsp(uint32 bufferId); > + embeddedComplete(uint32 bufferId); > + setIsp(ControlList controls); > + setStaggered(ControlList controls); > +};
Hi Paul, Thank you for the patch. On Thu, Dec 24, 2020 at 05:16:13PM +0900, Paul Elder wrote: > Add support to the rkisp1 pipeline handler and IPA for the new IPC > mechanism. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > 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; > +}; It would be nice to replace the op-based mechanism with dedicated API functions, but given that this will be extensively reworked anyway, it's fine for now. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > +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 021d0ffe..15b5e16e 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> > > @@ -139,9 +141,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); > }; > @@ -406,7 +410,7 @@ private: > > int RkISP1CameraData::loadIPA() > { > - ipa_ = IPAManager::createIPA(pipe_, 1, 1); > + ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); > if (!ipa_) > return -ENOENT; > > @@ -419,27 +423,27 @@ 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; > timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame, > sensor_.get(), > controls)); > break; > } > - case RKISP1_IPA_ACTION_PARAM_FILLED: { > + case ipa::rkisp1::ActionParamFilled: { > RkISP1FrameInfo *info = frameInfo_.find(frame); > if (info) > info->paramFilled = true; > 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; > } > } > @@ -766,15 +770,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()); > } > > @@ -828,8 +832,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) > @@ -870,10 +873,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()) { > @@ -887,10 +890,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 > + ); > } > > activeCamera_ = camera; > @@ -905,12 +908,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); > > return ret; > } > @@ -952,11 +953,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->timeline_.scheduleAction(std::make_unique<RkISP1ActionQueueBuffers>(data->frame_, > data, > @@ -1178,10 +1180,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)
v6 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/9 to 11/11 must be squashed together to avoid bisection breakage. Patch 8/11 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/11 and 11/11 also define mojom files and use them, but are for vimc and rkisp1, respectively. These will also give an idea on how the mojom definition looks and how it will be used. 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 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/meson.build | 124 ++++ include/libcamera/ipa/raspberrypi.h | 31 - include/libcamera/ipa/raspberrypi.mojom | 134 ++++ 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/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 | 183 +++-- 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 +- .../pipeline/raspberrypi/raspberrypi.cpp | 244 +++---- .../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 | 453 ------------ test/ipa/meson.build | 3 +- utils/ipc/generators/meson.build | 3 + utils/ipc/meson.build | 13 + 46 files changed, 778 insertions(+), 2936 deletions(-) delete mode 100644 include/libcamera/internal/ipa_context_wrapper.h 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