[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
Naushir Patuck
naush at raspberrypi.com
Wed Feb 17 10:04:25 CET 2021
Hi Paul,
On Wed, 17 Feb 2021 at 08:56, <paul.elder at ideasonboard.com> wrote:
> Hi Naush,
>
> On Wed, Feb 17, 2021 at 08:28:37AM +0000, Naushir Patuck wrote:
> > 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?
>
> There's nothing technical preventing it. I just chose that
> capitalization arbitrarily and nobody objected.
>
I'll change it to RPi then, just to keep the consistency. Sorry I did
not flag this in your review rounds.
Regards,
Naush
>
>
> Paul
>
> >
> > 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
> >
> >
>
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210217/265b70bb/attachment-0001.htm>
More information about the libcamera-devel
mailing list