[libcamera-devel] [PATCH v4 6/7] pipeline: ipa: raspberrypi: Use IPA cookies
Naushir Patuck
naush at raspberrypi.com
Fri Oct 28 14:58:00 CEST 2022
Hi Kieran,
Thanks for all the reviews!
On Fri, 28 Oct 2022 at 13:34, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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 ...
>
Perhaps this is better named context_? and ipaCookie could also be renamed
ipaContext?
>
> > 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?
>
Correct, it just assigns a cookie value to the starting conditions.
>
> And perhaps it shouldn't always be zero?
>
It does not strictly have to be 0, but if you consider it analogous to the
frame sequence number, it makes sense for it to.
>
>
> > 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.
>
Yes, this would work, assuming the request sequence number is strictly
monotonically increasing?
Happy to make these changes if it matches closer to the
ipu3/rkisp implementation.
Regards,
Naush
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221028/d88f1480/attachment.htm>
More information about the libcamera-devel
mailing list