[libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Feb 18 12:25:48 CET 2021
Hi Naush,
On 17/02/2021 10:08, Naushir Patuck wrote:
> This commit addresses a few fixes and tidy-ups after the IPAInterface
> rework:
> - Rename ConfigStaggeredWrite -> ConfigSensorParams
> - Rename setIsp -> setIspControls
> - Switch to camel case for ISPConfig::embeddedbufferId and
> ISPConfig::bayerbufferId.
> - 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.
That's quite a lot of change for a single patch, but I don't think
there's a point for splitting this out now, especially as it's required
to fix a crash.
> 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 | 8 ++---
> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++---
> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++++++---------
> 3 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index bab19a946e18..9c05cc68cceb 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 {
> @@ -27,8 +27,8 @@ struct SensorConfig {
> };
>
> struct ISPConfig {
> - uint32 embeddedbufferId;
> - uint32 bayerbufferId;
> + uint32 embeddedBufferId;
> + uint32 bayerBufferId;
> };
>
> struct ConfigInput {
> @@ -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..974f4ec63058 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;
> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)
> * avoid running the control algos for a few frames in case
> * they are "unreliable".
> */
> - prepareISP(data.embeddedbufferId);
> + prepareISP(data.embeddedBufferId);
> frameCount_++;
>
> /* Ready to push the input buffer into the ISP. */
> - runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> + runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
> }
>
> void IPARPi::reportMetadata()
> @@ -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..8770ae66a21a 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.
> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> {
> if (state_ == State::Stopped)
> - handleState();
> + return;
>
> FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>
> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> void RPiCameraData::runIsp(uint32_t bufferId)
> {
> if (state_ == State::Stopped)
> - handleState();
> + return;
>
> FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>
> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
> void RPiCameraData::embeddedComplete(uint32_t bufferId)
> {
> if (state_ == State::Stopped)
> - handleState();
> + return;
>
> 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);
> + }
I assume it's this hunk which fixes the bug?
Everything else seems to be a rename or the handleState changes perhaps
are a no-effect change?
As this is a bug fix, it would have been nice to see that split so the
reader can understand what code change actually causes the fix.
The renames are trivial, so they're fine, and this seesm to make sense
as it protects against accessing a control which isn't set.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> isp_[Isp::Input].dev()->setControls(&ctrls);
> handleState();
> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
> << " Embedded buffer id: " << embeddedId;
>
> ipa::rpi::ISPConfig ispPrepare;
> - ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> - ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> + ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> + ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
> ipa_->signalIspPrepare(ispPrepare);
> }
>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list