[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Feb 17 09:38:51 CET 2021
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 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();
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list