| Message ID | 20210217080241.1492143-1-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi all, This change addresses some tidying up after the IPA interface changes. However, I do have a question, see below. On Wed, 17 Feb 2021 at 08:02, Naushir Patuck <naush@raspberrypi.com> wrote: > This commit addresses a few fixes and tidy-ups after the IPAInterface > rework: > - Rename ConfigStaggeredWrite -> ConfigSensorParams > - Rename setIsp -> setIspControls > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() > should only run when state != Stopped. This matches the behavior of > the original code. Without this, start/stop cycles may cause errors. > - In setIspControls(), only update the LS config (with the fd) when a LS > control is present. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new > C++-only API") > --- > include/libcamera/ipa/raspberrypi.mojom | 4 +- > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- > 3 files changed, 50 insertions(+), 49 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > index bab19a946e18..8a256d022270 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -16,7 +16,7 @@ enum BufferMask { > const uint32 MaxLsGridSize = 0x8000; > > enum ConfigOutputParameters { > - ConfigStaggeredWrite = 0x01, > + ConfigSensorParams = 0x01, > }; > > struct SensorConfig { > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { > statsMetadataComplete(uint32 bufferId, ControlList controls); > runIsp(uint32 bufferId); > embeddedComplete(uint32 bufferId); > - setIsp(ControlList controls); > + setIspControls(ControlList controls); > setDelayedControls(ControlList controls); > }; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index 81a3195c82e9..0bf88bcc5f72 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo > &sensorInfo, > helper_->GetDelays(exposureDelay, gainDelay); > sensorMetadata = helper_->SensorEmbeddedDataPresent(); > > - result->params |= ipa::rpi::ConfigStaggeredWrite; > + result->params |= ipa::rpi::ConfigSensorParams; > result->sensorConfig.gainDelay = gainDelay; > result->sensorConfig.exposureDelay = exposureDelay; > result->sensorConfig.vblank = exposureDelay; > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) > applyDPC(dpcStatus, ctrls); > > if (!ctrls.empty()) > - setIsp.emit(ctrls); > + setIspControls.emit(ctrls); > } > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 15aa600ed581..d0ca015a01dc 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -152,7 +152,7 @@ public: > void statsMetadataComplete(uint32_t bufferId, const ControlList > &controls); > void runIsp(uint32_t bufferId); > void embeddedComplete(uint32_t bufferId); > - void setIsp(const ControlList &controls); > + void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > > /* bufferComplete signal handlers. */ > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() > ipa_->statsMetadataComplete.connect(this, > &RPiCameraData::statsMetadataComplete); > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); > ipa_->embeddedComplete.connect(this, > &RPiCameraData::embeddedComplete); > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); > + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > > IPASettings settings(ipa_->configurationFile(sensor_->model() + > ".json")); > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > return -EPIPE; > } > > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { > + if (result.params & ipa::rpi::ConfigSensorParams) { > /* > * Setup our delayed control writer with the sensor default > * gain and exposure delays. > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const > ControlList &controls) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = > isp_[Isp::Stats].getBuffers().at(bufferId); > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); > + /* Fill the Request metadata buffer with what the IPA has > provided */ > + Request *request = requestQueue_.front(); > + request->metadata() = std::move(controls); > > - /* Fill the Request metadata buffer with what the IPA has provided > */ > - Request *request = requestQueue_.front(); > - request->metadata() = std::move(controls); > + /* > + * Also update the ScalerCrop in the metadata with what we > actually > + * used. But we must first rescale that from ISP (camera > mode) pixels > + * back into sensor native pixels. > + * > + * Sending this information on every frame may be helpful. > + */ > + if (updateScalerCrop_) { > + updateScalerCrop_ = false; > + scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > + > sensorInfo_.outputSize); > + > scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + } > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > - /* > - * Also update the ScalerCrop in the metadata with what we actually > - * used. But we must first rescale that from ISP (camera mode) > pixels > - * back into sensor native pixels. > - * > - * Sending this information on every frame may be helpful. > - */ > - if (updateScalerCrop_) { > - updateScalerCrop_ = false; > - scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > - sensorInfo_.outputSize); > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + state_ = State::IpaComplete; > } > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > - > - state_ = State::IpaComplete; > > handleState(); > } > > void RPiCameraData::runIsp(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers().at(bufferId); > > - FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers().at(bufferId); > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << > bufferId > + << ", timestamp: " << > buffer->metadata().timestamp; > > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId > - << ", timestamp: " << buffer->metadata().timestamp; > + isp_[Isp::Input].queueBuffer(buffer); > + ispOutputCount_ = 0; > + } > > - isp_[Isp::Input].queueBuffer(buffer); > - ispOutputCount_ = 0; > handleState(); > } > > void RPiCameraData::embeddedComplete(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > + } > > - FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at(bufferId); > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > handleState(); > } > > -void RPiCameraData::setIsp(const ControlList &controls) > +void RPiCameraData::setIspControls(const ControlList &controls) > { > ControlList ctrls = controls; > > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > - ls.dmabuf = lsTable_.fd(); > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > + Span<const uint8_t> s = > + > ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > + bcm2835_isp_lens_shading ls = > + *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > + ls.dmabuf = lsTable_.fd(); > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t > *>(&ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlValue c(Span<const uint8_t>{ > reinterpret_cast<uint8_t *>(&ls), > + sizeof(ls) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > 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)? Thanks, Naush > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState(); > -- > 2.25.1 > >
Hi all, Sorry, I did have one more question on the new IPA interface topic. In raspberrypi.mojom, the module namespace is defined as module "ipa.rpi". Would it make sense to rename this to "ipa.RPi" so that it is consistent with the "RPi" namespace used elsewhere? Thanks, Naush On Wed, 17 Feb 2021 at 08:02, Naushir Patuck <naush@raspberrypi.com> wrote: > This commit addresses a few fixes and tidy-ups after the IPAInterface > rework: > - Rename ConfigStaggeredWrite -> ConfigSensorParams > - Rename setIsp -> setIspControls > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() > should only run when state != Stopped. This matches the behavior of > the original code. Without this, start/stop cycles may cause errors. > - In setIspControls(), only update the LS config (with the fd) when a LS > control is present. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new > C++-only API") > --- > include/libcamera/ipa/raspberrypi.mojom | 4 +- > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- > 3 files changed, 50 insertions(+), 49 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > index bab19a946e18..8a256d022270 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -16,7 +16,7 @@ enum BufferMask { > const uint32 MaxLsGridSize = 0x8000; > > enum ConfigOutputParameters { > - ConfigStaggeredWrite = 0x01, > + ConfigSensorParams = 0x01, > }; > > struct SensorConfig { > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { > statsMetadataComplete(uint32 bufferId, ControlList controls); > runIsp(uint32 bufferId); > embeddedComplete(uint32 bufferId); > - setIsp(ControlList controls); > + setIspControls(ControlList controls); > setDelayedControls(ControlList controls); > }; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index 81a3195c82e9..0bf88bcc5f72 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo > &sensorInfo, > helper_->GetDelays(exposureDelay, gainDelay); > sensorMetadata = helper_->SensorEmbeddedDataPresent(); > > - result->params |= ipa::rpi::ConfigStaggeredWrite; > + result->params |= ipa::rpi::ConfigSensorParams; > result->sensorConfig.gainDelay = gainDelay; > result->sensorConfig.exposureDelay = exposureDelay; > result->sensorConfig.vblank = exposureDelay; > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) > applyDPC(dpcStatus, ctrls); > > if (!ctrls.empty()) > - setIsp.emit(ctrls); > + setIspControls.emit(ctrls); > } > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 15aa600ed581..d0ca015a01dc 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -152,7 +152,7 @@ public: > void statsMetadataComplete(uint32_t bufferId, const ControlList > &controls); > void runIsp(uint32_t bufferId); > void embeddedComplete(uint32_t bufferId); > - void setIsp(const ControlList &controls); > + void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > > /* bufferComplete signal handlers. */ > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() > ipa_->statsMetadataComplete.connect(this, > &RPiCameraData::statsMetadataComplete); > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); > ipa_->embeddedComplete.connect(this, > &RPiCameraData::embeddedComplete); > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); > + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > > IPASettings settings(ipa_->configurationFile(sensor_->model() + > ".json")); > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > return -EPIPE; > } > > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { > + if (result.params & ipa::rpi::ConfigSensorParams) { > /* > * Setup our delayed control writer with the sensor default > * gain and exposure delays. > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const > ControlList &controls) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = > isp_[Isp::Stats].getBuffers().at(bufferId); > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); > + /* Fill the Request metadata buffer with what the IPA has > provided */ > + Request *request = requestQueue_.front(); > + request->metadata() = std::move(controls); > > - /* Fill the Request metadata buffer with what the IPA has provided > */ > - Request *request = requestQueue_.front(); > - request->metadata() = std::move(controls); > + /* > + * Also update the ScalerCrop in the metadata with what we > actually > + * used. But we must first rescale that from ISP (camera > mode) pixels > + * back into sensor native pixels. > + * > + * Sending this information on every frame may be helpful. > + */ > + if (updateScalerCrop_) { > + updateScalerCrop_ = false; > + scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > + > sensorInfo_.outputSize); > + > scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + } > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > - /* > - * Also update the ScalerCrop in the metadata with what we actually > - * used. But we must first rescale that from ISP (camera mode) > pixels > - * back into sensor native pixels. > - * > - * Sending this information on every frame may be helpful. > - */ > - if (updateScalerCrop_) { > - updateScalerCrop_ = false; > - scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > - sensorInfo_.outputSize); > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + state_ = State::IpaComplete; > } > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > - > - state_ = State::IpaComplete; > > handleState(); > } > > void RPiCameraData::runIsp(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers().at(bufferId); > > - FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers().at(bufferId); > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << > bufferId > + << ", timestamp: " << > buffer->metadata().timestamp; > > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId > - << ", timestamp: " << buffer->metadata().timestamp; > + isp_[Isp::Input].queueBuffer(buffer); > + ispOutputCount_ = 0; > + } > > - isp_[Isp::Input].queueBuffer(buffer); > - ispOutputCount_ = 0; > handleState(); > } > > void RPiCameraData::embeddedComplete(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > + } > > - FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at(bufferId); > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > handleState(); > } > > -void RPiCameraData::setIsp(const ControlList &controls) > +void RPiCameraData::setIspControls(const ControlList &controls) > { > ControlList ctrls = controls; > > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > - ls.dmabuf = lsTable_.fd(); > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > + Span<const uint8_t> s = > + > ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > + bcm2835_isp_lens_shading ls = > + *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > + ls.dmabuf = lsTable_.fd(); > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t > *>(&ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlValue c(Span<const uint8_t>{ > reinterpret_cast<uint8_t *>(&ls), > + sizeof(ls) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState(); > -- > 2.25.1 > >
Hi Naush, Thank you for the patch. On Wed, Feb 17, 2021 at 08:02:41AM +0000, Naushir Patuck wrote: > This commit addresses a few fixes and tidy-ups after the IPAInterface > rework: > - Rename ConfigStaggeredWrite -> ConfigSensorParams > - Rename setIsp -> setIspControls > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() > should only run when state != Stopped. This matches the behavior of > the original code. Without this, start/stop cycles may cause errors. > - In setIspControls(), only update the LS config (with the fd) when a LS > control is present. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new C++-only API") > --- > include/libcamera/ipa/raspberrypi.mojom | 4 +- > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- > 3 files changed, 50 insertions(+), 49 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index bab19a946e18..8a256d022270 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -16,7 +16,7 @@ enum BufferMask { > const uint32 MaxLsGridSize = 0x8000; > > enum ConfigOutputParameters { > - ConfigStaggeredWrite = 0x01, > + ConfigSensorParams = 0x01, > }; > > struct SensorConfig { > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { > statsMetadataComplete(uint32 bufferId, ControlList controls); > runIsp(uint32 bufferId); > embeddedComplete(uint32 bufferId); > - setIsp(ControlList controls); > + setIspControls(ControlList controls); > setDelayedControls(ControlList controls); > }; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 81a3195c82e9..0bf88bcc5f72 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, > helper_->GetDelays(exposureDelay, gainDelay); > sensorMetadata = helper_->SensorEmbeddedDataPresent(); > > - result->params |= ipa::rpi::ConfigStaggeredWrite; > + result->params |= ipa::rpi::ConfigSensorParams; > result->sensorConfig.gainDelay = gainDelay; > result->sensorConfig.exposureDelay = exposureDelay; > result->sensorConfig.vblank = exposureDelay; > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) > applyDPC(dpcStatus, ctrls); > > if (!ctrls.empty()) > - setIsp.emit(ctrls); > + setIspControls.emit(ctrls); > } > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 15aa600ed581..d0ca015a01dc 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -152,7 +152,7 @@ public: > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls); > void runIsp(uint32_t bufferId); > void embeddedComplete(uint32_t bufferId); > - void setIsp(const ControlList &controls); > + void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > > /* bufferComplete signal handlers. */ > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() > ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete); > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); > ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete); > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); > + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > > IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json")); > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > return -EPIPE; > } > > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { > + if (result.params & ipa::rpi::ConfigSensorParams) { > /* > * Setup our delayed control writer with the sensor default > * gain and exposure delays. > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls) > { > - if (state_ == State::Stopped) > - handleState(); For this (and the ones below) I wonder if it would be cleaner to return here instead. That was actually my intention, I guess it slipped through when I did the translation. > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); > + /* Fill the Request metadata buffer with what the IPA has provided */ > + Request *request = requestQueue_.front(); > + request->metadata() = std::move(controls); > > - /* Fill the Request metadata buffer with what the IPA has provided */ > - Request *request = requestQueue_.front(); > - request->metadata() = std::move(controls); > + /* > + * Also update the ScalerCrop in the metadata with what we actually > + * used. But we must first rescale that from ISP (camera mode) pixels > + * back into sensor native pixels. > + * > + * Sending this information on every frame may be helpful. > + */ > + if (updateScalerCrop_) { > + updateScalerCrop_ = false; > + scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > + sensorInfo_.outputSize); > + scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + } > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > - /* > - * Also update the ScalerCrop in the metadata with what we actually > - * used. But we must first rescale that from ISP (camera mode) pixels > - * back into sensor native pixels. > - * > - * Sending this information on every frame may be helpful. > - */ > - if (updateScalerCrop_) { > - updateScalerCrop_ = false; > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > - sensorInfo_.outputSize); > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + state_ = State::IpaComplete; > } > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > - > - state_ = State::IpaComplete; > > handleState(); > } > > void RPiCameraData::runIsp(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId); > > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId); > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId > + << ", timestamp: " << buffer->metadata().timestamp; > > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId > - << ", timestamp: " << buffer->metadata().timestamp; > + isp_[Isp::Input].queueBuffer(buffer); > + ispOutputCount_ = 0; > + } > > - isp_[Isp::Input].queueBuffer(buffer); > - ispOutputCount_ = 0; > handleState(); > } > > void RPiCameraData::embeddedComplete(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > + } > > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId); > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > handleState(); > } > > -void RPiCameraData::setIsp(const ControlList &controls) > +void RPiCameraData::setIspControls(const ControlList &controls) > { > ControlList ctrls = controls; > > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); > - ls.dmabuf = lsTable_.fd(); > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { Good catch :) > + Span<const uint8_t> s = > + ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > + bcm2835_isp_lens_shading ls = > + *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); I think you are right that we don't need to set() the control again below. In that case ls would have to be a pointer. With or without the suggested changes, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > + ls.dmabuf = lsTable_.fd(); > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), > + sizeof(ls) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState(); > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On Wed, Feb 17, 2021 at 08:28:37AM +0000, Naushir Patuck wrote: > Hi all, > > Sorry, I did have one more question on the new IPA interface topic. In > raspberrypi.mojom, the module namespace is defined as module "ipa.rpi". > > Would it make sense to rename this to "ipa.RPi" so that it is consistent with > the "RPi" namespace used elsewhere? There's nothing technical preventing it. I just chose that capitalization arbitrarily and nobody objected. Paul > > On Wed, 17 Feb 2021 at 08:02, Naushir Patuck <naush@raspberrypi.com> wrote: > > This commit addresses a few fixes and tidy-ups after the IPAInterface > rework: > - Rename ConfigStaggeredWrite -> ConfigSensorParams > - Rename setIsp -> setIspControls > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() > should only run when state != Stopped. This matches the behavior of > the original code. Without this, start/stop cycles may cause errors. > - In setIspControls(), only update the LS config (with the fd) when a LS > control is present. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new > C++-only API") > --- > include/libcamera/ipa/raspberrypi.mojom | 4 +- > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- > 3 files changed, 50 insertions(+), 49 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ > ipa/raspberrypi.mojom > index bab19a946e18..8a256d022270 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -16,7 +16,7 @@ enum BufferMask { > const uint32 MaxLsGridSize = 0x8000; > > enum ConfigOutputParameters { > - ConfigStaggeredWrite = 0x01, > + ConfigSensorParams = 0x01, > }; > > struct SensorConfig { > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { > statsMetadataComplete(uint32 bufferId, ControlList controls); > runIsp(uint32 bufferId); > embeddedComplete(uint32 bufferId); > - setIsp(ControlList controls); > + setIspControls(ControlList controls); > setDelayedControls(ControlList controls); > }; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/ > raspberrypi.cpp > index 81a3195c82e9..0bf88bcc5f72 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo & > sensorInfo, > helper_->GetDelays(exposureDelay, gainDelay); > sensorMetadata = helper_->SensorEmbeddedDataPresent(); > > - result->params |= ipa::rpi::ConfigStaggeredWrite; > + result->params |= ipa::rpi::ConfigSensorParams; > result->sensorConfig.gainDelay = gainDelay; > result->sensorConfig.exposureDelay = exposureDelay; > result->sensorConfig.vblank = exposureDelay; > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) > applyDPC(dpcStatus, ctrls); > > if (!ctrls.empty()) > - setIsp.emit(ctrls); > + setIspControls.emit(ctrls); > } > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/ > libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 15aa600ed581..d0ca015a01dc 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -152,7 +152,7 @@ public: > void statsMetadataComplete(uint32_t bufferId, const ControlList & > controls); > void runIsp(uint32_t bufferId); > void embeddedComplete(uint32_t bufferId); > - void setIsp(const ControlList &controls); > + void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > > /* bufferComplete signal handlers. */ > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() > ipa_->statsMetadataComplete.connect(this, & > RPiCameraData::statsMetadataComplete); > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); > ipa_->embeddedComplete.connect(this, & > RPiCameraData::embeddedComplete); > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); > + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, & > RPiCameraData::setDelayedControls); > > IPASettings settings(ipa_->configurationFile(sensor_->model() + > ".json")); > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > return -EPIPE; > } > > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { > + if (result.params & ipa::rpi::ConfigSensorParams) { > /* > * Setup our delayed control writer with the sensor default > * gain and exposure delays. > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const > ControlList &controls) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at > (bufferId); > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); > + /* Fill the Request metadata buffer with what the IPA has > provided */ > + Request *request = requestQueue_.front(); > + request->metadata() = std::move(controls); > > - /* Fill the Request metadata buffer with what the IPA has provided > */ > - Request *request = requestQueue_.front(); > - request->metadata() = std::move(controls); > + /* > + * Also update the ScalerCrop in the metadata with what we > actually > + * used. But we must first rescale that from ISP (camera > mode) pixels > + * back into sensor native pixels. > + * > + * Sending this information on every frame may be helpful. > + */ > + if (updateScalerCrop_) { > + updateScalerCrop_ = false; > + scalerCrop_ = ispCrop_.scaledBy > (sensorInfo_.analogCrop.size(), > + > sensorInfo_.outputSize); > + scalerCrop_.translateBy > (sensorInfo_.analogCrop.topLeft()); > + } > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > - /* > - * Also update the ScalerCrop in the metadata with what we actually > - * used. But we must first rescale that from ISP (camera mode) > pixels > - * back into sensor native pixels. > - * > - * Sending this information on every frame may be helpful. > - */ > - if (updateScalerCrop_) { > - updateScalerCrop_ = false; > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size > (), > - sensorInfo_.outputSize); > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > + state_ = State::IpaComplete; > } > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > - > - state_ = State::IpaComplete; > > handleState(); > } > > void RPiCameraData::runIsp(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers > ().at(bufferId); > > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at > (bufferId); > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << > bufferId > + << ", timestamp: " << buffer->metadata > ().timestamp; > > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId > - << ", timestamp: " << buffer->metadata().timestamp; > + isp_[Isp::Input].queueBuffer(buffer); > + ispOutputCount_ = 0; > + } > > - isp_[Isp::Input].queueBuffer(buffer); > - ispOutputCount_ = 0; > handleState(); > } > > void RPiCameraData::embeddedComplete(uint32_t bufferId) > { > - if (state_ == State::Stopped) > - handleState(); > + if (state_ != State::Stopped) { > + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers > ().at(bufferId); > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > + } > > - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at > (bufferId); > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > handleState(); > } > > -void RPiCameraData::setIsp(const ControlList &controls) > +void RPiCameraData::setIspControls(const ControlList &controls) > { > ControlList ctrls = controls; > > - Span<const uint8_t> s = > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > - bcm2835_isp_lens_shading ls = > - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data > ()); > - ls.dmabuf = lsTable_.fd(); > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > + Span<const uint8_t> s = > + ctrls.get > (V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > + bcm2835_isp_lens_shading ls = > + *reinterpret_cast<const bcm2835_isp_lens_shading *> > (s.data()); > + ls.dmabuf = lsTable_.fd(); > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(& > ls), > - sizeof(ls) }); > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + ControlValue c(Span<const uint8_t>{ reinterpret_cast > <uint8_t *>(&ls), > + sizeof(ls) }); > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > + } > > isp_[Isp::Input].dev()->setControls(&ctrls); > handleState(); > -- > 2.25.1 > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thank you for your review feedback. On Wed, 17 Feb 2021 at 08:38, <paul.elder@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Wed, Feb 17, 2021 at 08:02:41AM +0000, Naushir Patuck wrote: > > This commit addresses a few fixes and tidy-ups after the IPAInterface > > rework: > > - Rename ConfigStaggeredWrite -> ConfigSensorParams > > - Rename setIsp -> setIspControls > > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() > > should only run when state != Stopped. This matches the behavior of > > the original code. Without this, start/stop cycles may cause errors. > > - In setIspControls(), only update the LS config (with the fd) when a LS > > control is present. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the > new C++-only API") > > --- > > include/libcamera/ipa/raspberrypi.mojom | 4 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- > > 3 files changed, 50 insertions(+), 49 deletions(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > index bab19a946e18..8a256d022270 100644 > > --- a/include/libcamera/ipa/raspberrypi.mojom > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > @@ -16,7 +16,7 @@ enum BufferMask { > > const uint32 MaxLsGridSize = 0x8000; > > > > enum ConfigOutputParameters { > > - ConfigStaggeredWrite = 0x01, > > + ConfigSensorParams = 0x01, > > }; > > > > struct SensorConfig { > > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { > > statsMetadataComplete(uint32 bufferId, ControlList controls); > > runIsp(uint32 bufferId); > > embeddedComplete(uint32 bufferId); > > - setIsp(ControlList controls); > > + setIspControls(ControlList controls); > > setDelayedControls(ControlList controls); > > }; > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 81a3195c82e9..0bf88bcc5f72 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo > &sensorInfo, > > helper_->GetDelays(exposureDelay, gainDelay); > > sensorMetadata = helper_->SensorEmbeddedDataPresent(); > > > > - result->params |= ipa::rpi::ConfigStaggeredWrite; > > + result->params |= ipa::rpi::ConfigSensorParams; > > result->sensorConfig.gainDelay = gainDelay; > > result->sensorConfig.exposureDelay = exposureDelay; > > result->sensorConfig.vblank = exposureDelay; > > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) > > applyDPC(dpcStatus, ctrls); > > > > if (!ctrls.empty()) > > - setIsp.emit(ctrls); > > + setIspControls.emit(ctrls); > > } > > } > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 15aa600ed581..d0ca015a01dc 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -152,7 +152,7 @@ public: > > void statsMetadataComplete(uint32_t bufferId, const ControlList > &controls); > > void runIsp(uint32_t bufferId); > > void embeddedComplete(uint32_t bufferId); > > - void setIsp(const ControlList &controls); > > + void setIspControls(const ControlList &controls); > > void setDelayedControls(const ControlList &controls); > > > > /* bufferComplete signal handlers. */ > > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() > > ipa_->statsMetadataComplete.connect(this, > &RPiCameraData::statsMetadataComplete); > > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); > > ipa_->embeddedComplete.connect(this, > &RPiCameraData::embeddedComplete); > > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); > > + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > > ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > > > > IPASettings settings(ipa_->configurationFile(sensor_->model() + > ".json")); > > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > return -EPIPE; > > } > > > > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { > > + if (result.params & ipa::rpi::ConfigSensorParams) { > > /* > > * Setup our delayed control writer with the sensor default > > * gain and exposure delays. > > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const > ControlList &controls) > > { > > - if (state_ == State::Stopped) > > - handleState(); > > For this (and the ones below) I wonder if it would be cleaner to return > here instead. That was actually my intention, I guess it slipped through > when I did the translation. > I'm happy to do either. What do others feel is cleaner? > > > + if (state_ != State::Stopped) { > > + FrameBuffer *buffer = > isp_[Isp::Stats].getBuffers().at(bufferId); > > > > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); > > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > > > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > + /* Fill the Request metadata buffer with what the IPA has > provided */ > > + Request *request = requestQueue_.front(); > > + request->metadata() = std::move(controls); > > > > - /* Fill the Request metadata buffer with what the IPA has provided > */ > > - Request *request = requestQueue_.front(); > > - request->metadata() = std::move(controls); > > + /* > > + * Also update the ScalerCrop in the metadata with what we > actually > > + * used. But we must first rescale that from ISP (camera > mode) pixels > > + * back into sensor native pixels. > > + * > > + * Sending this information on every frame may be helpful. > > + */ > > + if (updateScalerCrop_) { > > + updateScalerCrop_ = false; > > + scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > > + > sensorInfo_.outputSize); > > + > scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > > + } > > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > > > - /* > > - * Also update the ScalerCrop in the metadata with what we actually > > - * used. But we must first rescale that from ISP (camera mode) > pixels > > - * back into sensor native pixels. > > - * > > - * Sending this information on every frame may be helpful. > > - */ > > - if (updateScalerCrop_) { > > - updateScalerCrop_ = false; > > - scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), > > - sensorInfo_.outputSize); > > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > > + state_ = State::IpaComplete; > > } > > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > > - > > - state_ = State::IpaComplete; > > > > handleState(); > > } > > > > void RPiCameraData::runIsp(uint32_t bufferId) > > { > > - if (state_ == State::Stopped) > > - handleState(); > > + if (state_ != State::Stopped) { > > + FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers().at(bufferId); > > > > - FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers().at(bufferId); > > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << > bufferId > > + << ", timestamp: " << > buffer->metadata().timestamp; > > > > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId > > - << ", timestamp: " << buffer->metadata().timestamp; > > + isp_[Isp::Input].queueBuffer(buffer); > > + ispOutputCount_ = 0; > > + } > > > > - isp_[Isp::Input].queueBuffer(buffer); > > - ispOutputCount_ = 0; > > handleState(); > > } > > > > void RPiCameraData::embeddedComplete(uint32_t bufferId) > > { > > - if (state_ == State::Stopped) > > - handleState(); > > + if (state_ != State::Stopped) { > > + FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at(bufferId); > > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > > + } > > > > - FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at(bufferId); > > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > > handleState(); > > } > > > > -void RPiCameraData::setIsp(const ControlList &controls) > > +void RPiCameraData::setIspControls(const ControlList &controls) > > { > > ControlList ctrls = controls; > > > > - Span<const uint8_t> s = > > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > - bcm2835_isp_lens_shading ls = > > - *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > > - ls.dmabuf = lsTable_.fd(); > > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > > Good catch :) > > > + Span<const uint8_t> s = > > + > ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > + bcm2835_isp_lens_shading ls = > > + *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data()); > > I think you are right that we don't need to set() the control again > below. In that case ls would have to be a pointer. > Great, I will make that change so we can remove the ctrls.set() call. Regards, Naush > > With or without the suggested changes, > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > + ls.dmabuf = lsTable_.fd(); > > > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t > *>(&ls), > > - sizeof(ls) }); > > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > + ControlValue c(Span<const uint8_t>{ > reinterpret_cast<uint8_t *>(&ls), > > + sizeof(ls) }); > > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > + } > > > > isp_[Isp::Input].dev()->setControls(&ctrls); > > handleState(); > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Paul, On Wed, 17 Feb 2021 at 08:56, <paul.elder@ideasonboard.com> wrote: > Hi Naush, > > On Wed, Feb 17, 2021 at 08:28:37AM +0000, Naushir Patuck wrote: > > Hi all, > > > > Sorry, I did have one more question on the new IPA interface topic. In > > raspberrypi.mojom, the module namespace is defined as module "ipa.rpi". > > > > Would it make sense to rename this to "ipa.RPi" so that it is consistent > with > > the "RPi" namespace used elsewhere? > > There's nothing technical preventing it. I just chose that > capitalization arbitrarily and nobody objected. > I'll change it to RPi then, just to keep the consistency. Sorry I did not flag this in your review rounds. Regards, Naush > > > Paul > > > > > On Wed, 17 Feb 2021 at 08:02, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > This commit addresses a few fixes and tidy-ups after the IPAInterface > > rework: > > - Rename ConfigStaggeredWrite -> ConfigSensorParams > > - Rename setIsp -> setIspControls > > - Signal handlers statsMetadataComplete(), runISP(), > embeddedComplete() > > should only run when state != Stopped. This matches the behavior of > > the original code. Without this, start/stop cycles may cause errors. > > - In setIspControls(), only update the LS config (with the fd) when > a LS > > control is present. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with > the new > > C++-only API") > > --- > > include/libcamera/ipa/raspberrypi.mojom | 4 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 91 > ++++++++++--------- > > 3 files changed, 50 insertions(+), 49 deletions(-) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ > > ipa/raspberrypi.mojom > > index bab19a946e18..8a256d022270 100644 > > --- a/include/libcamera/ipa/raspberrypi.mojom > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > @@ -16,7 +16,7 @@ enum BufferMask { > > const uint32 MaxLsGridSize = 0x8000; > > > > enum ConfigOutputParameters { > > - ConfigStaggeredWrite = 0x01, > > + ConfigSensorParams = 0x01, > > }; > > > > struct SensorConfig { > > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { > > statsMetadataComplete(uint32 bufferId, ControlList controls); > > runIsp(uint32 bufferId); > > embeddedComplete(uint32 bufferId); > > - setIsp(ControlList controls); > > + setIspControls(ControlList controls); > > setDelayedControls(ControlList controls); > > }; > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/ > > raspberrypi.cpp > > index 81a3195c82e9..0bf88bcc5f72 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo & > > sensorInfo, > > helper_->GetDelays(exposureDelay, gainDelay); > > sensorMetadata = > helper_->SensorEmbeddedDataPresent(); > > > > - result->params |= ipa::rpi::ConfigStaggeredWrite; > > + result->params |= ipa::rpi::ConfigSensorParams; > > result->sensorConfig.gainDelay = gainDelay; > > result->sensorConfig.exposureDelay = exposureDelay; > > result->sensorConfig.vblank = exposureDelay; > > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) > > applyDPC(dpcStatus, ctrls); > > > > if (!ctrls.empty()) > > - setIsp.emit(ctrls); > > + setIspControls.emit(ctrls); > > } > > } > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/ > > libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 15aa600ed581..d0ca015a01dc 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -152,7 +152,7 @@ public: > > void statsMetadataComplete(uint32_t bufferId, const > ControlList & > > controls); > > void runIsp(uint32_t bufferId); > > void embeddedComplete(uint32_t bufferId); > > - void setIsp(const ControlList &controls); > > + void setIspControls(const ControlList &controls); > > void setDelayedControls(const ControlList &controls); > > > > /* bufferComplete signal handlers. */ > > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() > > ipa_->statsMetadataComplete.connect(this, & > > RPiCameraData::statsMetadataComplete); > > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); > > ipa_->embeddedComplete.connect(this, & > > RPiCameraData::embeddedComplete); > > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); > > + ipa_->setIspControls.connect(this, > &RPiCameraData::setIspControls); > > ipa_->setDelayedControls.connect(this, & > > RPiCameraData::setDelayedControls); > > > > IPASettings > settings(ipa_->configurationFile(sensor_->model() + > > ".json")); > > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const > > CameraConfiguration *config) > > return -EPIPE; > > } > > > > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { > > + if (result.params & ipa::rpi::ConfigSensorParams) { > > /* > > * Setup our delayed control writer with the sensor > default > > * gain and exposure delays. > > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const > > CameraConfiguration *config) > > > > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const > > ControlList &controls) > > { > > - if (state_ == State::Stopped) > > - handleState(); > > + if (state_ != State::Stopped) { > > + FrameBuffer *buffer = > isp_[Isp::Stats].getBuffers().at > > (bufferId); > > > > - FrameBuffer *buffer = > isp_[Isp::Stats].getBuffers().at(bufferId); > > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > > > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); > > + /* Fill the Request metadata buffer with what the > IPA has > > provided */ > > + Request *request = requestQueue_.front(); > > + request->metadata() = std::move(controls); > > > > - /* Fill the Request metadata buffer with what the IPA has > provided > > */ > > - Request *request = requestQueue_.front(); > > - request->metadata() = std::move(controls); > > + /* > > + * Also update the ScalerCrop in the metadata with > what we > > actually > > + * used. But we must first rescale that from ISP > (camera > > mode) pixels > > + * back into sensor native pixels. > > + * > > + * Sending this information on every frame may be > helpful. > > + */ > > + if (updateScalerCrop_) { > > + updateScalerCrop_ = false; > > + scalerCrop_ = ispCrop_.scaledBy > > (sensorInfo_.analogCrop.size(), > > + > > sensorInfo_.outputSize); > > + scalerCrop_.translateBy > > (sensorInfo_.analogCrop.topLeft()); > > + } > > + request->metadata().set(controls::ScalerCrop, > scalerCrop_); > > > > - /* > > - * Also update the ScalerCrop in the metadata with what we > actually > > - * used. But we must first rescale that from ISP (camera > mode) > > pixels > > - * back into sensor native pixels. > > - * > > - * Sending this information on every frame may be helpful. > > - */ > > - if (updateScalerCrop_) { > > - updateScalerCrop_ = false; > > - scalerCrop_ = > ispCrop_.scaledBy(sensorInfo_.analogCrop.size > > (), > > - > sensorInfo_.outputSize); > > - > scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); > > + state_ = State::IpaComplete; > > } > > - request->metadata().set(controls::ScalerCrop, scalerCrop_); > > - > > - state_ = State::IpaComplete; > > > > handleState(); > > } > > > > void RPiCameraData::runIsp(uint32_t bufferId) > > { > > - if (state_ == State::Stopped) > > - handleState(); > > + if (state_ != State::Stopped) { > > + FrameBuffer *buffer = > unicam_[Unicam::Image].getBuffers > > ().at(bufferId); > > > > - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at > > (bufferId); > > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id > " << > > bufferId > > + << ", timestamp: " << > buffer->metadata > > ().timestamp; > > > > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << > bufferId > > - << ", timestamp: " << > buffer->metadata().timestamp; > > + isp_[Isp::Input].queueBuffer(buffer); > > + ispOutputCount_ = 0; > > + } > > > > - isp_[Isp::Input].queueBuffer(buffer); > > - ispOutputCount_ = 0; > > handleState(); > > } > > > > void RPiCameraData::embeddedComplete(uint32_t bufferId) > > { > > - if (state_ == State::Stopped) > > - handleState(); > > + if (state_ != State::Stopped) { > > + FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers > > ().at(bufferId); > > + handleStreamBuffer(buffer, > &unicam_[Unicam::Embedded]); > > + } > > > > - FrameBuffer *buffer = > unicam_[Unicam::Embedded].getBuffers().at > > (bufferId); > > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); > > handleState(); > > } > > > > -void RPiCameraData::setIsp(const ControlList &controls) > > +void RPiCameraData::setIspControls(const ControlList &controls) > > { > > ControlList ctrls = controls; > > > > - Span<const uint8_t> s = > > - > ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > - bcm2835_isp_lens_shading ls = > > - *reinterpret_cast<const bcm2835_isp_lens_shading > *>(s.data > > ()); > > - ls.dmabuf = lsTable_.fd(); > > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { > > + Span<const uint8_t> s = > > + ctrls.get > > (V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); > > + bcm2835_isp_lens_shading ls = > > + *reinterpret_cast<const > bcm2835_isp_lens_shading *> > > (s.data()); > > + ls.dmabuf = lsTable_.fd(); > > > > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t > *>(& > > ls), > > - sizeof(ls) }); > > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > + ControlValue c(Span<const uint8_t>{ reinterpret_cast > > <uint8_t *>(&ls), > > + sizeof(ls) }); > > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); > > + } > > > > isp_[Isp::Input].dev()->setControls(&ctrls); > > handleState(); > > -- > > 2.25.1 > > > > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Paul, On Wed, 17 Feb 2021 at 09:03, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi Paul, > > Thank you for your review feedback. > > On Wed, 17 Feb 2021 at 08:38, <paul.elder@ideasonboard.com> wrote: > >> Hi Naush, >> >> Thank you for the patch. >> >> On Wed, Feb 17, 2021 at 08:02:41AM +0000, Naushir Patuck wrote: >> > This commit addresses a few fixes and tidy-ups after the IPAInterface >> > rework: >> > - Rename ConfigStaggeredWrite -> ConfigSensorParams >> > - Rename setIsp -> setIspControls >> > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() >> > should only run when state != Stopped. This matches the behavior of >> > the original code. Without this, start/stop cycles may cause errors. >> > - In setIspControls(), only update the LS config (with the fd) when a LS >> > control is present. >> > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the >> new C++-only API") >> > --- >> > include/libcamera/ipa/raspberrypi.mojom | 4 +- >> > src/ipa/raspberrypi/raspberrypi.cpp | 4 +- >> > .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- >> > 3 files changed, 50 insertions(+), 49 deletions(-) >> > >> > diff --git a/include/libcamera/ipa/raspberrypi.mojom >> b/include/libcamera/ipa/raspberrypi.mojom >> > index bab19a946e18..8a256d022270 100644 >> > --- a/include/libcamera/ipa/raspberrypi.mojom >> > +++ b/include/libcamera/ipa/raspberrypi.mojom >> > @@ -16,7 +16,7 @@ enum BufferMask { >> > const uint32 MaxLsGridSize = 0x8000; >> > >> > enum ConfigOutputParameters { >> > - ConfigStaggeredWrite = 0x01, >> > + ConfigSensorParams = 0x01, >> > }; >> > >> > struct SensorConfig { >> > @@ -126,6 +126,6 @@ interface IPARPiEventInterface { >> > statsMetadataComplete(uint32 bufferId, ControlList controls); >> > runIsp(uint32 bufferId); >> > embeddedComplete(uint32 bufferId); >> > - setIsp(ControlList controls); >> > + setIspControls(ControlList controls); >> > setDelayedControls(ControlList controls); >> > }; >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp >> b/src/ipa/raspberrypi/raspberrypi.cpp >> > index 81a3195c82e9..0bf88bcc5f72 100644 >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp >> > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo >> &sensorInfo, >> > helper_->GetDelays(exposureDelay, gainDelay); >> > sensorMetadata = helper_->SensorEmbeddedDataPresent(); >> > >> > - result->params |= ipa::rpi::ConfigStaggeredWrite; >> > + result->params |= ipa::rpi::ConfigSensorParams; >> > result->sensorConfig.gainDelay = gainDelay; >> > result->sensorConfig.exposureDelay = exposureDelay; >> > result->sensorConfig.vblank = exposureDelay; >> > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) >> > applyDPC(dpcStatus, ctrls); >> > >> > if (!ctrls.empty()) >> > - setIsp.emit(ctrls); >> > + setIspControls.emit(ctrls); >> > } >> > } >> > >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > index 15aa600ed581..d0ca015a01dc 100644 >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > @@ -152,7 +152,7 @@ public: >> > void statsMetadataComplete(uint32_t bufferId, const ControlList >> &controls); >> > void runIsp(uint32_t bufferId); >> > void embeddedComplete(uint32_t bufferId); >> > - void setIsp(const ControlList &controls); >> > + void setIspControls(const ControlList &controls); >> > void setDelayedControls(const ControlList &controls); >> > >> > /* bufferComplete signal handlers. */ >> > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() >> > ipa_->statsMetadataComplete.connect(this, >> &RPiCameraData::statsMetadataComplete); >> > ipa_->runIsp.connect(this, &RPiCameraData::runIsp); >> > ipa_->embeddedComplete.connect(this, >> &RPiCameraData::embeddedComplete); >> > - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); >> > + ipa_->setIspControls.connect(this, >> &RPiCameraData::setIspControls); >> > ipa_->setDelayedControls.connect(this, >> &RPiCameraData::setDelayedControls); >> > >> > IPASettings settings(ipa_->configurationFile(sensor_->model() + >> ".json")); >> > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const >> CameraConfiguration *config) >> > return -EPIPE; >> > } >> > >> > - if (result.params & ipa::rpi::ConfigStaggeredWrite) { >> > + if (result.params & ipa::rpi::ConfigSensorParams) { >> > /* >> > * Setup our delayed control writer with the sensor >> default >> > * gain and exposure delays. >> > @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const >> CameraConfiguration *config) >> > >> > void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const >> ControlList &controls) >> > { >> > - if (state_ == State::Stopped) >> > - handleState(); >> >> For this (and the ones below) I wonder if it would be cleaner to return >> here instead. That was actually my intention, I guess it slipped through >> when I did the translation. >> > > I'm happy to do either. What do others feel is cleaner? > Actually having read the code in handleState(), it does not do anything when state_ == State::Stopped anymore. So, these can simply be replaced with a single return statement. I'll make the change and submit a revision including the changes for the ipa::rpi namespace change and the LS fixes. I will not add your R-B tag so you have a chance to re-review if that's ok. Regards, Naush > > >> >> > + if (state_ != State::Stopped) { >> > + FrameBuffer *buffer = >> isp_[Isp::Stats].getBuffers().at(bufferId); >> > >> > - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); >> > + handleStreamBuffer(buffer, &isp_[Isp::Stats]); >> > >> > - handleStreamBuffer(buffer, &isp_[Isp::Stats]); >> > + /* Fill the Request metadata buffer with what the IPA has >> provided */ >> > + Request *request = requestQueue_.front(); >> > + request->metadata() = std::move(controls); >> > >> > - /* Fill the Request metadata buffer with what the IPA has >> provided */ >> > - Request *request = requestQueue_.front(); >> > - request->metadata() = std::move(controls); >> > + /* >> > + * Also update the ScalerCrop in the metadata with what we >> actually >> > + * used. But we must first rescale that from ISP (camera >> mode) pixels >> > + * back into sensor native pixels. >> > + * >> > + * Sending this information on every frame may be helpful. >> > + */ >> > + if (updateScalerCrop_) { >> > + updateScalerCrop_ = false; >> > + scalerCrop_ = >> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), >> > + >> sensorInfo_.outputSize); >> > + >> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); >> > + } >> > + request->metadata().set(controls::ScalerCrop, >> scalerCrop_); >> > >> > - /* >> > - * Also update the ScalerCrop in the metadata with what we >> actually >> > - * used. But we must first rescale that from ISP (camera mode) >> pixels >> > - * back into sensor native pixels. >> > - * >> > - * Sending this information on every frame may be helpful. >> > - */ >> > - if (updateScalerCrop_) { >> > - updateScalerCrop_ = false; >> > - scalerCrop_ = >> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), >> > - sensorInfo_.outputSize); >> > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); >> > + state_ = State::IpaComplete; >> > } >> > - request->metadata().set(controls::ScalerCrop, scalerCrop_); >> > - >> > - state_ = State::IpaComplete; >> > >> > handleState(); >> > } >> > >> > void RPiCameraData::runIsp(uint32_t bufferId) >> > { >> > - if (state_ == State::Stopped) >> > - handleState(); >> > + if (state_ != State::Stopped) { >> > + FrameBuffer *buffer = >> unicam_[Unicam::Image].getBuffers().at(bufferId); >> > >> > - FrameBuffer *buffer = >> unicam_[Unicam::Image].getBuffers().at(bufferId); >> > + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << >> bufferId >> > + << ", timestamp: " << >> buffer->metadata().timestamp; >> > >> > - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId >> > - << ", timestamp: " << >> buffer->metadata().timestamp; >> > + isp_[Isp::Input].queueBuffer(buffer); >> > + ispOutputCount_ = 0; >> > + } >> > >> > - isp_[Isp::Input].queueBuffer(buffer); >> > - ispOutputCount_ = 0; >> > handleState(); >> > } >> > >> > void RPiCameraData::embeddedComplete(uint32_t bufferId) >> > { >> > - if (state_ == State::Stopped) >> > - handleState(); >> > + if (state_ != State::Stopped) { >> > + FrameBuffer *buffer = >> unicam_[Unicam::Embedded].getBuffers().at(bufferId); >> > + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); >> > + } >> > >> > - FrameBuffer *buffer = >> unicam_[Unicam::Embedded].getBuffers().at(bufferId); >> > - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); >> > handleState(); >> > } >> > >> > -void RPiCameraData::setIsp(const ControlList &controls) >> > +void RPiCameraData::setIspControls(const ControlList &controls) >> > { >> > ControlList ctrls = controls; >> > >> > - Span<const uint8_t> s = >> > - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); >> > - bcm2835_isp_lens_shading ls = >> > - *reinterpret_cast<const bcm2835_isp_lens_shading >> *>(s.data()); >> > - ls.dmabuf = lsTable_.fd(); >> > + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { >> >> Good catch :) >> >> > + Span<const uint8_t> s = >> > + >> ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); >> > + bcm2835_isp_lens_shading ls = >> > + *reinterpret_cast<const bcm2835_isp_lens_shading >> *>(s.data()); >> >> I think you are right that we don't need to set() the control again >> below. In that case ls would have to be a pointer. >> > > Great, I will make that change so we can remove the ctrls.set() call. > > Regards, > Naush > > >> >> With or without the suggested changes, >> >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> >> >> > + ls.dmabuf = lsTable_.fd(); >> > >> > - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t >> *>(&ls), >> > - sizeof(ls) }); >> > - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); >> > + ControlValue c(Span<const uint8_t>{ >> reinterpret_cast<uint8_t *>(&ls), >> > + sizeof(ls) }); >> > + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); >> > + } >> > >> > isp_[Isp::Input].dev()->setControls(&ctrls); >> > handleState(); >> > -- >> > 2.25.1 >> > >> > _______________________________________________ >> > libcamera-devel mailing list >> > libcamera-devel@lists.libcamera.org >> > https://lists.libcamera.org/listinfo/libcamera-devel >> >
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index bab19a946e18..8a256d022270 100644 --- a/include/libcamera/ipa/raspberrypi.mojom +++ b/include/libcamera/ipa/raspberrypi.mojom @@ -16,7 +16,7 @@ enum BufferMask { const uint32 MaxLsGridSize = 0x8000; enum ConfigOutputParameters { - ConfigStaggeredWrite = 0x01, + ConfigSensorParams = 0x01, }; struct SensorConfig { @@ -126,6 +126,6 @@ interface IPARPiEventInterface { statsMetadataComplete(uint32 bufferId, ControlList controls); runIsp(uint32 bufferId); embeddedComplete(uint32 bufferId); - setIsp(ControlList controls); + setIspControls(ControlList controls); setDelayedControls(ControlList controls); }; diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 81a3195c82e9..0bf88bcc5f72 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo, helper_->GetDelays(exposureDelay, gainDelay); sensorMetadata = helper_->SensorEmbeddedDataPresent(); - result->params |= ipa::rpi::ConfigStaggeredWrite; + result->params |= ipa::rpi::ConfigSensorParams; result->sensorConfig.gainDelay = gainDelay; result->sensorConfig.exposureDelay = exposureDelay; result->sensorConfig.vblank = exposureDelay; @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId) applyDPC(dpcStatus, ctrls); if (!ctrls.empty()) - setIsp.emit(ctrls); + setIspControls.emit(ctrls); } } diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 15aa600ed581..d0ca015a01dc 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -152,7 +152,7 @@ public: void statsMetadataComplete(uint32_t bufferId, const ControlList &controls); void runIsp(uint32_t bufferId); void embeddedComplete(uint32_t bufferId); - void setIsp(const ControlList &controls); + void setIspControls(const ControlList &controls); void setDelayedControls(const ControlList &controls); /* bufferComplete signal handlers. */ @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA() ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete); ipa_->runIsp.connect(this, &RPiCameraData::runIsp); ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete); - ipa_->setIsp.connect(this, &RPiCameraData::setIsp); + ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json")); @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) return -EPIPE; } - if (result.params & ipa::rpi::ConfigStaggeredWrite) { + if (result.params & ipa::rpi::ConfigSensorParams) { /* * Setup our delayed control writer with the sensor default * gain and exposure delays. @@ -1280,75 +1280,76 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls) { - if (state_ == State::Stopped) - handleState(); + if (state_ != State::Stopped) { + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId); + handleStreamBuffer(buffer, &isp_[Isp::Stats]); - handleStreamBuffer(buffer, &isp_[Isp::Stats]); + /* Fill the Request metadata buffer with what the IPA has provided */ + Request *request = requestQueue_.front(); + request->metadata() = std::move(controls); - /* Fill the Request metadata buffer with what the IPA has provided */ - Request *request = requestQueue_.front(); - request->metadata() = std::move(controls); + /* + * Also update the ScalerCrop in the metadata with what we actually + * used. But we must first rescale that from ISP (camera mode) pixels + * back into sensor native pixels. + * + * Sending this information on every frame may be helpful. + */ + if (updateScalerCrop_) { + updateScalerCrop_ = false; + scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), + sensorInfo_.outputSize); + scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); + } + request->metadata().set(controls::ScalerCrop, scalerCrop_); - /* - * Also update the ScalerCrop in the metadata with what we actually - * used. But we must first rescale that from ISP (camera mode) pixels - * back into sensor native pixels. - * - * Sending this information on every frame may be helpful. - */ - if (updateScalerCrop_) { - updateScalerCrop_ = false; - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(), - sensorInfo_.outputSize); - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft()); + state_ = State::IpaComplete; } - request->metadata().set(controls::ScalerCrop, scalerCrop_); - - state_ = State::IpaComplete; handleState(); } void RPiCameraData::runIsp(uint32_t bufferId) { - if (state_ == State::Stopped) - handleState(); + if (state_ != State::Stopped) { + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId); - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId); + LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId + << ", timestamp: " << buffer->metadata().timestamp; - LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId - << ", timestamp: " << buffer->metadata().timestamp; + isp_[Isp::Input].queueBuffer(buffer); + ispOutputCount_ = 0; + } - isp_[Isp::Input].queueBuffer(buffer); - ispOutputCount_ = 0; handleState(); } void RPiCameraData::embeddedComplete(uint32_t bufferId) { - if (state_ == State::Stopped) - handleState(); + if (state_ != State::Stopped) { + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId); + handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); + } - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId); - handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]); handleState(); } -void RPiCameraData::setIsp(const ControlList &controls) +void RPiCameraData::setIspControls(const ControlList &controls) { ControlList ctrls = controls; - Span<const uint8_t> s = - ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); - bcm2835_isp_lens_shading ls = - *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); - ls.dmabuf = lsTable_.fd(); + if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) { + Span<const uint8_t> s = + ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data(); + bcm2835_isp_lens_shading ls = + *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data()); + ls.dmabuf = lsTable_.fd(); - ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), - sizeof(ls) }); - ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); + ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls), + sizeof(ls) }); + ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c); + } isp_[Isp::Input].dev()->setControls(&ctrls); handleState();
This commit addresses a few fixes and tidy-ups after the IPAInterface rework: - Rename ConfigStaggeredWrite -> ConfigSensorParams - Rename setIsp -> setIspControls - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete() should only run when state != Stopped. This matches the behavior of the original code. Without this, start/stop cycles may cause errors. - In setIspControls(), only update the LS config (with the fd) when a LS control is present. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new C++-only API") --- include/libcamera/ipa/raspberrypi.mojom | 4 +- src/ipa/raspberrypi/raspberrypi.cpp | 4 +- .../pipeline/raspberrypi/raspberrypi.cpp | 91 ++++++++++--------- 3 files changed, 50 insertions(+), 49 deletions(-)