<div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 17 Feb 2021 at 10:18, <<a href="mailto:paul.elder@ideasonboard.com">paul.elder@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>
On Wed, Feb 17, 2021 at 10:08:50AM +0000, Naushir Patuck wrote:<br>
> This commit addresses a few fixes and tidy-ups after the IPAInterface<br>
> rework:<br>
> - Rename ConfigStaggeredWrite -> ConfigSensorParams<br>
> - Rename setIsp -> setIspControls<br>
> - Switch to camel case for ISPConfig::embeddedbufferId and<br>
> ISPConfig::bayerbufferId.<br>
> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()<br>
> should only run when state != Stopped. This matches the behavior of<br>
> the original code. Without this, start/stop cycles may cause errors.<br>
> - In setIspControls(), only update the LS config (with the fd) when a LS<br>
> control is present.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new C++-only API")<br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 8 ++---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------<br>
> 3 files changed, 27 insertions(+), 25 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index bab19a946e18..9c05cc68cceb 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -16,7 +16,7 @@ enum BufferMask {<br>
> const uint32 MaxLsGridSize = 0x8000;<br>
> <br>
> enum ConfigOutputParameters {<br>
> - ConfigStaggeredWrite = 0x01,<br>
> + ConfigSensorParams = 0x01,<br>
> };<br>
> <br>
> struct SensorConfig {<br>
> @@ -27,8 +27,8 @@ struct SensorConfig {<br>
> };<br>
> <br>
> struct ISPConfig {<br>
> - uint32 embeddedbufferId;<br>
> - uint32 bayerbufferId;<br>
> + uint32 embeddedBufferId;<br>
> + uint32 bayerBufferId;<br>
> };<br>
> <br>
> struct ConfigInput {<br>
> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {<br>
> statsMetadataComplete(uint32 bufferId, ControlList controls);<br>
> runIsp(uint32 bufferId);<br>
> embeddedComplete(uint32 bufferId);<br>
> - setIsp(ControlList controls);<br>
> + setIspControls(ControlList controls);<br>
> setDelayedControls(ControlList controls);<br>
> };<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 81a3195c82e9..974f4ec63058 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> helper_->GetDelays(exposureDelay, gainDelay);<br>
> sensorMetadata = helper_->SensorEmbeddedDataPresent();<br>
> <br>
> - result->params |= ipa::rpi::ConfigStaggeredWrite;<br>
> + result->params |= ipa::rpi::ConfigSensorParams;<br>
> result->sensorConfig.gainDelay = gainDelay;<br>
> result->sensorConfig.exposureDelay = exposureDelay;<br>
> result->sensorConfig.vblank = exposureDelay;<br>
> @@ -447,11 +447,11 @@ 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.embeddedBufferId);<br>
> frameCount_++;<br>
> <br>
> /* Ready to push the input buffer into the ISP. */<br>
> - runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);<br>
> + runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);<br>
> }<br>
> <br>
> void IPARPi::reportMetadata()<br>
> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)<br>
> applyDPC(dpcStatus, ctrls);<br>
> <br>
> if (!ctrls.empty())<br>
> - setIsp.emit(ctrls);<br>
> + setIspControls.emit(ctrls);<br>
> }<br>
> }<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 15aa600ed581..8770ae66a21a 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -152,7 +152,7 @@ public:<br>
> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);<br>
> void runIsp(uint32_t bufferId);<br>
> void embeddedComplete(uint32_t bufferId);<br>
> - void setIsp(const ControlList &controls);<br>
> + void setIspControls(const ControlList &controls);<br>
> void setDelayedControls(const ControlList &controls);<br>
> <br>
> /* bufferComplete signal handlers. */<br>
> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()<br>
> ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);<br>
> ipa_->runIsp.connect(this, &RPiCameraData::runIsp);<br>
> ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);<br>
> - ipa_->setIsp.connect(this, &RPiCameraData::setIsp);<br>
> + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);<br>
> ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);<br>
> <br>
> IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));<br>
> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> return -EPIPE;<br>
> }<br>
> <br>
> - if (result.params & ipa::rpi::ConfigStaggeredWrite) {<br>
> + if (result.params & ipa::rpi::ConfigSensorParams) {<br>
> /*<br>
> * Setup our delayed control writer with the sensor default<br>
> * gain and exposure delays.<br>
> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)<br>
> {<br>
> if (state_ == State::Stopped)<br>
> - handleState();<br>
> + return;<br>
<br>
I thought handleState() still needs to be called in this (and below) case?<br></blockquote><div><br></div><div>I thought so too :)</div><div><br></div><div>But if you have a look at handleState(), in the case State::Stopped, it simply breaks and returns doing nothing.</div><div>So, replacing by a single return is sufficient. Clearly it did do something in the past, but we should be ok without</div><div>calling handleState now.</div><div><br></div><div>Regards,</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>
Paul<br>
<br>
> <br>
> FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);<br>
> <br>
> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &<br>
> void RPiCameraData::runIsp(uint32_t bufferId)<br>
> {<br>
> if (state_ == State::Stopped)<br>
> - handleState();<br>
> + return;<br>
> <br>
> FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);<br>
> <br>
> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)<br>
> void RPiCameraData::embeddedComplete(uint32_t bufferId)<br>
> {<br>
> if (state_ == State::Stopped)<br>
> - handleState();<br>
> + return;<br>
> <br>
> FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);<br>
> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);<br>
> handleState();<br>
> }<br>
> <br>
> -void RPiCameraData::setIsp(const ControlList &controls)<br>
> +void RPiCameraData::setIspControls(const ControlList &controls)<br>
> {<br>
> ControlList ctrls = controls;<br>
> <br>
> - Span<const uint8_t> s =<br>
> - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();<br>
> - bcm2835_isp_lens_shading ls =<br>
> - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());<br>
> - ls.dmabuf = lsTable_.fd();<br>
> + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {<br>
> + Span<const uint8_t> s =<br>
> + ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();<br>
> + bcm2835_isp_lens_shading ls =<br>
> + *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());<br>
> + ls.dmabuf = lsTable_.fd();<br>
> <br>
> - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),<br>
> - sizeof(ls) });<br>
> - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);<br>
> + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),<br>
> + sizeof(ls) });<br>
> + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);<br>
> + }<br>
> <br>
> isp_[Isp::Input].dev()->setControls(&ctrls);<br>
> handleState();<br>
> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()<br>
> << " Embedded buffer id: " << embeddedId;<br>
> <br>
> ipa::rpi::ISPConfig ispPrepare;<br>
> - ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;<br>
> - ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;<br>
> + ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;<br>
> + ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;<br>
> ipa_->signalIspPrepare(ispPrepare);<br>
> }<br>
> <br>
> -- <br>
> 2.25.1<br>
> <br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>