[libcamera-devel] [PATCH 1/2] ipa: ipu3: Replace event-based operations with dedicated functions
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Mon Feb 28 19:32:47 CET 2022
On 28/02/2022 19:30, Jean-Michel Hautbois wrote:
> Hi Umang,
>
> Thanks for this patch, a big one :-).
>
> On 28/02/2022 14:22, Umang Jain wrote:
>> The IPAIPU3 interface 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.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> include/libcamera/ipa/ipu3.mojom | 36 ++------
>> src/ipa/ipu3/ipu3-ipa-design-guide.rst | 19 ++--
>> src/ipa/ipu3/ipu3.cpp | 116 ++++++++++-------------
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 123 +++++++++++--------------
>> 4 files changed, 123 insertions(+), 171 deletions(-)
>>
>> diff --git a/include/libcamera/ipa/ipu3.mojom
>> b/include/libcamera/ipa/ipu3.mojom
>> index cc0d822f..12b8bb2f 100644
>> --- a/include/libcamera/ipa/ipu3.mojom
>> +++ b/include/libcamera/ipa/ipu3.mojom
>> @@ -8,32 +8,6 @@ module ipa.ipu3;
>> import "include/libcamera/ipa/core.mojom";
>> -enum IPU3Operations {
>> - ActionSetSensorControls = 1,
>> - ActionParamFilled = 2,
>> - ActionMetadataReady = 3,
>> - EventProcessControls = 4,
>> - EventStatReady = 5,
>> - EventFillParams = 6,
>> -};
>> -
>> -struct IPU3Event {
>> - IPU3Operations op;
>> - uint32 frame;
>> - int64 frameTimestamp;
>> - uint32 bufferId;
>> - libcamera.ControlList controls;
>> - libcamera.ControlList sensorControls;
>> - libcamera.ControlList lensControls;
>> -};
>> -
>> -struct IPU3Action {
>> - IPU3Operations op;
>> - libcamera.ControlList controls;
>> - libcamera.ControlList sensorControls;
>> - libcamera.ControlList lensControls;
>> -};
>> -
>> struct IPAConfigInfo {
>> libcamera.IPACameraSensorInfo sensorInfo;
>> libcamera.ControlInfoMap sensorControls;
>> @@ -55,9 +29,15 @@ interface IPAIPU3Interface {
>> mapBuffers(array<libcamera.IPABuffer> buffers);
>> unmapBuffers(array<uint32> ids);
>> - [async] processEvent(IPU3Event ev);
>> + [async] fillParameterBuffer(uint32 frame, uint32 bufferId);
>> + [async] processControls(uint32 frame, libcamera.ControlList
>> controls);
>> + [async] statsReady(uint32 frame, int64 frameTimestamp, uint32
>> bufferId,
>> + libcamera.ControlList sensorControls);
>> };
>> interface IPAIPU3EventInterface {
>> - queueFrameAction(uint32 frame, IPU3Action action);
>> + setSensorControls(uint32 frame, libcamera.ControlList
>> sensorControls,
>> + libcamera.ControlList lensControls);
>> + paramFilled(uint32 frame);
>> + metadataReady(uint32 frame, libcamera.ControlList metadata);
>> };
>
> I am a bit confused about the namings, I would go (personnal choice :-))
> for:
> fillParamsBuffer
> paramsBufferFilled
>
> On the other end, we have the stats.
> s/statsReady/processStatsBuffer ?
> And then, I can't say why it is named metadataReady...
> s/metadataReady/statsBufferReady ?
>
> This is a very first thinking about it, I know that namings are always
> tricky, my proposal are certainly not the best ones :-).
>
And as I was reading myself, I thought:
s/fillParameterBuffer/processParamsBuffer
s/paramFilled/paramsBufferReady
s/statsReady/processStatsBuffer
s/metadataReady/statsBufferReady
>> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst
>> b/src/ipa/ipu3/ipu3-ipa-design-guide.rst
>> index 89c71108..83f8634d 100644
>> --- a/src/ipa/ipu3/ipu3-ipa-design-guide.rst
>> +++ b/src/ipa/ipu3/ipu3-ipa-design-guide.rst
>> @@ -25,7 +25,8 @@ from applications, and managing events from the
>> pipeline handler.
>> └─┬───┬───┬──────┬────┬────┬────┬─┴────▼─┬──┘ 1: init()
>> │ │ │ │ ▲ │ ▲ │ ▲ │ ▲ │ 2: configure()
>> │1 │2 │3 │4│ │4│ │4│ │4│ │5 3:
>> mapBuffers(), start()
>> - ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ 4: processEvent()
>> + │ │ │ │ │ │ │ │ │ │ │ │ 4: (▼)
>> processControls(), fillParametersBuffer(), statsReady()
>> + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲)
>> setSensorControls, paramFilled, metadataReady Signals
>> ┌──────────────────┴────┴────┴────┴─────────┐ 5: stop(),
>> unmapBuffers()
>> │ IPU3 IPA │
>> │ ┌───────────────────────┐ │
>> @@ -100,8 +101,9 @@ There are three main interactions with the
>> algorithms for the IPU3 IPA
>> to operate when running:
>> - configure()
>> -- processEvent(``EventFillParams``)
>> -- processEvent(``EventStatReady``)
>> +- fillParameterBuffer()
>> +- processControls()
>> +- statsReady()
>> The configuration phase allows the pipeline-handler to inform the
>> IPA of
>> the current stream configurations, which is then passed into each
>> @@ -115,7 +117,7 @@ When configured, the IPA is notified by the
>> pipeline handler of the
>> Camera ``start()`` event, after which incoming requests will be queued
>> for processing, requiring a parameter buffer (``ipu3_uapi_params``) to
>> be populated for the ImgU. This is given to the IPA through the
>> -``EventFillParams`` event, and then passed directly to each algorithm
>> +``fillParameterBuffer()``, and then passed directly to each algorithm
>> through the ``prepare()`` call allowing the ISP configuration to be
>> updated for the needs of each component that the algorithm is
>> responsible for.
>> @@ -125,7 +127,7 @@ structure that it modifies, and it should take
>> care to ensure that any
>> structure set by a use flag is fully initialised to suitable values.
>> The parameter buffer is returned to the pipeline handler through the
>> -``ActionParamFilled`` event, and from there queued to the ImgU along
>> +``paramFilled`` signal, and from there queued to the ImgU along
>> with a raw frame captured with the CIO2.
>> Post-frame completion
>> @@ -134,7 +136,7 @@ Post-frame completion
>> When the capture of an image is completed, and successfully processed
>> through the ImgU, the generated statistics buffer
>> (``ipu3_uapi_stats_3a``) is given to the IPA through the
>> -``EventStatReady`` event. This provides the IPA with an opportunity to
>> +``statsReady()``. This provides the IPA with an opportunity to
>> examine the results of the ISP and run the calculations required by
>> each
>> algorithm on the new data. The algorithms may require context from the
>> operations of other algorithms, for example, the AWB might choose to
>> use
>> @@ -145,11 +147,14 @@ before they are needed.
>> The ordering of the algorithm processing is determined by their
>> placement in the ``IPU3::algorithms_`` ordered list.
>> +Finally, the IPA metadata for the frame completed is returned back via
>> +``metadataReady`` Signal.
>> +
>> Sensor Controls
>> ~~~~~~~~~~~~~~~
>> The AutoExposure and AutoGain (AGC) algorithm differs slightly from the
>> others as it requires operating directly on the sensor, as opposed to
>> through the ImgU ISP. To support this, there is a dedicated action
>> -`ActionSetSensorControls` to allow the IPA to request controls to be set
>> +``setSensorControls`` signal to allow the IPA to request controls to
>> be set
>> on the camera sensor through the pipeline handler.
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 3d307708..2fab4ee0 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -142,14 +142,18 @@ public:
>> void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>> void unmapBuffers(const std::vector<unsigned int> &ids) override;
>> - void processEvent(const IPU3Event &event) override;
>> +
>> + void fillParameterBuffer(const uint32_t frame, const uint32_t
>> bufferId) override;
>> + void processControls(const uint32_t frame, const ControlList
>> &controls) override;
>> + void statsReady(const uint32_t frame, const int64_t frameTimestamp,
>> + const uint32_t bufferId, const ControlList
>> &sensorControls) override;
>> private:
>> void updateControls(const IPACameraSensorInfo &sensorInfo,
>> const ControlInfoMap &sensorControls,
>> ControlInfoMap *ipaControls);
>> void updateSessionConfiguration(const ControlInfoMap
>> &sensorControls);
>> - void processControls(unsigned int frame, const ControlList
>> &controls);
>> +
>> void fillParams(unsigned int frame, ipu3_uapi_params *params);
>> void parseStatistics(unsigned int frame,
>> int64_t frameTimestamp,
>> @@ -505,58 +509,49 @@ void IPAIPU3::unmapBuffers(const
>> std::vector<unsigned int> &ids)
>> }
>> /**
>> - * \brief Process an event generated by the pipeline handler
>> - * \param[in] event The event sent from pipeline handler
>> - *
>> - * The expected event handling over the lifetime of a Request has
>> - * the following sequence:
>> - *
>> - * - EventProcessControls : Handle controls from a new Request
>> - * - EventFillParams : Prepare the ISP to process the Request
>> - * - EventStatReady : Process statistics after ISP completion
>> + * \brief Prepare the ISP to process the Request
>> + * \param[in] frame The frame number
>> + * \param[in] bufferId Buffer ID
>> */
>> -void IPAIPU3::processEvent(const IPU3Event &event)
>> +void IPAIPU3::fillParameterBuffer(const uint32_t frame, const
>> uint32_t bufferId)
>> {
>> - switch (event.op) {
>> - case EventProcessControls: {
>> - processControls(event.frame, event.controls);
>> - break;
>> - }
>> - case EventFillParams: {
>> - auto it = buffers_.find(event.bufferId);
>> - if (it == buffers_.end()) {
>> - LOG(IPAIPU3, Error) << "Could not find param buffer!";
>> - return;
>> - }
>> -
>> - Span<uint8_t> mem = it->second.planes()[0];
>> - ipu3_uapi_params *params =
>> - reinterpret_cast<ipu3_uapi_params *>(mem.data());
>> -
>> - fillParams(event.frame, params);
>> - break;
>> + auto it = buffers_.find(bufferId);
>> + if (it == buffers_.end()) {
>> + LOG(IPAIPU3, Error) << "Could not find param buffer!";
>> + return;
>> }
>> - case EventStatReady: {
>> - auto it = buffers_.find(event.bufferId);
>> - if (it == buffers_.end()) {
>> - LOG(IPAIPU3, Error) << "Could not find stats buffer!";
>> - return;
>> - }
>> -
>> - Span<uint8_t> mem = it->second.planes()[0];
>> - const ipu3_uapi_stats_3a *stats =
>> - reinterpret_cast<ipu3_uapi_stats_3a *>(mem.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>());
>>
>> -
>> - parseStatistics(event.frame, event.frameTimestamp, stats);
>> - break;
>> - }
>> - default:
>> - LOG(IPAIPU3, Error) << "Unknown event " << event.op;
>> - break;
>> +
>> + Span<uint8_t> mem = it->second.planes()[0];
>> + ipu3_uapi_params *params =
>> + reinterpret_cast<ipu3_uapi_params *>(mem.data());
>> +
>> + fillParams(frame, params);
>> +}
>> +
>> +/**
>> + * \brief Process statistics after ISP completion
>> + * \param[in] frame The frame number
>> + * \param[in] frameTimestamp Timestamp of the frame
>> + * \param[in] bufferId Buffer ID
>> + * \param[in] sensorControls Sensor controls
>> + */
>> +void IPAIPU3::statsReady(const uint32_t frame, const int64_t
>> frameTimestamp,
>> + const uint32_t bufferId, const ControlList &sensorControls)
>> +{
>> + auto it = buffers_.find(bufferId);
>> + if (it == buffers_.end()) {
>> + LOG(IPAIPU3, Error) << "Could not find stats buffer!";
>> + return;
>> }
>> +
>> + Span<uint8_t> mem = it->second.planes()[0];
>> + const ipu3_uapi_stats_3a *stats =
>> + reinterpret_cast<ipu3_uapi_stats_3a *>(mem.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>());
>>
>> +
>> + parseStatistics(frame, frameTimestamp, stats);
>> }
>> /**
>> @@ -567,7 +562,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>> * Parse the request to handle any IPA-managed controls that were
>> set from the
>> * application such as manual sensor settings.
>> */
>> -void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>> +void IPAIPU3::processControls(const uint32_t frame,
>> [[maybe_unused]] const ControlList &controls)
>> {
>> /* \todo Start processing for 'frame' based on 'controls'. */
>> @@ -597,10 +592,7 @@ void IPAIPU3::fillParams(unsigned int frame,
>> ipu3_uapi_params *params)
>> for (auto const &algo : algorithms_)
>> algo->prepare(context_, params);
>> - IPU3Action op;
>> - op.op = ActionParamFilled;
>> -
>> - queueFrameAction.emit(frame, op);
>> + paramFilled.emit(frame);
>> }
>> /**
>> @@ -642,11 +634,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>> * likely want to avoid putting platform specific metadata in.
>> */
>> - IPU3Action op;
>> - op.op = ActionMetadataReady;
>> - op.controls = ctrls;
>> -
>> - queueFrameAction.emit(frame, op);
>> + metadataReady.emit(frame, ctrls);
>> }
>> /**
>> @@ -658,18 +646,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>> */
>> void IPAIPU3::setControls(unsigned int frame)
>> {
>> - IPU3Action op;
>> - op.op = ActionSetSensorControls;
>> -
>> exposure_ = context_.frameContext.agc.exposure;
>> 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);
>> + ControlList lensCtrls;
>> +
>> + setSensorControls.emit(frame, ctrls, lensCtrls);
>> }
>> } /* namespace ipa::ipu3 */
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 6c5617cd..dd1780ef 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -86,8 +86,10 @@ public:
>> ControlInfoMap ipaControls_;
>> private:
>> - void queueFrameAction(unsigned int id,
>> - const ipa::ipu3::IPU3Action &action);
>> + void metadataReady(unsigned int id, const ControlList &metadata);
>> + void paramFilled(unsigned int id);
>> + void setSensorControls(unsigned int id, const ControlList
>> &sensorControls,
>> + const ControlList &lensControls);
>> };
>> class IPU3CameraConfiguration : public CameraConfiguration
>> @@ -866,11 +868,7 @@ void IPU3CameraData::queuePendingRequests()
>> info->rawBuffer = rawBuffer;
>> - ipa::ipu3::IPU3Event ev;
>> - ev.op = ipa::ipu3::EventProcessControls;
>> - ev.frame = info->id;
>> - ev.controls = request->controls();
>> - ipa_->processEvent(ev);
>> + ipa_->processControls(info->id, request->controls());
>> pendingRequests_.pop();
>> processingRequests_.push(request);
>> @@ -1213,7 +1211,9 @@ int IPU3CameraData::loadIPA()
>> if (!ipa_)
>> return -ENOENT;
>> - ipa_->queueFrameAction.connect(this,
>> &IPU3CameraData::queueFrameAction);
>> + ipa_->setSensorControls.connect(this,
>> &IPU3CameraData::setSensorControls);
>> + ipa_->paramFilled.connect(this, &IPU3CameraData::paramFilled);
>> + ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);
>> /*
>> * Pass the sensor info to the IPA to initialize controls.
>> @@ -1248,69 +1248,59 @@ int IPU3CameraData::loadIPA()
>> return 0;
>> }
>> -void IPU3CameraData::queueFrameAction(unsigned int id,
>> - const ipa::ipu3::IPU3Action &action)
>> +void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,
>> + const ControlList &sensorControls,
>> + const ControlList &lensControls)
>> {
>> - switch (action.op) {
>> - case ipa::ipu3::ActionSetSensorControls: {
>> - const ControlList &sensorControls = action.sensorControls;
>> - delayedCtrls_->push(sensorControls);
>> + delayedCtrls_->push(sensorControls);
>> - CameraLens *focusLens = cio2_.sensor()->focusLens();
>> - if (!focusLens)
>> - break;
>> -
>> - const ControlList lensControls = action.lensControls;
>> - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
>> - break;
>> + CameraLens *focusLens = cio2_.sensor()->focusLens();
>> + if (!focusLens)
>> + return;
>> - const ControlValue &focusValue =
>> - lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
>> + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
>> + return;
>> - focusLens->setFocusPostion(focusValue.get<int32_t>());
>> + const ControlValue &focusValue =
>> + lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
>> - break;
>> - }
>> - case ipa::ipu3::ActionParamFilled: {
>> - IPU3Frames::Info *info = frameInfos_.find(id);
>> - if (!info)
>> - break;
>> -
>> - /* Queue all buffers from the request aimed for the ImgU. */
>> - for (auto it : info->request->buffers()) {
>> - const Stream *stream = it.first;
>> - FrameBuffer *outbuffer = it.second;
>> + focusLens->setFocusPostion(focusValue.get<int32_t>());
>> +}
>> - if (stream == &outStream_)
>> - imgu_->output_->queueBuffer(outbuffer);
>> - else if (stream == &vfStream_)
>> - imgu_->viewfinder_->queueBuffer(outbuffer);
>> - }
>> +void IPU3CameraData::paramFilled(unsigned int id)
>> +{
>> + IPU3Frames::Info *info = frameInfos_.find(id);
>> + if (!info)
>> + return;
>> - imgu_->param_->queueBuffer(info->paramBuffer);
>> - imgu_->stat_->queueBuffer(info->statBuffer);
>> - imgu_->input_->queueBuffer(info->rawBuffer);
>> + /* Queue all buffers from the request aimed for the ImgU. */
>> + for (auto it : info->request->buffers()) {
>> + const Stream *stream = it.first;
>> + FrameBuffer *outbuffer = it.second;
>> - break;
>> + if (stream == &outStream_)
>> + imgu_->output_->queueBuffer(outbuffer);
>> + else if (stream == &vfStream_)
>> + imgu_->viewfinder_->queueBuffer(outbuffer);
>> }
>> - case ipa::ipu3::ActionMetadataReady: {
>> - IPU3Frames::Info *info = frameInfos_.find(id);
>> - if (!info)
>> - break;
>> - Request *request = info->request;
>> - request->metadata().merge(action.controls);
>> + imgu_->param_->queueBuffer(info->paramBuffer);
>> + imgu_->stat_->queueBuffer(info->statBuffer);
>> + imgu_->input_->queueBuffer(info->rawBuffer);
>> +}
>> - info->metadataProcessed = true;
>> - if (frameInfos_.tryComplete(info))
>> - pipe()->completeRequest(request);
>> +void IPU3CameraData::metadataReady(unsigned int id, const ControlList
>> &metadata)
>> +{
>> + IPU3Frames::Info *info = frameInfos_.find(id);
>> + if (!info)
>> + return;
>> - break;
>> - }
>> - default:
>> - LOG(IPU3, Error) << "Unknown action " << action.op;
>> - break;
>> - }
>> + Request *request = info->request;
>> + request->metadata().merge(metadata);
>> +
>> + info->metadataProcessed = true;
>> + if (frameInfos_.tryComplete(info))
>> + pipe()->completeRequest(request);
>> }
>> /*
>> -----------------------------------------------------------------------------
>>
>> @@ -1385,11 +1375,7 @@ void
>> IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>> if (request->findBuffer(&rawStream_))
>> pipe()->completeBuffer(request, buffer);
>> - ipa::ipu3::IPU3Event ev;
>> - ev.op = ipa::ipu3::EventFillParams;
>> - ev.frame = info->id;
>> - ev.bufferId = info->paramBuffer->cookie();
>> - ipa_->processEvent(ev);
>> + ipa_->fillParameterBuffer(info->id, info->paramBuffer->cookie());
>> }
>> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>> @@ -1433,13 +1419,8 @@ void
>> IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>> return;
>> }
>> - ipa::ipu3::IPU3Event ev;
>> - ev.op = ipa::ipu3::EventStatReady;
>> - ev.frame = info->id;
>> - ev.bufferId = info->statBuffer->cookie();
>> - ev.frameTimestamp =
>> request->metadata().get(controls::SensorTimestamp);
>> - ev.sensorControls = info->effectiveSensorControls;
>> - ipa_->processEvent(ev);
>> + ipa_->statsReady(info->id,
>> request->metadata().get(controls::SensorTimestamp),
>> + info->statBuffer->cookie(), info->effectiveSensorControls);
>> }
>> /*
More information about the libcamera-devel
mailing list