<div dir="ltr"><div dir="ltr">Hi all,<div><br></div><div>This change addresses some tidying up after the IPA interface changes. However, I do have a question, see below.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 17 Feb 2021 at 08:02, Naushir Patuck <<a href="mailto:naush@raspberrypi.com">naush@raspberrypi.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">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 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/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/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 &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/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 &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>
@@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
<br>
void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)<br>
{<br>
- if (state_ == State::Stopped)<br>
- handleState();<br>
+ if (state_ != State::Stopped) {<br>
+ FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(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 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>
- Request *request = requestQueue_.front();<br>
- request->metadata() = std::move(controls);<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) 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>
+ sensorInfo_.outputSize);<br>
+ scalerCrop_.translateBy(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) 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>
- 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().at(bufferId);<br>
<br>
- FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);<br>
+ LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId<br>
+ << ", timestamp: " << buffer->metadata().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().at(bufferId);<br>
+ handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);<br>
+ }<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></blockquote><div><br></div><div>Is this ctrls.set() call required? Does the Span s address the memory in the ControlList or is it a copy (in which case we do need the set)?</div><div><br></div><div>Thanks,</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>
<br>
isp_[Isp::Input].dev()->setControls(&ctrls);<br>
handleState();<br>
-- <br>
2.25.1<br>
<br>
</blockquote></div></div>