[libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
David Plowman
david.plowman at raspberrypi.com
Wed Feb 17 12:18:55 CET 2021
Hi Naush
Thanks for these patches. They fix an intermittent crash I was
experiencing when trying to stop libcamera (in fact exactly as
predicted in the commit message in relation to the signal handlers),
thus:
Tested-by: David Plowman <david.plowman at raspberrypi.com>
Best regards
David
On Wed, 17 Feb 2021 at 10:29, <paul.elder at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Wed, Feb 17, 2021 at 10:25:29AM +0000, Naushir Patuck wrote:
> > Hi Paul,
> >
> >
> > On Wed, 17 Feb 2021 at 10:18, <paul.elder at ideasonboard.com> wrote:
> >
> > Hi Naush,
> >
> > On Wed, Feb 17, 2021 at 10:08:50AM +0000, 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.
> > >
> > > 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;
> >
> > I thought handleState() still needs to be called in this (and below) case?
> >
> >
> > I thought so too :)
> >
> > But if you have a look at handleState(), in the case State::Stopped, it simply
> > breaks and returns doing nothing.
> > So, replacing by a single return is sufficient. Clearly it did do something in
> > the past, but we should be ok without
> > calling handleState now.
>
> Ah, I see. I just saw your reply to v1 too :)
>
> Well that's nice, it makes everything cleaner.
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
>
> >
> > >
> > > 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);
> > > + }
> > >
> > > 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);
> > > }
> > >
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> _______________________________________________
> 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