[libcamera-devel] [PATCH v4 28/37] libcamera: pipeline, ipa: rkisp1: Support the new IPC mechanism
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Nov 12 12:13:47 CET 2020
Hi Paul,
Thanks for your work.
On 2020-11-06 19:36:58 +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 at ideasonboard.com>
I love how much nicer this interface is to comprehend. And once we get
this series merged I think we can evolve it even further, but for now I
really like it to "just" translate the old interface to the new.
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> ---
> 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 | 18 ++++----
> include/libcamera/ipa/rkisp1.mojom | 42 +++++++++++++++++++
> src/ipa/rkisp1/meson.build | 2 +-
> src/ipa/rkisp1/rkisp1.cpp | 53 +++++++++++-------------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 44 +++++++++++---------
> 6 files changed, 104 insertions(+), 56 deletions(-)
> create mode 100644 include/libcamera/ipa/rkisp1.mojom
>
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index 70f5dfc8..84a7f63d 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -26,6 +26,7 @@ ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',
>
> ipa_mojom_files = [
> 'raspberrypi.mojom',
> + 'rkisp1.mojom',
> 'vimc.mojom',
> ]
>
> diff --git a/include/libcamera/ipa/rkisp1.h b/include/libcamera/ipa/rkisp1.h
> index bb824f29..35f97ee4 100644
> --- a/include/libcamera/ipa/rkisp1.h
> +++ b/include/libcamera/ipa/rkisp1.h
> @@ -7,15 +7,19 @@
> #ifndef __LIBCAMERA_IPA_INTERFACE_RKISP1_H__
> #define __LIBCAMERA_IPA_INTERFACE_RKISP1_H__
>
> +#include <libcamera/controls.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,
> -};
> +namespace libcamera {
> +
> +namespace RkISP1 {
> +
> +static ControlInfoMap controls;
> +
> +} /* namespace RkISP1 */
> +
> +} /* namespace libcamera */
>
> #endif /* __DOXYGEN__ */
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> new file mode 100644
> index 00000000..7f1a4ddd
> --- /dev/null
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +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);
> +};
> +
> +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,
> +};
> +
> +struct RkISP1Event {
> + RkISP1Operations op;
> + uint32 frame;
> + uint32 bufferId;
> + ControlList controls;
> +};
> +
> +struct RkISP1Action {
> + RkISP1Operations op;
> + ControlList controls;
> +};
> 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 07d7f1b2..7bb261c6 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -20,6 +20,7 @@
> #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 <libipa/ipa_interface_wrapper.h>
> @@ -30,7 +31,7 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(IPARkISP1)
>
> -class IPARkISP1 : public IPAInterface
> +class IPARkISP1 : public IPARkISP1Interface
> {
> public:
> int init([[maybe_unused]] const IPASettings &settings) override
> @@ -41,13 +42,11 @@ public:
> 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 RkISP1Event &event) override;
>
> private:
> void queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> @@ -80,10 +79,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;
> @@ -159,12 +156,12 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> }
> }
>
> -void IPARkISP1::processEvent(const IPAOperationData &event)
> +void IPARkISP1::processEvent(const RkISP1Event &event)
> {
> - switch (event.operation) {
> + switch (event.op_) {
> case RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER: {
> - unsigned int frame = event.data[0];
> - unsigned int bufferId = event.data[1];
> + unsigned int frame = event.frame_;
> + unsigned int bufferId = event.bufferId_;
>
> const rkisp1_stat_buffer *stats =
> static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]);
> @@ -173,17 +170,17 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
> break;
> }
> case RKISP1_IPA_EVENT_QUEUE_REQUEST: {
> - unsigned int frame = event.data[0];
> - unsigned int bufferId = event.data[1];
> + 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;
> }
> }
> @@ -203,8 +200,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;
> + RkISP1Action op;
> + op.op_ = RKISP1_IPA_ACTION_PARAM_FILLED;
>
> queueFrameAction.emit(frame, op);
> }
> @@ -256,13 +253,13 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>
> void IPARkISP1::setControls(unsigned int frame)
> {
> - IPAOperationData op;
> - op.operation = RKISP1_IPA_ACTION_V4L2_SET;
> + RkISP1Action op;
> + op.op_ = RKISP1_IPA_ACTION_V4L2_SET;
>
> 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);
> }
> @@ -274,9 +271,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);
> + RkISP1Action op;
> + op.op_ = RKISP1_IPA_ACTION_METADATA;
> + op.controls_ = ctrls;
>
> queueFrameAction.emit(frame, op);
> }
> @@ -293,9 +290,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 c74a2e9b..0bf31918 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -19,6 +19,8 @@
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> #include <libcamera/ipa/rkisp1.h>
> +#include <libcamera/ipa/rkisp1_ipa_interface.h>
> +#include <libcamera/ipa/ipa_proxy_rkisp1.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
>
> @@ -144,9 +146,11 @@ public:
> RkISP1MainPath *mainPath_;
> RkISP1SelfPath *selfPath_;
>
> + std::unique_ptr<IPAProxyRkISP1> ipa_;
> +
> private:
> void queueFrameAction(unsigned int frame,
> - const IPAOperationData &action);
> + const RkISP1Action &action);
>
> void metadataReady(unsigned int frame, const ControlList &metadata);
> };
> @@ -412,7 +416,7 @@ private:
>
> int RkISP1CameraData::loadIPA()
> {
> - ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> + ipa_ = IPAManager::createIPA<IPAProxyRkISP1>(pipe_, 1, 1);
> if (!ipa_)
> return -ENOENT;
>
> @@ -425,11 +429,11 @@ int RkISP1CameraData::loadIPA()
> }
>
> void RkISP1CameraData::queueFrameAction(unsigned int frame,
> - const IPAOperationData &action)
> + const RkISP1Action &action)
> {
> - switch (action.operation) {
> + switch (action.op_) {
> case RKISP1_IPA_ACTION_V4L2_SET: {
> - const ControlList &controls = action.controls[0];
> + const ControlList &controls = action.controls_;
> timeline_.scheduleAction(std::make_unique<RkISP1ActionSetSensor>(frame,
> sensor_,
> controls));
> @@ -442,10 +446,10 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
> break;
> }
> case RKISP1_IPA_ACTION_METADATA:
> - metadataReady(frame, action.controls[0]);
> + metadataReady(frame, action.controls_);
> break;
> default:
> - LOG(RkISP1, Error) << "Unknown action " << action.operation;
> + LOG(RkISP1, Error) << "Unknown action " << action.op_;
> break;
> }
> }
> @@ -908,12 +912,10 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> 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;
> }
> @@ -955,11 +957,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);
> + RkISP1Event ev;
> + ev.op_ = RKISP1_IPA_EVENT_QUEUE_REQUEST;
> + 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,
> @@ -1181,10 +1184,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);
> + RkISP1Event ev;
> + ev.op_ = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> + 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list