[libcamera-devel] [PATCH v4 6/7] pipeline: ipa: raspberrypi: Use IPA cookies
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Oct 28 14:34:42 CEST 2022
Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06)
> Pass an IPA cookie from the pipeline handler to the IPA and eventually back to
> the pipeline handler through the setDelayedControls signal. This cookie is used
> to index the RPiController::Metadata object to be used for the frame.
>
> The IPA cookie is then returned from DelayedControls when the frame with the
> applied controls has been returned from the sensor, and eventually passed back
> to the IPA from the signalIspPrepare signal.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> include/libcamera/ipa/raspberrypi.mojom | 4 +++-
> src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++-----
> .../pipeline/raspberrypi/raspberrypi.cpp | 19 +++++++++++++------
> 3 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 40f78d9e3b3f..bb5abd895262 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -37,6 +37,8 @@ struct ISPConfig {
> uint32 bayerBufferId;
> bool embeddedBufferPresent;
> libcamera.ControlList controls;
> + uint32 ipaCookie;
> + uint32 delayCookie;
> };
>
> struct IPAConfig {
> @@ -137,5 +139,5 @@ interface IPARPiEventInterface {
> runIsp(uint32 bufferId);
> embeddedComplete(uint32 bufferId);
> setIspControls(libcamera.ControlList controls);
> - setDelayedControls(libcamera.ControlList controls);
> + setDelayedControls(libcamera.ControlList controls, uint32 delayCookie);
> };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9e7792f5dfbe..aed8f68aded9 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -168,6 +168,7 @@ private:
> RPiController::Controller controller_;
> std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;
> unsigned int metadataIdx_;
> + unsigned int lastMetadataIdx_;
>
> /*
> * We count frames to decide if the frame must be hidden (e.g. from
> @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>
> firstStart_ = false;
> lastRunTimestamp_ = 0;
> - metadataIdx_ = 0;
> }
>
> void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig &data)
> * avoid running the control algos for a few frames in case
> * they are "unreliable".
> */
> + lastMetadataIdx_ = metadataIdx_;
> + metadataIdx_ = data.ipaCookie % rpiMetadata_.size();
I really fear this metadataIdx_ is a code smell ... but I am tempted to
put a peg on my nose ...
> prepareISP(data);
> frameCount_++;
>
> @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> void IPARPi::prepareISP(const ISPConfig &data)
> {
> int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> - RPiController::Metadata lastMetadata;
> RPiController::Metadata &rpiMetadata = rpiMetadata_[metadataIdx_];
> Span<uint8_t> embeddedBuffer;
>
> - lastMetadata = std::move(rpiMetadata);
> + rpiMetadata.clear();
> fillDeviceStatus(data.controls);
>
> if (data.embeddedBufferPresent) {
> @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data)
> * current frame, or any other bits of metadata that were added
> * in helper_->Prepare().
> */
> - rpiMetadata.merge(lastMetadata);
> + RPiController::Metadata &lastMetadata = rpiMetadata_[lastMetadataIdx_];
> + rpiMetadata.mergeCopy(lastMetadata);
> processPending_ = false;
> return;
> }
> @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int bufferId)
> ControlList ctrls(sensorCtrls_);
> applyAGC(&agcStatus, ctrls);
>
> - setDelayedControls.emit(ctrls);
> + setDelayedControls.emit(ctrls, metadataIdx_);
> }
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 23f2460190f4..8f6c6c0ce89f 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -206,7 +206,7 @@ public:
> void runIsp(uint32_t bufferId);
> void embeddedComplete(uint32_t bufferId);
> void setIspControls(const ControlList &controls);
> - void setDelayedControls(const ControlList &controls);
> + void setDelayedControls(const ControlList &controls, uint32_t delayCookie);
> void setSensorControls(ControlList &controls);
> void unicamTimeout();
>
> @@ -262,6 +262,7 @@ public:
> struct BayerFrame {
> FrameBuffer *buffer;
> ControlList controls;
> + unsigned int delayCookie;
> };
>
> std::queue<BayerFrame> bayerQueue_;
> @@ -294,6 +295,9 @@ public:
> /* Have internal buffers been allocated? */
> bool buffersAllocated_;
>
> + /* Frame cookie to pass to the IPA */
> + unsigned int ipaCookie_;
> +
> private:
> void checkRequestCompleted();
> void fillRequestMetadata(const ControlList &bufferControls,
> @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> * Reset the delayed controls with the gain and exposure values set by
> * the IPA.
> */
> - data->delayedCtrls_->reset();
> + data->ipaCookie_ = 0;
> + data->delayedCtrls_->reset(data->ipaCookie_);
I don't think I've understood why resetting delayed controls desires a
cookie to be given. I guess it's so that the 'starting' condition is
known?
And perhaps it shouldn't always be zero?
> data->state_ = RPiCameraData::State::Idle;
>
> @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const ControlList &controls)
> handleState();
> }
>
> -void RPiCameraData::setDelayedControls(const ControlList &controls)
> +void RPiCameraData::setDelayedControls(const ControlList &controls, uint32_t delayCookie)
> {
> - if (!delayedCtrls_->push(controls))
> + if (!delayedCtrls_->push(controls, delayCookie))
> LOG(RPI, Error) << "V4L2 DelayedControl set failed";
> handleState();
> }
> @@ -1867,13 +1872,13 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> * Lookup the sensor controls used for this frame sequence from
> * DelayedControl and queue them along with the frame buffer.
> */
> - auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence);
> + auto [ctrl, delayCookie] = delayedCtrls_->get(buffer->metadata().sequence);
> /*
> * Add the frame timestamp to the ControlList for the IPA to use
> * as it does not receive the FrameBuffer object.
> */
> ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp);
> - bayerQueue_.push({ buffer, std::move(ctrl) });
> + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie });
> } else {
> embeddedQueue_.push(buffer);
> }
> @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline()
> ipa::RPi::ISPConfig ispPrepare;
> ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;
> ispPrepare.controls = std::move(bayerFrame.controls);
> + ispPrepare.ipaCookie = ipaCookie_++;
I'd be intrigued to know if this should be the request sequence number,
as we would use in the RKISP/IPU3, rather than a new sequence number.
But I suspect it doesn't matter. It depends how you want to reference
the completed metadata against a queued request, which I think you are
not too worried about explicitly like that.
Small worries aside, I expect this is working... so it's your discretion
if you think the cookies/sequences should be handled differently.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + ispPrepare.delayCookie = bayerFrame.delayCookie;
>
> if (embeddedBuffer) {
> unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> --
> 2.25.1
>
More information about the libcamera-devel
mailing list