<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 21 Feb 2021 at 22:18, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Feb 18, 2021 at 05:01:23PM +0000, Naushir Patuck wrote:<br>
> When running with sensors that had no embedded data, the pipeline handler<br>
> would fill a dummy embedded data buffer with gain/exposure values, and<br>
> pass this buffer to the IPA together with the bayer buffer. The IPA would<br>
> extract these values for use in the controller algorithms.<br>
> <br>
> Rework this logic entirely by having a new RPiCameraData::BayerFrame<br>
> queue to replace the existing bayer queue. In addition to storing the<br>
> FrameBuffer pointer, this also stores all the controls tracked by<br>
> DelayedControls for that frame in a ControlList. This includes include<br>
> exposure and gain values. On signalling RPi::IPA_EVENT_SIGNAL_ISP_PREPARE<br>
> IPA event, the pipeline handler now passes this ControlList from the<br>
> RPiCameraData::BayerFrame queue.<br>
> <br>
> The IPA now extracts the gain and exposure values from the ControlList<br>
> instead of using RPiController::MdParserRPi to parse the embedded data<br>
> buffer.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 2 +<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 132 +++++++++++-------<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 58 ++++----<br>
> 3 files changed, 108 insertions(+), 84 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index 5a27b1e4fc2d..f733a2cd2a40 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -29,6 +29,8 @@ struct SensorConfig {<br>
> struct ISPConfig {<br>
> uint32 embeddedBufferId;<br>
> uint32 bayerBufferId;<br>
> + bool embeddedBufferPresent;<br>
> + ControlList controls;<br>
> };<br>
> <br>
> struct ConfigInput {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 1226ea514521..f9ab6417866e 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1,6 +1,6 @@<br>
> /* SPDX-License-Identifier: BSD-2-Clause */<br>
> /*<br>
> - * Copyright (C) 2019-2020, Raspberry Pi (Trading) Ltd.<br>
> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.<br>
> *<br>
> * rpi.cpp - Raspberry Pi Image Processing Algorithms<br>
> */<br>
> @@ -101,9 +101,11 @@ private:<br>
> bool validateIspControls();<br>
> void queueRequest(const ControlList &controls);<br>
> void returnEmbeddedBuffer(unsigned int bufferId);<br>
> - void prepareISP(unsigned int bufferId);<br>
> + void prepareISP(const ipa::RPi::ISPConfig &data);<br>
> void reportMetadata();<br>
> bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);<br>
> + void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,<br>
> + struct DeviceStatus &deviceStatus);<br>
> void processStats(unsigned int bufferId);<br>
> void applyFrameDurations(double minFrameDuration, double maxFrameDuration);<br>
> void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);<br>
> @@ -447,7 +449,7 @@ void IPARPi::signalIspPrepare(const ipa::RPi::ISPConfig &data)<br>
> * avoid running the control algos for a few frames in case<br>
> * they are "unreliable".<br>
> */<br>
> - prepareISP(data.embeddedBufferId);<br>
> + prepareISP(data);<br>
> frameCount_++;<br>
> <br>
> /* Ready to push the input buffer into the ISP. */<br>
> @@ -909,67 +911,84 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)<br>
> embeddedComplete.emit(bufferId & ipa::RPi::MaskID);<br>
> }<br>
> <br>
> -void IPARPi::prepareISP(unsigned int bufferId)<br>
> +void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)<br>
> {<br>
> struct DeviceStatus deviceStatus = {};<br>
> - bool success = parseEmbeddedData(bufferId, deviceStatus);<br>
> + bool success = false;<br>
<br>
The changes below are hard to read ('git show -b' helps after applying).<br>
For future patch series, if large changes in indentation can be isolated<br>
in a patch that performs no or very little functional changes, it can<br>
help review.<br></blockquote><div><br></div><div>Apologies, I will keep that in mind for the future.</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>
> - /* Done with embedded data now, return to pipeline handler asap. */<br>
> - returnEmbeddedBuffer(bufferId);<br>
> + if (data.embeddedBufferPresent) {<br>
> + /*<br>
> + * Pipeline handler has supplied us with an embedded data buffer,<br>
> + * so parse it and extract the exposure and gain.<br>
> + */<br>
> + success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);<br>
> <br>
> - if (success) {<br>
> - ControlList ctrls(ispCtrls_);<br>
> + /* Done with embedded data now, return to pipeline handler asap. */<br>
> + returnEmbeddedBuffer(data.embeddedBufferId);<br>
> + }<br>
> <br>
> - rpiMetadata_.Clear();<br>
> - rpiMetadata_.Set("device.status", deviceStatus);<br>
> - controller_.Prepare(&rpiMetadata_);<br>
> + if (!success) {<br>
> + /*<br>
> + * Pipeline handler has not supplied an embedded data buffer,<br>
> + * or embedded data buffer parsing has failed for some reason,<br>
> + * so pull the exposure and gain values from the control list.<br>
> + */<br>
> + int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
> + int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
> + fillDeviceStatus(exposureLines, gainCode, deviceStatus);<br>
> + }<br>
> <br>
> - /* Lock the metadata buffer to avoid constant locks/unlocks. */<br>
> - std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);<br>
> + ControlList ctrls(ispCtrls_);<br>
> <br>
> - AwbStatus *awbStatus = rpiMetadata_.GetLocked<AwbStatus>("awb.status");<br>
> - if (awbStatus)<br>
> - applyAWB(awbStatus, ctrls);<br>
> + rpiMetadata_.Clear();<br>
> + rpiMetadata_.Set("device.status", deviceStatus);<br>
> + controller_.Prepare(&rpiMetadata_);<br>
> <br>
> - CcmStatus *ccmStatus = rpiMetadata_.GetLocked<CcmStatus>("ccm.status");<br>
> - if (ccmStatus)<br>
> - applyCCM(ccmStatus, ctrls);<br>
> + /* Lock the metadata buffer to avoid constant locks/unlocks. */<br>
> + std::unique_lock<RPiController::Metadata> lock(rpiMetadata_);<br>
> <br>
> - AgcStatus *dgStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");<br>
> - if (dgStatus)<br>
> - applyDG(dgStatus, ctrls);<br>
> + AwbStatus *awbStatus = rpiMetadata_.GetLocked<AwbStatus>("awb.status");<br>
> + if (awbStatus)<br>
> + applyAWB(awbStatus, ctrls);<br>
> <br>
> - AlscStatus *lsStatus = rpiMetadata_.GetLocked<AlscStatus>("alsc.status");<br>
> - if (lsStatus)<br>
> - applyLS(lsStatus, ctrls);<br>
> + CcmStatus *ccmStatus = rpiMetadata_.GetLocked<CcmStatus>("ccm.status");<br>
> + if (ccmStatus)<br>
> + applyCCM(ccmStatus, ctrls);<br>
> <br>
> - ContrastStatus *contrastStatus = rpiMetadata_.GetLocked<ContrastStatus>("contrast.status");<br>
> - if (contrastStatus)<br>
> - applyGamma(contrastStatus, ctrls);<br>
> + AgcStatus *dgStatus = rpiMetadata_.GetLocked<AgcStatus>("agc.status");<br>
> + if (dgStatus)<br>
> + applyDG(dgStatus, ctrls);<br>
> <br>
> - BlackLevelStatus *blackLevelStatus = rpiMetadata_.GetLocked<BlackLevelStatus>("black_level.status");<br>
> - if (blackLevelStatus)<br>
> - applyBlackLevel(blackLevelStatus, ctrls);<br>
> + AlscStatus *lsStatus = rpiMetadata_.GetLocked<AlscStatus>("alsc.status");<br>
> + if (lsStatus)<br>
> + applyLS(lsStatus, ctrls);<br>
> <br>
> - GeqStatus *geqStatus = rpiMetadata_.GetLocked<GeqStatus>("geq.status");<br>
> - if (geqStatus)<br>
> - applyGEQ(geqStatus, ctrls);<br>
> + ContrastStatus *contrastStatus = rpiMetadata_.GetLocked<ContrastStatus>("contrast.status");<br>
> + if (contrastStatus)<br>
> + applyGamma(contrastStatus, ctrls);<br>
> <br>
> - DenoiseStatus *denoiseStatus = rpiMetadata_.GetLocked<DenoiseStatus>("denoise.status");<br>
> - if (denoiseStatus)<br>
> - applyDenoise(denoiseStatus, ctrls);<br>
> + BlackLevelStatus *blackLevelStatus = rpiMetadata_.GetLocked<BlackLevelStatus>("black_level.status");<br>
> + if (blackLevelStatus)<br>
> + applyBlackLevel(blackLevelStatus, ctrls);<br>
> <br>
> - SharpenStatus *sharpenStatus = rpiMetadata_.GetLocked<SharpenStatus>("sharpen.status");<br>
> - if (sharpenStatus)<br>
> - applySharpen(sharpenStatus, ctrls);<br>
> + GeqStatus *geqStatus = rpiMetadata_.GetLocked<GeqStatus>("geq.status");<br>
> + if (geqStatus)<br>
> + applyGEQ(geqStatus, ctrls);<br>
> <br>
> - DpcStatus *dpcStatus = rpiMetadata_.GetLocked<DpcStatus>("dpc.status");<br>
> - if (dpcStatus)<br>
> - applyDPC(dpcStatus, ctrls);<br>
> + DenoiseStatus *denoiseStatus = rpiMetadata_.GetLocked<DenoiseStatus>("denoise.status");<br>
> + if (denoiseStatus)<br>
> + applyDenoise(denoiseStatus, ctrls);<br>
> <br>
> - if (!ctrls.empty())<br>
> - setIspControls.emit(ctrls);<br>
> - }<br>
> + SharpenStatus *sharpenStatus = rpiMetadata_.GetLocked<SharpenStatus>("sharpen.status");<br>
> + if (sharpenStatus)<br>
> + applySharpen(sharpenStatus, ctrls);<br>
> +<br>
> + DpcStatus *dpcStatus = rpiMetadata_.GetLocked<DpcStatus>("dpc.status");<br>
> + if (dpcStatus)<br>
> + applyDPC(dpcStatus, ctrls);<br>
> +<br>
> + if (!ctrls.empty())<br>
> + setIspControls.emit(ctrls);<br>
> }<br>
> <br>
> bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)<br>
> @@ -985,6 +1004,7 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic<br>
> RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());<br>
> if (status != RPiController::MdParser::Status::OK) {<br>
> LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;<br>
> + return false;<br>
> } else {<br>
> uint32_t exposureLines, gainCode;<br>
> if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {<br>
> @@ -992,21 +1012,29 @@ bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &devic<br>
> return false;<br>
> }<br>
> <br>
> - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);<br>
> if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {<br>
> LOG(IPARPI, Error) << "Gain failed";<br>
> return false;<br>
> }<br>
> <br>
> - deviceStatus.analogue_gain = helper_->Gain(gainCode);<br>
> - LOG(IPARPI, Debug) << "Metadata - Exposure : "<br>
> - << deviceStatus.shutter_speed << " Gain : "<br>
> - << deviceStatus.analogue_gain;<br>
> + fillDeviceStatus(exposureLines, gainCode, deviceStatus);<br>
> }<br>
> <br>
> return true;<br>
> }<br>
> <br>
> +void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,<br>
> + struct DeviceStatus &deviceStatus)<br>
> +{<br>
> + deviceStatus.shutter_speed = helper_->Exposure(exposureLines);<br>
> + deviceStatus.analogue_gain = helper_->Gain(gainCode);<br>
> +<br>
> + LOG(IPARPI, Debug) << "Metadata - Exposure : "<br>
> + << deviceStatus.shutter_speed<br>
> + << " Gain : "<br>
> + << deviceStatus.analogue_gain;<br>
> +}<br>
> +<br>
> void IPARPi::processStats(unsigned int bufferId)<br>
> {<br>
> auto it = buffers_.find(bufferId);<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index ba74ace183db..b43d86166c63 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1,6 +1,6 @@<br>
> /* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> /*<br>
> - * Copyright (C) 2019-2020, Raspberry Pi (Trading) Ltd.<br>
> + * Copyright (C) 2019-2021, Raspberry Pi (Trading) Ltd.<br>
> *<br>
> * raspberrypi.cpp - Pipeline handler for Raspberry Pi devices<br>
> */<br>
> @@ -197,7 +197,13 @@ public:<br>
> */<br>
> enum class State { Stopped, Idle, Busy, IpaComplete };<br>
> State state_;<br>
> - std::queue<FrameBuffer *> bayerQueue_;<br>
> +<br>
> + struct BayerFrame {<br>
> + FrameBuffer *buffer;<br>
> + ControlList controls;<br>
> + };<br>
> +<br>
> + std::queue<BayerFrame> bayerQueue_;<br>
> std::queue<FrameBuffer *> embeddedQueue_;<br>
> std::deque<Request *> requestQueue_;<br>
> <br>
> @@ -222,7 +228,7 @@ public:<br>
> private:<br>
> void checkRequestCompleted();<br>
> void tryRunPipeline();<br>
> - bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer);<br>
> + bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer);<br>
> <br>
> unsigned int ispOutputCount_;<br>
> };<br>
> @@ -1383,29 +1389,14 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> << ", timestamp: " << buffer->metadata().timestamp;<br>
> <br>
> if (stream == &unicam_[Unicam::Image]) {<br>
> - bayerQueue_.push(buffer);<br>
> - } else {<br>
> - embeddedQueue_.push(buffer);<br>
> -<br>
> - ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);<br>
> -<br>
> /*<br>
> - * Sensor metadata is unavailable, so put the expected ctrl<br>
> - * values (accounting for the staggered delays) into the empty<br>
> - * metadata buffer.<br>
> + * Lookup the sensor controls used for this frame sequence from<br>
> + * StaggeredCtrl and queue them along with the frame buffer.<br>
<br>
s/StaggeredCtrl/DelayedControls/ ?<br>
<br>
> */<br>
> - if (!sensorMetadata_) {<br>
> - unsigned int bufferId = unicam_[Unicam::Embedded].getBufferId(buffer);<br>
> - auto it = mappedEmbeddedBuffers_.find(bufferId);<br>
> - if (it != mappedEmbeddedBuffers_.end()) {<br>
> - uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data());<br>
> - mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
> - mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
> - } else {<br>
> - LOG(RPI, Warning) << "Failed to find embedded buffer "<br>
> - << bufferId;<br>
> - }<br>
> - }<br>
> + ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + bayerQueue_.push({ buffer, std::move(ctrl) });<br>
> + } else {<br>
> + embeddedQueue_.push(buffer);<br>
> }<br>
> <br>
> handleState();<br>
> @@ -1656,14 +1647,15 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)<br>
> <br>
> void RPiCameraData::tryRunPipeline()<br>
> {<br>
> - FrameBuffer *bayerBuffer, *embeddedBuffer;<br>
> + FrameBuffer *embeddedBuffer;<br>
> + BayerFrame bayerFrame;<br>
> <br>
> /* If any of our request or buffer queues are empty, we cannot proceed. */<br>
> if (state_ != State::Idle || requestQueue_.empty() ||<br>
> bayerQueue_.empty() || embeddedQueue_.empty())<br>
> return;<br>
> <br>
> - if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))<br>
> + if (!findMatchingBuffers(bayerFrame, embeddedBuffer))<br>
> return;<br>
> <br>
> /* Take the first request from the queue and action the IPA. */<br>
> @@ -1682,7 +1674,7 @@ void RPiCameraData::tryRunPipeline()<br>
> /* Set our state to say the pipeline is active. */<br>
> state_ = State::Busy;<br>
> <br>
> - unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);<br>
> + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);<br>
> unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);<br>
> <br>
> LOG(RPI, Debug) << "Signalling signalIspPrepare:"<br>
> @@ -1692,24 +1684,26 @@ void RPiCameraData::tryRunPipeline()<br>
> ipa::RPi::ISPConfig ispPrepare;<br>
> ispPrepare.embeddedBufferId = ipa::RPi::MaskEmbeddedData | embeddedId;<br>
> ispPrepare.bayerBufferId = ipa::RPi::MaskBayerData | bayerId;<br>
> + ispPrepare.embeddedBufferPresent = true;<br>
<br>
Shouldn't the be conditioned by embedded data being present ?<br></blockquote><div><br></div><div>Yes, good catch! It is conditional on the next patches, but should be here as well.</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>
> + ispPrepare.controls = std::move(bayerFrame.controls);<br>
> ipa_->signalIspPrepare(ispPrepare);<br>
> }<br>
> <br>
> -bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)<br>
> +bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)<br>
> {<br>
> unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;<br>
> <br>
> /* Loop until we find a matching bayer and embedded data buffer. */<br>
> while (!bayerQueue_.empty()) {<br>
> /* Start with the front of the bayer queue. */<br>
> - bayerBuffer = bayerQueue_.front();<br>
> + bayerFrame = bayerQueue_.front();<br>
<br>
This will create a copy of the ControlList. Is there a way the logic in<br>
this function could be reworked to allow an std::move() here ?<br></blockquote><div><br></div><div>Good point. I think it should be easy enough to allow a std::move() here.</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>
> * Find the embedded data buffer with a matching timestamp to pass to<br>
> * the IPA. Any embedded buffers with a timestamp lower than the<br>
> * current bayer buffer will be removed and re-queued to the driver.<br>
> */<br>
> - uint64_t ts = bayerBuffer->metadata().timestamp;<br>
> + uint64_t ts = bayerFrame.buffer->metadata().timestamp;<br>
> embeddedBuffer = nullptr;<br>
> while (!embeddedQueue_.empty()) {<br>
> FrameBuffer *b = embeddedQueue_.front();<br>
> @@ -1739,7 +1733,7 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *<br>
> * the front of the queue. This buffer is now orphaned, so requeue<br>
> * it back to the device.<br>
> */<br>
> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);<br>
> bayerQueue_.pop();<br>
> bayerRequeueCount++;<br>
> LOG(RPI, Warning) << "Dropping unmatched input frame in stream "<br>
> @@ -1757,7 +1751,7 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *<br>
> <br>
> LOG(RPI, Warning) << "Flushing bayer stream!";<br>
> while (!bayerQueue_.empty()) {<br>
> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front().buffer);<br>
> bayerQueue_.pop();<br>
> }<br>
> flushedBuffers = true;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>