[libcamera-devel] [PATCH v4 6/7] pipeline: ipa: raspberrypi: Use IPA cookies
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Oct 28 15:15:27 CEST 2022
Quoting Naushir Patuck (2022-10-28 13:58:00)
> 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?
I think it's more the nature of using this global variable to set which
metadata to read, for events that in my head are 'asynchronous' (even
though they can't be called in parallel).
>
>
> >
> > > 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.
I suspect it's analogous to the controls that were set at the start() ..
but I'm not sure that can be modelled easily, so keeping it as 0 seems
reasonable.
I'm just not sure it would ever make sense to be ... 1052 (some other
number..)
Anyway, I don't think it's crucial here.
> > > 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?
Request sequences are 1 for 1 for every request that is queued. It's
handled by
- https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410
It always starts from zero, and should always increase by one for every
request queued to a Camera object. (It's stored in the CameraData).
It gets reset at Camera::stop() time. Requests after that are
sequenced from zero again.
Note that any requests rejected and not queued will not increment the
sequence number, as they won't make it into the pipeline handler:
- https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113
So actually, that could impact you depending on how you handle IPA
contining through a start stop cycle. Do you expect to reference
metadata from a previous streaming session?
If it's an issue - keep it with your own counter.
--
Kieran
>
> 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
> > >
> >
More information about the libcamera-devel
mailing list