[libcamera-devel] [PATCH v7 11/12] libcamera: pipeline, ipa: rkisp1: Support the new IPC mechanism
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Feb 12 15:12:20 CET 2021
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 at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list