[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:30:47 CET 2022
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 :-).
> 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