[libcamera-devel] [PATCH v4 6/7] pipeline: ipa: raspberrypi: Use IPA cookies
Naushir Patuck
naush at raspberrypi.com
Fri Oct 28 15:26:11 CEST 2022
On Fri, 28 Oct 2022 at 14:15, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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).
>
Storing the metadataIdx_ was a bit of a shortcut really. I could remove
this variable and pass the same "cookie" into the process phase as well.
Would that be cleaner?
However, I do need to store lastMetadataIdx_ if I use the request sequence
number as the cookie, and they do not increment by 1 on each call... see
below.
>
> >
> >
> > >
> > > > 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.
>
We don't need to reference metadata from previous streaming sessions, so
assuming the request counter that the pipeline handler sees are
monotonically increasing by 1, I can switch to that.
So perhaps it does make sense to switch to request sequence numbers...?
Naush
>
> --
> 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
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221028/e8ad626e/attachment.htm>
More information about the libcamera-devel
mailing list