[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
Naushir Patuck
naush at raspberrypi.com
Wed Feb 17 10:59:36 CET 2021
Hi Paul,
On Wed, 17 Feb 2021 at 09:03, Naushir Patuck <naush at raspberrypi.com> wrote:
> Hi Paul,
>
> Thank you for your review feedback.
>
> On Wed, 17 Feb 2021 at 08:38, <paul.elder at ideasonboard.com> wrote:
>
>> 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.
>>
>
> I'm happy to do either. What do others feel is cleaner?
>
Actually having read the code in handleState(), it does not do anything
when state_ == State::Stopped anymore. So, these can simply be replaced
with a single return statement.
I'll make the change and submit a revision including the changes for the
ipa::rpi namespace change and the LS fixes. I will not add your R-B tag so
you have a chance to re-review if that's ok.
Regards,
Naush
>
>
>>
>> > + 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.
>>
>
> Great, I will make that change so we can remove the ctrls.set() call.
>
> Regards,
> Naush
>
>
>>
>> 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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210217/5b5c1fda/attachment-0001.htm>
More information about the libcamera-devel
mailing list