<div dir="ltr"><div dir="ltr">Hi Paul,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 17 Feb 2021 at 08:56, <<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 08:28:37AM +0000, Naushir Patuck wrote:<br>
> Hi all,<br>
> <br>
> Sorry, I did have one more question on the new IPA interface topic.  In<br>
> raspberrypi.mojom, the module namespace is defined as module "ipa.rpi".<br>
> <br>
> Would it make sense to rename this to "ipa.RPi" so that it is consistent with<br>
> the "RPi" namespace used elsewhere?<br>
<br>
There's nothing technical preventing it. I just chose that<br>
capitalization arbitrarily and nobody objected.<br></blockquote><div><br></div><div>I'll change it to RPi then, just to keep the consistency.  Â  Sorry I did not flag this in your review rounds.</div><div><br></div><div>Regards,</div><div>Naush</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>
> On Wed, 17 Feb 2021 at 08:02, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
> <br>
>  Â  Â This commit addresses a few fixes and tidy-ups after the IPAInterface<br>
>  Â  Â rework:<br>
>  Â  Â - Rename ConfigStaggeredWrite -> ConfigSensorParams<br>
>  Â  Â - Rename setIsp -> setIspControls<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<br>
>  Â  Â C++-only API")<br>
>  Â  Â ---<br>
>  Â  Â Â include/libcamera/ipa/raspberrypi.mojom  Â  Â  Â |  4 +-<br>
>  Â  Â Â src/ipa/raspberrypi/raspberrypi.cpp  Â  Â  Â  Â  Â |  4 +-<br>
>  Â  Â Â .../pipeline/raspberrypi/raspberrypi.cpp  Â  Â  | 91 ++++++++++---------<br>
>  Â  Â Â 3 files changed, 50 insertions(+), 49 deletions(-)<br>
> <br>
>  Â  Â diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/<br>
>  Â  Â ipa/raspberrypi.mojom<br>
>  Â  Â index bab19a946e18..8a256d022270 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>
>  Â  Â @@ -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/<br>
>  Â  Â raspberrypi.cpp<br>
>  Â  Â index 81a3195c82e9..0bf88bcc5f72 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 &<br>
>  Â  Â 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>
>  Â  Â @@ -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/<br>
>  Â  Â libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>  Â  Â index 15aa600ed581..d0ca015a01dc 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 &<br>
>  Â  Â 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, &<br>
>  Â  Â RPiCameraData::statsMetadataComplete);<br>
>  Â  Â Â  Â  Â  Â  ipa_->runIsp.connect(this, &RPiCameraData::runIsp);<br>
>  Â  Â Â  Â  Â  Â  ipa_->embeddedComplete.connect(this, &<br>
>  Â  Â RPiCameraData::embeddedComplete);<br>
>  Â  Â -  Â  Â  Â ipa_->setIsp.connect(this, &RPiCameraData::setIsp);<br>
>  Â  Â +  Â  Â  Â ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);<br>
>  Â  Â Â  Â  Â  Â  ipa_->setDelayedControls.connect(this, &<br>
>  Â  Â RPiCameraData::setDelayedControls);<br>
> <br>
>  Â  Â Â  Â  Â  Â  IPASettings settings(ipa_->configurationFile(sensor_->model() +<br>
>  Â  Â ".json"));<br>
>  Â  Â @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const<br>
>  Â  Â 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>
>  Â  Â @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const<br>
>  Â  Â CameraConfiguration *config)<br>
> <br>
>  Â  Â Â void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const<br>
>  Â  Â ControlList &controls)<br>
>  Â  Â Â {<br>
>  Â  Â -  Â  Â  Â if (state_ == State::Stopped)<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â handleState();<br>
>  Â  Â +  Â  Â  Â if (state_ != State::Stopped) {<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at<br>
>  Â  Â (bufferId);<br>
> <br>
>  Â  Â -  Â  Â  Â FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â handleStreamBuffer(buffer, &isp_[Isp::Stats]);<br>
> <br>
>  Â  Â -  Â  Â  Â handleStreamBuffer(buffer, &isp_[Isp::Stats]);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â /* Fill the Request metadata buffer with what the IPA has<br>
>  Â  Â provided */<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â Request *request = requestQueue_.front();<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â request->metadata() = std::move(controls);<br>
> <br>
>  Â  Â -  Â  Â  Â /* Fill the Request metadata buffer with what the IPA has provided<br>
>  Â  Â */<br>
>  Â  Â -  Â  Â  Â Request *request = requestQueue_.front();<br>
>  Â  Â -  Â  Â  Â request->metadata() = std::move(controls);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â /*<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â * Also update the ScalerCrop in the metadata with what we<br>
>  Â  Â actually<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â * used. But we must first rescale that from ISP (camera<br>
>  Â  Â mode) pixels<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â * back into sensor native pixels.<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â *<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â * Sending this information on every frame may be helpful.<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â */<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â if (updateScalerCrop_) {<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â updateScalerCrop_ = false;<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â scalerCrop_ = ispCrop_.scaledBy<br>
>  Â  Â (sensorInfo_.analogCrop.size(),<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â <br>
>  Â  Â Â sensorInfo_.outputSize);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â scalerCrop_.translateBy<br>
>  Â  Â (sensorInfo_.analogCrop.topLeft());<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â }<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â request->metadata().set(controls::ScalerCrop, scalerCrop_);<br>
> <br>
>  Â  Â -  Â  Â  Â /*<br>
>  Â  Â -  Â  Â  Â  * Also update the ScalerCrop in the metadata with what we actually<br>
>  Â  Â -  Â  Â  Â  * used. But we must first rescale that from ISP (camera mode)<br>
>  Â  Â pixels<br>
>  Â  Â -  Â  Â  Â  * back into sensor native pixels.<br>
>  Â  Â -  Â  Â  Â  *<br>
>  Â  Â -  Â  Â  Â  * Sending this information on every frame may be helpful.<br>
>  Â  Â -  Â  Â  Â  */<br>
>  Â  Â -  Â  Â  Â if (updateScalerCrop_) {<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â updateScalerCrop_ = false;<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size<br>
>  Â  Â (),<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sensorInfo_.outputSize);<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â state_ = State::IpaComplete;<br>
>  Â  Â Â  Â  Â  Â  }<br>
>  Â  Â -  Â  Â  Â request->metadata().set(controls::ScalerCrop, scalerCrop_);<br>
>  Â  Â -<br>
>  Â  Â -  Â  Â  Â state_ = State::IpaComplete;<br>
> <br>
>  Â  Â Â  Â  Â  Â  handleState();<br>
>  Â  Â Â }<br>
> <br>
>  Â  Â Â void RPiCameraData::runIsp(uint32_t bufferId)<br>
>  Â  Â Â {<br>
>  Â  Â -  Â  Â  Â if (state_ == State::Stopped)<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â handleState();<br>
>  Â  Â +  Â  Â  Â if (state_ != State::Stopped) {<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers<br>
>  Â  Â ().at(bufferId);<br>
> <br>
>  Â  Â -  Â  Â  Â FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at<br>
>  Â  Â (bufferId);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " <<<br>
>  Â  Â bufferId<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â << ", timestamp: " << buffer->metadata<br>
>  Â  Â ().timestamp;<br>
> <br>
>  Â  Â -  Â  Â  Â LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â << ", timestamp: " << buffer->metadata().timestamp;<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â isp_[Isp::Input].queueBuffer(buffer);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â ispOutputCount_ = 0;<br>
>  Â  Â +  Â  Â  Â }<br>
> <br>
>  Â  Â -  Â  Â  Â isp_[Isp::Input].queueBuffer(buffer);<br>
>  Â  Â -  Â  Â  Â ispOutputCount_ = 0;<br>
>  Â  Â Â  Â  Â  Â  handleState();<br>
>  Â  Â Â }<br>
> <br>
>  Â  Â Â void RPiCameraData::embeddedComplete(uint32_t bufferId)<br>
>  Â  Â Â {<br>
>  Â  Â -  Â  Â  Â if (state_ == State::Stopped)<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â handleState();<br>
>  Â  Â +  Â  Â  Â if (state_ != State::Stopped) {<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers<br>
>  Â  Â ().at(bufferId);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);<br>
>  Â  Â +  Â  Â  Â }<br>
> <br>
>  Â  Â -  Â  Â  Â FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at<br>
>  Â  Â (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>
>  Â  Â ());<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<br>
>  Â  Â (V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â bcm2835_isp_lens_shading ls =<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â *reinterpret_cast<const bcm2835_isp_lens_shading *><br>
>  Â  Â (s.data());<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â ls.dmabuf = lsTable_.fd();<br>
> <br>
>  Â  Â -  Â  Â  Â ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&<br>
>  Â  Â ls),<br>
>  Â  Â -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â sizeof(ls) });<br>
>  Â  Â -  Â  Â  Â ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);<br>
>  Â  Â +  Â  Â  Â  Â  Â  Â  Â ControlValue c(Span<const uint8_t>{ reinterpret_cast<br>
>  Â  Â <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>
>  Â  Â --<br>
>  Â  Â 2.25.1<br>
> <br>
> <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>
<br>
</blockquote></div></div>