[libcamera-devel] [PATCH 2/2] ipa: rkisp1: Replace event-based ops with dedicated functions
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 15 09:54:25 CET 2022
Hi Umang,
Thank you for the patch.
On Tue, Mar 08, 2022 at 02:45:20PM +0530, Umang Jain via libcamera-devel wrote:
> The IPARkISP1Interface currently uses event-type based structures in
> order to communicate with the pipeline-handler (and vice-versa).
> Replace the event based structures with dedicated functions associated
> to each operation.
>
> The translated naming scheme of operations to dedicated functions:
> ActionV4L2Set => setSensorControls
> ActionParamFilled => paramsBufferReady
> ActionMetadata => statsBufferReady
> EventSignalStatBuffer => processStatsBuffer()
> EventQueueRequest => queueRequest()
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> include/libcamera/ipa/rkisp1.mojom | 31 ++-------
> src/ipa/rkisp1/rkisp1.cpp | 82 +++++++-----------------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 71 ++++++++------------
> 3 files changed, 56 insertions(+), 128 deletions(-)
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index c3a6d8e1..a0b92b84 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -8,28 +8,6 @@ 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;
> - libcamera.ControlList controls;
> - libcamera.ControlList sensorControls;
> -};
> -
> -struct RkISP1Action {
> - RkISP1Operations op;
> - libcamera.ControlList controls;
> - libcamera.ControlList sensorControls;
> -};
> -
> interface IPARkISP1Interface {
> init(libcamera.IPASettings settings,
> uint32 hwRevision)
> @@ -45,9 +23,14 @@ interface IPARkISP1Interface {
> mapBuffers(array<libcamera.IPABuffer> buffers);
> unmapBuffers(array<uint32> ids);
>
> - [async] processEvent(RkISP1Event ev);
> + [async] queueRequest(uint32 frame, uint32 bufferId,
> + libcamera.ControlList reqControls);
> + [async] processStatsBuffer(uint32 frame, uint32 bufferId,
> + libcamera.ControlList sensorControls);
> };
>
> interface IPARkISP1EventInterface {
> - queueFrameAction(uint32 frame, RkISP1Action action);
> + paramsBufferReady(uint32 frame);
> + setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
> + statsBufferReady(uint32 frame, libcamera.ControlList metadata);
I'd name this metadataReady, as it reports the request metadata computed
by the IPA. The metadata is computed from the stats (among other
things), but this event doesn't indicate that stats are ready.
With this fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> };
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 129afddd..dc00c1f8 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -51,14 +51,12 @@ public:
> 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 RkISP1Event &event) override;
>
> + void queueRequest(const uint32_t frame, const uint32_t bufferId,
> + const ControlList &controls) override;
> + void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> + const ControlList &sensorControls) override;
> private:
> - void queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> - const ControlList &controls);
> - void updateStatistics(unsigned int frame,
> - const rkisp1_stat_buffer *stats);
> -
> void setControls(unsigned int frame);
> void metadataReady(unsigned int frame, unsigned int aeState);
>
> @@ -231,60 +229,34 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> }
> }
>
> -void IPARkISP1::processEvent(const RkISP1Event &event)
> -{
> - switch (event.op) {
> - case EventSignalStatBuffer: {
> - unsigned int frame = event.frame;
> - unsigned int bufferId = event.bufferId;
> -
> - const rkisp1_stat_buffer *stats =
> - reinterpret_cast<rkisp1_stat_buffer *>(
> - mappedBuffers_.at(bufferId).planes()[0].data());
> -
> - context_.frameContext.sensor.exposure =
> - event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> - context_.frameContext.sensor.gain =
> - camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> -
> - updateStatistics(frame, stats);
> - break;
> - }
> - case EventQueueRequest: {
> - unsigned int frame = event.frame;
> - unsigned int bufferId = event.bufferId;
> -
> - rkisp1_params_cfg *params =
> - reinterpret_cast<rkisp1_params_cfg *>(
> - mappedBuffers_.at(bufferId).planes()[0].data());
> -
> - queueRequest(frame, params, event.controls);
> - break;
> - }
> - default:
> - LOG(IPARkISP1, Error) << "Unknown event " << event.op;
> - break;
> - }
> -}
> -
> -void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> +void IPARkISP1::queueRequest(const uint32_t frame, const uint32_t bufferId,
> [[maybe_unused]] const ControlList &controls)
> {
> + rkisp1_params_cfg *params =
> + reinterpret_cast<rkisp1_params_cfg *>(
> + mappedBuffers_.at(bufferId).planes()[0].data());
> +
> /* Prepare parameters buffer. */
> memset(params, 0, sizeof(*params));
>
> for (auto const &algo : algorithms_)
> algo->prepare(context_, params);
>
> - RkISP1Action op;
> - op.op = ActionParamFilled;
> -
> - queueFrameAction.emit(frame, op);
> + paramsBufferReady.emit(frame);
> }
>
> -void IPARkISP1::updateStatistics(unsigned int frame,
> - const rkisp1_stat_buffer *stats)
> +void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> + const ControlList &sensorControls)
> {
> + const rkisp1_stat_buffer *stats =
> + reinterpret_cast<rkisp1_stat_buffer *>(
> + mappedBuffers_.at(bufferId).planes()[0].data());
> +
> + context_.frameContext.sensor.exposure =
> + sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> + context_.frameContext.sensor.gain =
> + camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +
> unsigned int aeState = 0;
>
> for (auto const &algo : algorithms_)
> @@ -297,18 +269,14 @@ void IPARkISP1::updateStatistics(unsigned int frame,
>
> void IPARkISP1::setControls(unsigned int frame)
> {
> - RkISP1Action op;
> - op.op = ActionV4L2Set;
> -
> uint32_t exposure = context_.frameContext.agc.exposure;
> uint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
>
> 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.sensorControls = ctrls;
>
> - queueFrameAction.emit(frame, op);
> + setSensorControls.emit(frame, ctrls);
> }
>
> void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> @@ -318,11 +286,7 @@ void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> if (aeState)
> ctrls.set(controls::AeLocked, aeState == 2);
>
> - RkISP1Action op;
> - op.op = ActionMetadata;
> - op.controls = ctrls;
> -
> - queueFrameAction.emit(frame, op);
> + statsBufferReady.emit(frame, ctrls);
> }
>
> } /* namespace ipa::rkisp1 */
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8cca8a15..d395e335 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -104,8 +104,9 @@ public:
> std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
>
> private:
> - void queueFrameAction(unsigned int frame,
> - const ipa::rkisp1::RkISP1Action &action);
> + void paramFilled(unsigned int frame);
> + void setSensorControls(unsigned int frame,
> + const ControlList &sensorControls);
>
> void metadataReady(unsigned int frame, const ControlList &metadata);
> };
> @@ -316,8 +317,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> if (!ipa_)
> return -ENOENT;
>
> - ipa_->queueFrameAction.connect(this,
> - &RkISP1CameraData::queueFrameAction);
> + ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
> + ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
> + ipa_->statsBufferReady.connect(this, &RkISP1CameraData::metadataReady);
>
> int ret = ipa_->init(IPASettings{ "", sensor_->model() }, hwRevision);
> if (ret < 0) {
> @@ -328,39 +330,27 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> return 0;
> }
>
> -void RkISP1CameraData::queueFrameAction(unsigned int frame,
> - const ipa::rkisp1::RkISP1Action &action)
> +void RkISP1CameraData::paramFilled(unsigned int frame)
> {
> - switch (action.op) {
> - case ipa::rkisp1::ActionV4L2Set: {
> - const ControlList &controls = action.sensorControls;
> - delayedCtrls_->push(controls);
> - break;
> - }
> - case ipa::rkisp1::ActionParamFilled: {
> - PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();
> - RkISP1FrameInfo *info = frameInfo_.find(frame);
> - if (!info)
> - break;
> + PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();
> + RkISP1FrameInfo *info = frameInfo_.find(frame);
> + if (!info)
> + return;
>
> - pipe->param_->queueBuffer(info->paramBuffer);
> - pipe->stat_->queueBuffer(info->statBuffer);
> + pipe->param_->queueBuffer(info->paramBuffer);
> + pipe->stat_->queueBuffer(info->statBuffer);
>
> - if (info->mainPathBuffer)
> - mainPath_->queueBuffer(info->mainPathBuffer);
> + if (info->mainPathBuffer)
> + mainPath_->queueBuffer(info->mainPathBuffer);
>
> - if (info->selfPathBuffer)
> - selfPath_->queueBuffer(info->selfPathBuffer);
> + if (info->selfPathBuffer)
> + selfPath_->queueBuffer(info->selfPathBuffer);
> +}
>
> - break;
> - }
> - case ipa::rkisp1::ActionMetadata:
> - metadataReady(frame, action.controls);
> - break;
> - default:
> - LOG(RkISP1, Error) << "Unknown action " << action.op;
> - break;
> - }
> +void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> + const ControlList &sensorControls)
> +{
> + delayedCtrls_->push(sensorControls);
> }
>
> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> @@ -865,13 +855,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> if (!info)
> return -ENOENT;
>
> - 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->ipa_->queueRequest(data->frame_, info->paramBuffer->cookie(),
> + request->controls());
> data->frame_++;
>
> return 0;
> @@ -1120,12 +1105,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> if (data->frame_ <= buffer->metadata().sequence)
> data->frame_ = buffer->metadata().sequence + 1;
>
> - ipa::rkisp1::RkISP1Event ev;
> - ev.op = ipa::rkisp1::EventSignalStatBuffer;
> - ev.frame = info->frame;
> - ev.bufferId = info->statBuffer->cookie();
> - ev.sensorControls = data->delayedCtrls_->get(buffer->metadata().sequence);
> - data->ipa_->processEvent(ev);
> + data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
> + data->delayedCtrls_->get(buffer->metadata().sequence));
> }
>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list