[libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Feb 18 12:38:32 CET 2021
On 18/02/2021 11:35, Naushir Patuck wrote:
> Hi Kieran,
>
> On Thu, 18 Feb 2021 at 11:25, Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto:kieran.bingham at ideasonboard.com>> wrote:
>
> 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.
>
>
> I do agree. If you prefer, I could split this into code changes (for
> fixing the crash) and the other cosmetic changes.
>
>
>
>
>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com
> <mailto: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?
>
>
> Actually not this one, but the hunks just above that fix the bug. Let
> me split this change up and I will resubmit a v3. If it's ok, I will
> add your R-b tags to them.
>
Ahaha, great, I messed up, so yes - splitting at least functional
changes from cosmetic would be helpful.
Yes, please keep the tag though.
--
Kieran
> Regards,
> Naush
>
>
>
>
> 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
> <mailto: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
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list