[libcamera-devel] [PATCH 2/2] ipa: rkisp1: Replace event-based ops with dedicated functions
Umang Jain
umang.jain at ideasonboard.com
Wed Mar 23 15:53:25 CET 2022
On 3/15/22 14:24, Laurent Pinchart wrote:
> 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,
Address locally however, metadataReady() is a private member of
IPARkISP1 class hence having the same signal name made the compliers
unhappy:
../src/ipa/rkisp1/rkisp1.cpp: In member function ‘void
libcamera::ipa::rkisp1::IPARkISP1::metadataReady(unsigned int, unsigned
int)’:
../src/ipa/rkisp1/rkisp1.cpp:289:9: error: invalid use of member
function ‘void libcamera::ipa::rkisp1::IPARkISP1::metadataReady(unsigned
int, unsigned int)’ (did you forget the ‘()’ ?)
289 | metadataReady.emit(frame, ctrls);
| ^~~~~~~~~~~~~
../src/ipa/rkisp1/rkisp1.cpp:282:44: error: unused parameter ‘frame’
[-Werror=unused-parameter]
282 | void IPARkISP1::metadataReady(unsigned int frame, unsigned int
aeState)
| ~~~~~~~~~~~~~^~~~~
cc1plus: all warnings being treated as errors
[173/334] Generating doxygen with a custom command
Hence I am forced to rename IPARkISP1::metadataReady() private function
to IPARkISP1::prepareMetadata() . I'll attach a final patch in few minutes.
>
> 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)
More information about the libcamera-devel
mailing list