[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
Naushir Patuck
naush at raspberrypi.com
Wed Feb 17 09:28:37 CET 2021
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 at 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 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210217/a5dc2d6e/attachment.htm>
More information about the libcamera-devel
mailing list