Message ID | 20210213042225.112477-1-paul.elder@ideasonboard.com |
---|---|
Headers | show |
Series |
|
Related | show |
Hi Paul, Thank you for the patch. On Sat, Feb 13, 2021 at 01:22:24PM +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> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > Changes in v8: > - rebase on "libcamera: pipeline: rkisp1: configure IPA from configure > method instead of start method" > > 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 | 74 ++++++++++++------------ > 6 files changed, 111 insertions(+), 93 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 95eb5393..1a1c7159 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 f15efa9f..d29a3884 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; > } > } > @@ -613,17 +617,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (cfg.stream() == &data->mainPathStream_) { > ret = mainPath_.configure(cfg, format); > if (data->mainPath_->isEnabled()) > - streamConfig[0] = { > - .pixelFormat = cfg.pixelFormat, > - .size = cfg.size, > - }; > + streamConfig[0] = IPAStream(cfg.pixelFormat, > + cfg.size); > } else { > ret = selfPath_.configure(cfg, format); > if (data->selfPath_->isEnabled()) > - streamConfig[1] = { > - .pixelFormat = cfg.pixelFormat, > - .size = cfg.size, > - }; > + streamConfig[1] = IPAStream(cfg.pixelFormat, > + cfg.size); > } > > if (ret) > @@ -652,11 +652,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *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 0; > } > @@ -696,15 +695,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())); data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes())); would be a bit more efficient. > 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())); Same here. > availableStatBuffers_.push(buffer.get()); > } > > @@ -758,8 +757,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) > @@ -854,11 +852,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_++; > > @@ -1089,10 +1088,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)