[libcamera-devel] [PATCH v3 1/1] ipa: ipu3: Replace event-based ops with dedicated functions
Umang Jain
umang.jain at ideasonboard.com
Wed Mar 30 07:30:55 CEST 2022
Hi Laurent,
On 3/30/22 01:34, Laurent Pinchart wrote:
> Hi Umang,
>
> On Wed, Mar 30, 2022 at 12:55:29AM +0530, Umang Jain wrote:
>> On 3/29/22 03:55, Laurent Pinchart wrote:
>>> On Mon, Mar 28, 2022 at 11:06:11PM +0530, Umang Jain via libcamera-devel wrote:
>>>> From: Umang Jain via libcamera-devel <libcamera-devel at lists.libcamera.org>
>>> That from line looks suspicious.
>> Yeah, I noticed only after sending in the patch :-/
>>
>>>> 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.
>>>>
>>>> The translated naming scheme of operations to dedicated functions:
>>>> ActionSetSensorControls => setSensorControls
>>>> ActionParamFilled => paramsBufferReady
>>>> ActionMetadataReady => metadataReady
>>>> EventProcessControls => processControls()
>>> What would you think about renaming this one to queueRequest(), to match
>>> the operation used by the rkisp1 IPA ?
>> I don't find the correct matching. rkisp1 queueRequest() is more of
>> ActionParamFilled rather than of processing Controls. It's basically
>> doing processing of incoming coming and filling of param buffer in one
>> function.
>>
>> I probably we should probably separate the rkisp1 queueRequest() to
>> match more of ipu3, rather the way around.
>>
>> The IPAIPU3::processControls() seems more intuitive to me rather than
>> IPAIPU3::queueRequest(...controls..). Since the queue-ing of Request
>> part is done by the pipeline-handler and in process of doing so, it
>> shall call ipa_->processControls(..controls..) to instruct the IPA to
>> address the incoming controls. Does it make sense?
> I think that what bothers me here is the word "process". When we will
> have proper per-frame controls in the IPU3 and RkISP1 IPAs, there will
> be a queue of frame contexts. The processControls() function, called
> when the request is queued, will give the controls present in the
> request to the IPA. Some of them may be processed immediately, but I'd
> expect the general idea will be to store those controls in the
> appropriate frame context, for later processing. For instance, controls
> that affect the ISP only may only be processing in the
> fillParamsBuffer() call. That's why I proposed queueRequest(), as the
Fair enough.
> function will put the parameters for the request in a queue. We could
> call it queueControls(), but "controls" alone is a bit ambiguous I
> think.
Yes, queueControls() doesn't sound right.
Maybe IPAIPU3::handleControls() or handleRequestControls() instead of
processControls() ?
>
> Regarding the RkISP1, I would indeed like to split the exising function
> that passes the request controls to the IPA in two. We currently return
Yes, I was irked by this while changing the interface for RkISP1 IPA.
However, I deliberately didn't introduce the change since I wanted to
address the interface migration first. I can take a look at it now that
it has landed.
> the ISP parameters at that time, which may be very early if the queue of
> requests is large. If we wait a bit before asking the IPA for ISP
> parameters, algorithms could be more reactive. However, we can't wait
> too long, as the RkISP1 is an inline ISP, we don't have the luxury to
> wait for ISP parameters like with the IPU3.
>
>>>> EventStatReady => processStatsBuffer()
>>>> EventFillParams => processParamsBuffer()
>>> This function doesn't process the buffer, but fills it. How about
>>> fillParamsBuffer() ?
>> Sounds good
>>
>>>> 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 | 132 +++++++++++--------------
>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 123 ++++++++++-------------
>>>> 4 files changed, 129 insertions(+), 181 deletions(-)
>>>>
>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>>>> index 18cdc744..0bd5d641 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;
>>>> @@ -56,9 +30,15 @@ interface IPAIPU3Interface {
>>>> mapBuffers(array<libcamera.IPABuffer> buffers);
>>>> unmapBuffers(array<uint32> ids);
>>>>
>>>> - [async] processEvent(IPU3Event ev);
>>>> + [async] processControls(uint32 frame, libcamera.ControlList controls);
>>>> + [async] processParamsBuffer(uint32 frame, uint32 bufferId);
>>>> + [async] processStatsBuffer(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);
>>>> + paramsBufferReady(uint32 frame);
>>>> + metadataReady(uint32 frame, libcamera.ControlList metadata);
>>>> };
>>>> diff --git a/src/ipa/ipu3/ipu3-ipa-design-guide.rst b/src/ipa/ipu3/ipu3-ipa-design-guide.rst
>>>> index 89c71108..ed4b5ab7 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(), processParamsBuffer(), processStatsBuffer()
>>>> + ▼ ▼ ▼ ▼ │ ▼ │ ▼ │ ▼ │ ▼ (▲) setSensorControls, paramsBufferReady, 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``)
>>>> +- processParamsBuffer()
>>>> +- processControls()
>>>> +- processStatsBuffer()
>>>>
>>>> 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
>>> s/ the$//
>>>
>>> (or add "function" or something similar after ``processParamsBuffer()``)
>>>
>>>> -``EventFillParams`` event, and then passed directly to each algorithm
>>>> +``processParamsBuffer()``, 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
>>>> +``paramsBufferReady`` 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
>>>> +``processStatsBuffer()``. This provides the IPA with an opportunity to
>>> Same here/
>>>
>>>> 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 completed frame is returned back via
>>>> +the ``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
>>> s/ action$//
>>>
>>> (you may want to drop "dedicated" too)
>>>
>>>> -`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 50b52d8b..2c99055d 100644
>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>> @@ -65,8 +65,9 @@ namespace ipa::ipu3 {
>>>> * The IPU3 Pipeline defines an IPU3-specific interface for communication
>>>> * between the PipelineHandler and the IPA module.
>>>> *
>>>> - * We extend the IPAIPU3Interface to implement our algorithms and handle events
>>>> - * from the IPU3 PipelineHandler to satisfy requests from the application.
>>>> + * We extend the IPAIPU3Interface to implement our algorithms and handle
>>>> + * invocations from the IPU3 PipelineHandler to satisfy requests from the
>>> s/invocations/calls/
>>>
>>>> + * application.
>>>> *
>>>> * At initialisation time, a CameraSensorHelper is instantiated to support
>>>> * camera-specific calculations, while the default controls are computed, and
>>>> @@ -81,14 +82,14 @@ namespace ipa::ipu3 {
>>>> * parameter buffer, and adapting the settings of the sensor attached to the
>>>> * IPU3 CIO2 through sensor-specific V4L2 controls.
>>>> *
>>>> - * When the event \a EventFillParams occurs we populate the ImgU parameter
>>>> - * buffer with settings to configure the device in preparation for handling the
>>>> - * frame queued in the Request.
>>>> + * In \a processParamsBuffer(), we populate the ImgU parameter buffer with
>>> No need for \a. It's used by doxygen to change the rendering of the next
>>> word, and we use it for function parameters only. Doxygen will detect
>>> function names thanks to the () and generate links.
>>>
>>>> + * settings to configure the device in preparation for handling the frame
>>>> + * queued in the Request.
>>>> *
>>>> * When the frame has completed processing, the ImgU will generate a statistics
>>>> - * buffer which is given to the IPA as part of the \a EventStatReady event. At
>>>> - * this event we run the algorithms to parse the statistics and cache any
>>>> - * results for the next \a EventFillParams event.
>>>> + * buffer which is given to the IPA \a processStatsBuffer(). In this we run the
>>> Same, replace "\a" with "with".
>>>
>>>> + * algorithms to parse the statistics and cache any results for the next
>>>> + * \a processParamsBuffer() invocation.
>>> s/invocation/call/
>>>
>>>> *
>>>> * The individual algorithms are split into modular components that are called
>>>> * iteratively to allow them to process statistics from the ImgU in a defined
>>>> @@ -143,14 +144,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 processParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
>>>> + void processControls(const uint32_t frame, const ControlList &controls) override;
>>>> + void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,
>>>> + const uint32_t bufferId, const ControlList &sensorControls) override;
>>> Line break after bufferId ?
>>>
>>>>
>>>> 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,61 +510,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
>>> Maybe we could improve this to be a bit more detailed ?
>>>
>>> * \brief Fill and return a buffer with ISP processing parameters for a frame
>>>
>>>> + * \param[in] frame The frame number
>>>> + * \param[in] bufferId Buffer ID
>>> ID of the parameter buffer to fill
>>>
>>>> */
>>>> -void IPAIPU3::processEvent(const IPU3Event &event)
>>>> +void IPAIPU3::processParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>>>> {
>>>> - switch (event.op) {
>>>> - case EventProcessControls: {
>>>> - processControls(event.frame, event.controls);
>>>> - break;
>>>> + auto it = buffers_.find(bufferId);
>>>> + if (it == buffers_.end()) {
>>>> + LOG(IPAIPU3, Error) << "Could not find param buffer!";
>>>> + return;
>>>> }
>>>> - 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());
>>>> + Span<uint8_t> mem = it->second.planes()[0];
>>>> + ipu3_uapi_params *params =
>>>> + reinterpret_cast<ipu3_uapi_params *>(mem.data());
>>>>
>>>> - fillParams(event.frame, params);
>>>> - break;
>>>> - }
>>>> - case EventStatReady: {
>>>> - auto it = buffers_.find(event.bufferId);
>>>> - if (it == buffers_.end()) {
>>>> - LOG(IPAIPU3, Error) << "Could not find stats buffer!";
>>>> - return;
>>>> - }
>>>> + fillParams(frame, params);
>>> How about inlinking the fillParams() function here ? Now that there's no
>>> need for a switch/case on the operation ID, there's little reason to
>>> split the operation in two functions. Same for the other operation
>>> handlers.
>>>
>>> This can be done on top of this patch, to keep it reviewable.
>>>
>>>> +}
>>>>
>>>> - Span<uint8_t> mem = it->second.planes()[0];
>>>> - const ipu3_uapi_stats_3a *stats =
>>>> - reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>>> +/**
>>>> + * \brief Process statistics after ISP completion
>>>> + * \param[in] frame The frame number
>>>> + * \param[in] frameTimestamp Timestamp of the frame
>>>> + * \param[in] bufferId Buffer ID
>>> ID of the statistics buffer
>>>
>>>> + * \param[in] sensorControls Sensor controls
>>>> + */
>>>> +void IPAIPU3::processStatsBuffer(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;
>>>> + }
>>>>
>>>> - int32_t exposure = event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>>>> - int32_t gain = event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>>> + 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 = exposure;
>>>> - context_.frameContext.sensor.gain = camHelper_->gain(gain);
>>>> + 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(event.frame, event.frameTimestamp, stats);
>>>> - break;
>>>> - }
>>>> - default:
>>>> - LOG(IPAIPU3, Error) << "Unknown event " << event.op;
>>>> - break;
>>>> - }
>>>> + parseStatistics(frame, frameTimestamp, stats);
>>>> }
>>>>
>>>> /**
>>>> @@ -570,7 +563,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'. */
>>>> @@ -600,10 +593,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);
>>>> + paramsBufferReady.emit(frame);
>>>> }
>>>>
>>>> /**
>>>> @@ -647,11 +637,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);
>>>> }
>>>>
>>>> /**
>>>> @@ -663,23 +649,19 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>> */
>>>> void IPAIPU3::setControls(unsigned int frame)
>>>> {
>>>> - IPU3Action op;
>>>> - op.op = ActionSetSensorControls;
>>>> -
>>>> int32_t exposure = context_.frameContext.agc.exposure;
>>>> int32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
>>>>
>>>> ControlList ctrls(sensorCtrls_);
>>>> ctrls.set(V4L2_CID_EXPOSURE, exposure);
>>>> ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
>>>> - op.sensorControls = ctrls;
>>>>
>>>> - ControlList lensCtrls(lensCtrls_);
>>>> - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
>>>> - static_cast<int32_t>(context_.frameContext.af.focus));
>>>> - op.lensControls = lensCtrls;
>>>> + ControlList lensCtrls(sensorCtrls_);
>>>> + /*
>>>> + * \todo lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, <lens_position>));
>>>> + */
>>> Doesn't this cause a regression ?
>> ops, yeah I think I ended up making a rebase fiasco. I'll address it.
>>
>>>>
>>>> - queueFrameAction.emit(frame, op);
>>>> + 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 60e01917..75b070bf 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);
>>> s/param/params/
>>>
>>>> + void setSensorControls(unsigned int id, const ControlList &sensorControls,
>>>> + const ControlList &lensControls);
>>>> };
>>>>
>>>> class IPU3CameraConfiguration : public CameraConfiguration
>>>> @@ -871,11 +873,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);
>>>> @@ -1218,7 +1216,9 @@ int IPU3CameraData::loadIPA()
>>>> if (!ipa_)
>>>> return -ENOENT;
>>>>
>>>> - ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);
>>>> + ipa_->setSensorControls.connect(this, &IPU3CameraData::setSensorControls);
>>>> + ipa_->paramsBufferReady.connect(this, &IPU3CameraData::paramFilled);
>>>> + ipa_->metadataReady.connect(this, &IPU3CameraData::metadataReady);
>>>>
>>>> /*
>>>> * Pass the sensor info to the IPA to initialize controls.
>>>> @@ -1253,69 +1253,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->setFocusPosition(focusValue.get<int32_t>());
>>>> + const ControlValue &focusValue =
>>>> + lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
>>> You could make this a single line.
>>>
>>>>
>>>> - 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->setFocusPosition(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);
>>>> }
>>>>
>>>> /* -----------------------------------------------------------------------------
>>>> @@ -1390,11 +1380,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_->processParamsBuffer(info->id, info->paramBuffer->cookie());
>>>> }
>>>>
>>>> void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>>>> @@ -1438,13 +1424,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_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),
>>>> + info->statBuffer->cookie(), info->effectiveSensorControls);
>>>> }
>>>>
>>>> /*
More information about the libcamera-devel
mailing list