<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 28 Oct 2022 at 14:15, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Naushir Patuck (2022-10-28 13:58:00)<br>
> Hi Kieran,<br>
> <br>
> Thanks for all the reviews!<br>
> <br>
> <br>
> On Fri, 28 Oct 2022 at 13:34, Kieran Bingham <<br>
> <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> <br>
> > Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:06)<br>
> > > Pass an IPA cookie from the pipeline handler to the IPA and eventually<br>
> > back to<br>
> > > the pipeline handler through the setDelayedControls signal. This cookie<br>
> > is used<br>
> > > to index the RPiController::Metadata object to be used for the frame.<br>
> > ><br>
> > > The IPA cookie is then returned from DelayedControls when the frame with<br>
> > the<br>
> > > applied controls has been returned from the sensor, and eventually<br>
> > passed back<br>
> > > to the IPA from the signalIspPrepare signal.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > ---<br>
> > > include/libcamera/ipa/raspberrypi.mojom | 4 +++-<br>
> > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++-----<br>
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 19 +++++++++++++------<br>
> > > 3 files changed, 23 insertions(+), 12 deletions(-)<br>
> > ><br>
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom<br>
> > b/include/libcamera/ipa/raspberrypi.mojom<br>
> > > index 40f78d9e3b3f..bb5abd895262 100644<br>
> > > --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> > > @@ -37,6 +37,8 @@ struct ISPConfig {<br>
> > > uint32 bayerBufferId;<br>
> > > bool embeddedBufferPresent;<br>
> > > libcamera.ControlList controls;<br>
> > > + uint32 ipaCookie;<br>
> > > + uint32 delayCookie;<br>
> > > };<br>
> > ><br>
> > > struct IPAConfig {<br>
> > > @@ -137,5 +139,5 @@ interface IPARPiEventInterface {<br>
> > > runIsp(uint32 bufferId);<br>
> > > embeddedComplete(uint32 bufferId);<br>
> > > setIspControls(libcamera.ControlList controls);<br>
> > > - setDelayedControls(libcamera.ControlList controls);<br>
> > > + setDelayedControls(libcamera.ControlList controls, uint32<br>
> > delayCookie);<br>
> > > };<br>
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > index 9e7792f5dfbe..aed8f68aded9 100644<br>
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > @@ -168,6 +168,7 @@ private:<br>
> > > RPiController::Controller controller_;<br>
> > > std::array<RPiController::Metadata, numMetadataContexts><br>
> > rpiMetadata_;<br>
> > > unsigned int metadataIdx_;<br>
> > > + unsigned int lastMetadataIdx_;<br>
> > ><br>
> > > /*<br>
> > > * We count frames to decide if the frame must be hidden (e.g.<br>
> > from<br>
> > > @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls,<br>
> > StartConfig *startConfig)<br>
> > ><br>
> > > firstStart_ = false;<br>
> > > lastRunTimestamp_ = 0;<br>
> > > - metadataIdx_ = 0;<br>
> > > }<br>
> > ><br>
> > > void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)<br>
> > > @@ -535,6 +535,8 @@ void IPARPi::signalIspPrepare(const ISPConfig &data)<br>
> > > * avoid running the control algos for a few frames in case<br>
> > > * they are "unreliable".<br>
> > > */<br>
> > > + lastMetadataIdx_ = metadataIdx_;<br>
> > > + metadataIdx_ = data.ipaCookie % rpiMetadata_.size();<br>
> ><br>
> > I really fear this metadataIdx_ is a code smell ... but I am tempted to<br>
> > put a peg on my nose ...<br>
> ><br>
> <br>
> Perhaps this is better named context_? and ipaCookie could also be renamed<br>
> ipaContext?<br>
<br>
I think it's more the nature of using this global variable to set which<br>
metadata to read, for events that in my head are 'asynchronous' (even<br>
though they can't be called in parallel).<br></blockquote><div><br></div><div>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?</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> <br>
> ><br>
> > > prepareISP(data);<br>
> > > frameCount_++;<br>
> > ><br>
> > > @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int<br>
> > bufferId)<br>
> > > void IPARPi::prepareISP(const ISPConfig &data)<br>
> > > {<br>
> > > int64_t frameTimestamp =<br>
> > data.controls.get(controls::SensorTimestamp).value_or(0);<br>
> > > - RPiController::Metadata lastMetadata;<br>
> > > RPiController::Metadata &rpiMetadata =<br>
> > rpiMetadata_[metadataIdx_];<br>
> > > Span<uint8_t> embeddedBuffer;<br>
> > ><br>
> > > - lastMetadata = std::move(rpiMetadata);<br>
> > > + rpiMetadata.clear();<br>
> > > fillDeviceStatus(data.controls);<br>
> > ><br>
> > > if (data.embeddedBufferPresent) {<br>
> > > @@ -1048,7 +1049,8 @@ void IPARPi::prepareISP(const ISPConfig &data)<br>
> > > * current frame, or any other bits of metadata that<br>
> > were added<br>
> > > * in helper_->Prepare().<br>
> > > */<br>
> > > - rpiMetadata.merge(lastMetadata);<br>
> > > + RPiController::Metadata &lastMetadata =<br>
> > rpiMetadata_[lastMetadataIdx_];<br>
> > > + rpiMetadata.mergeCopy(lastMetadata);<br>
> > > processPending_ = false;<br>
> > > return;<br>
> > > }<br>
> > > @@ -1147,7 +1149,7 @@ void IPARPi::processStats(unsigned int bufferId)<br>
> > > ControlList ctrls(sensorCtrls_);<br>
> > > applyAGC(&agcStatus, ctrls);<br>
> > ><br>
> > > - setDelayedControls.emit(ctrls);<br>
> > > + setDelayedControls.emit(ctrls, metadataIdx_);<br>
> > > }<br>
> > > }<br>
> > ><br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index 23f2460190f4..8f6c6c0ce89f 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -206,7 +206,7 @@ public:<br>
> > > void runIsp(uint32_t bufferId);<br>
> > > void embeddedComplete(uint32_t bufferId);<br>
> > > void setIspControls(const ControlList &controls);<br>
> > > - void setDelayedControls(const ControlList &controls);<br>
> > > + void setDelayedControls(const ControlList &controls, uint32_t<br>
> > delayCookie);<br>
> > > void setSensorControls(ControlList &controls);<br>
> > > void unicamTimeout();<br>
> > ><br>
> > > @@ -262,6 +262,7 @@ public:<br>
> > > struct BayerFrame {<br>
> > > FrameBuffer *buffer;<br>
> > > ControlList controls;<br>
> > > + unsigned int delayCookie;<br>
> > > };<br>
> > ><br>
> > > std::queue<BayerFrame> bayerQueue_;<br>
> > > @@ -294,6 +295,9 @@ public:<br>
> > > /* Have internal buffers been allocated? */<br>
> > > bool buffersAllocated_;<br>
> > ><br>
> > > + /* Frame cookie to pass to the IPA */<br>
> > > + unsigned int ipaCookie_;<br>
> > > +<br>
> > > private:<br>
> > > void checkRequestCompleted();<br>
> > > void fillRequestMetadata(const ControlList &bufferControls,<br>
> > > @@ -1064,7 +1068,8 @@ int PipelineHandlerRPi::start(Camera *camera,<br>
> > const ControlList *controls)<br>
> > > * Reset the delayed controls with the gain and exposure values<br>
> > set by<br>
> > > * the IPA.<br>
> > > */<br>
> > > - data->delayedCtrls_->reset();<br>
> > > + data->ipaCookie_ = 0;<br>
> > > + data->delayedCtrls_->reset(data->ipaCookie_);<br>
> ><br>
> > I don't think I've understood why resetting delayed controls desires a<br>
> > cookie to be given. I guess it's so that the 'starting' condition is<br>
> > known?<br>
> ><br>
> <br>
> Correct, it just assigns a cookie value to the starting conditions.<br>
> <br>
> <br>
> ><br>
> > And perhaps it shouldn't always be zero?<br>
> ><br>
> <br>
> It does not strictly have to be 0, but if you consider it analogous to the<br>
> frame sequence number, it makes sense for it to.<br>
<br>
I suspect it's analogous to the controls that were set at the start() ..<br>
but I'm not sure that can be modelled easily, so keeping it as 0 seems<br>
reasonable.<br>
<br>
I'm just not sure it would ever make sense to be ... 1052 (some other<br>
number..)<br>
<br>
Anyway, I don't think it's crucial here.<br>
<br>
<br>
> > > data->state_ = RPiCameraData::State::Idle;<br>
> > ><br>
> > > @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const<br>
> > ControlList &controls)<br>
> > > handleState();<br>
> > > }<br>
> > ><br>
> > > -void RPiCameraData::setDelayedControls(const ControlList &controls)<br>
> > > +void RPiCameraData::setDelayedControls(const ControlList &controls,<br>
> > uint32_t delayCookie)<br>
> > > {<br>
> > > - if (!delayedCtrls_->push(controls))<br>
> > > + if (!delayedCtrls_->push(controls, delayCookie))<br>
> > > LOG(RPI, Error) << "V4L2 DelayedControl set failed";<br>
> > > handleState();<br>
> > > }<br>
> > > @@ -1867,13 +1872,13 @@ void<br>
> > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> > > * Lookup the sensor controls used for this frame<br>
> > sequence from<br>
> > > * DelayedControl and queue them along with the frame<br>
> > buffer.<br>
> > > */<br>
> > > - auto [ctrl, cookie] =<br>
> > delayedCtrls_->get(buffer->metadata().sequence);<br>
> > > + auto [ctrl, delayCookie] =<br>
> > delayedCtrls_->get(buffer->metadata().sequence);<br>
> > > /*<br>
> > > * Add the frame timestamp to the ControlList for the<br>
> > IPA to use<br>
> > > * as it does not receive the FrameBuffer object.<br>
> > > */<br>
> > > ctrl.set(controls::SensorTimestamp,<br>
> > buffer->metadata().timestamp);<br>
> > > - bayerQueue_.push({ buffer, std::move(ctrl) });<br>
> > > + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie<br>
> > });<br>
> > > } else {<br>
> > > embeddedQueue_.push(buffer);<br>
> > > }<br>
> > > @@ -2168,6 +2173,8 @@ void RPiCameraData::tryRunPipeline()<br>
> > > ipa::RPi::ISPConfig ispPrepare;<br>
> > > ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;<br>
> > > ispPrepare.controls = std::move(bayerFrame.controls);<br>
> > > + ispPrepare.ipaCookie = ipaCookie_++;<br>
> ><br>
> > I'd be intrigued to know if this should be the request sequence number,<br>
> > as we would use in the RKISP/IPU3, rather than a new sequence number.<br>
> ><br>
> <br>
> Yes, this would work, assuming the request sequence number is strictly<br>
> monotonically increasing?<br>
<br>
Request sequences are 1 for 1 for every request that is queued. It's<br>
handled by <br>
<br>
- <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n410</a><br>
<br>
It always starts from zero, and should always increase by one for every<br>
request queued to a Camera object. (It's stored in the CameraData).<br>
<br>
It gets reset at Camera::stop() time. Requests after that are<br>
sequenced from zero again.<br>
<br>
Note that any requests rejected and not queued will not increment the<br>
sequence number, as they won't make it into the pipeline handler:<br>
<br>
- <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1113</a><br>
<br>
<br>
So actually, that could impact you depending on how you handle IPA<br>
contining through a start stop cycle. Do you expect to reference<br>
metadata from a previous streaming session?<br>
<br>
If it's an issue - keep it with your own counter.<br></blockquote><div><br></div><div>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.</div><div>So perhaps it does make sense to switch to request sequence numbers...?</div><div><br></div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> Happy to make these changes if it matches closer to the<br>
> ipu3/rkisp implementation.<br>
> <br>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> > But I suspect it doesn't matter. It depends how you want to reference<br>
> > the completed metadata against a queued request, which I think you are<br>
> > not too worried about explicitly like that.<br>
> ><br>
> > Small worries aside, I expect this is working... so it's your discretion<br>
> > if you think the cookies/sequences should be handled differently.<br>
> ><br>
> > Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > > + ispPrepare.delayCookie = bayerFrame.delayCookie;<br>
> > ><br>
> > > if (embeddedBuffer) {<br>
> > > unsigned int embeddedId =<br>
> > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);<br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
> ><br>
</blockquote></div></div>