<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thanks for all the reviews!</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 28 Oct 2022 at 13:34, 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 via libcamera-devel (2022-10-19 10:01:06)<br>
> Pass an IPA cookie from the pipeline handler to the IPA and eventually back to<br>
> the pipeline handler through the setDelayedControls signal. This cookie 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 the<br>
> applied controls has been returned from the sensor, and eventually 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 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 delayCookie);<br>
> };<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp 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> 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. from<br>
> @@ -324,7 +325,6 @@ void IPARPi::start(const ControlList &controls, 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></blockquote><div><br></div><div>Perhaps this is better named context_? and ipaCookie could also be renamed ipaContext?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> prepareISP(data);<br>
> frameCount_++;<br>
> <br>
> @@ -1011,11 +1013,10 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)<br>
> void IPARPi::prepareISP(const ISPConfig &data)<br>
> {<br>
> int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);<br>
> - RPiController::Metadata lastMetadata;<br>
> RPiController::Metadata &rpiMetadata = 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 were added<br>
> * in helper_->Prepare().<br>
> */<br>
> - rpiMetadata.merge(lastMetadata);<br>
> + RPiController::Metadata &lastMetadata = 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 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 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, const ControlList *controls)<br>
> * Reset the delayed controls with the gain and exposure values 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></blockquote><div><br></div><div>Correct, it just assigns a cookie value to the starting conditions.</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>
And perhaps it shouldn't always be zero?<br></blockquote><div><br></div><div>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.</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>
> data->state_ = RPiCameraData::State::Idle;<br>
> <br>
> @@ -1792,9 +1797,9 @@ void RPiCameraData::setIspControls(const ControlList &controls)<br>
> handleState();<br>
> }<br>
> <br>
> -void RPiCameraData::setDelayedControls(const ControlList &controls)<br>
> +void RPiCameraData::setDelayedControls(const ControlList &controls, 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 RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> * Lookup the sensor controls used for this frame sequence from<br>
> * DelayedControl and queue them along with the frame buffer.<br>
> */<br>
> - auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + auto [ctrl, delayCookie] = delayedCtrls_->get(buffer->metadata().sequence);<br>
> /*<br>
> * Add the frame timestamp to the ControlList for the IPA to use<br>
> * as it does not receive the FrameBuffer object.<br>
> */<br>
> ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp);<br>
> - bayerQueue_.push({ buffer, std::move(ctrl) });<br>
> + bayerQueue_.push({ buffer, std::move(ctrl), delayCookie });<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></blockquote><div><br></div><div>Yes, this would work, assuming the request sequence number is strictly monotonically increasing?</div><div><br></div><div>Happy to make these changes if it matches closer to the ipu3/rkisp implementation.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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 = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>