[libcamera-devel] [PATCH 07/13] pipeline: ipa: raspberrypi: Replace IPA metadataReady() call

[Naushir Patuck]

On Thu, 27 Apr 2023 at 16:02, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Apr 26, 2023 at 02:10:51PM +0100, Naushir Patuck via libcamera-devel wrote:
> > Remove the IPA -> pipeline metadataReady() call to signify the return
> > Request metdata is ready to be consumed. Instead, replace this with a
> > new pipeline -> IPA reportMetadata() call that explicitly ask the IPA
> > to fill in the Request metdata at an appropriate time.
> >
> > This change allows the pipeline handler to dictate when it receives the
> > metadata from the IPA if the structure of the pipeline were to ever
> > change.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom        |  2 ++
> >  src/ipa/rpi/vc4/raspberrypi.cpp                |  9 ++++-----
> >  src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp | 18 +++++++-----------
> >  3 files changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > index 7d56248f4ab3..620f13ba9717 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -82,6 +82,8 @@ interface IPARPiInterface {
> >
> >       [async] prepareIsp(PrepareParams params);
> >       [async] processStats(ProcessParams params);
> > +
> > +     reportMetadata(uint32 ipaContext) => (libcamera.ControlList controls);
>
> I'm afraid this can't be done. While streaming, only asynchronous calls
> are allowed, synchronous calls may block for too long and have adverse
> effects on the real time requirements on both sides.

Oops, that's not great.  I'm going to have to think of how to get around this!

Naush


> This is documented in Documentation/guides/ipa.rst:
>
> ----
> In addition, any call made after start() and before stop() must be
> asynchronous. The motivation for this is to avoid damaging real-time
> performance of the pipeline handler. If the pipeline handler wants some data
> from the IPA, the IPA should return the data asynchronously via an event
> (see "The Event IPA interface").
> ----
>
> >  };
> >
> >  interface IPARPiEventInterface {
> > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp
> > index d737f1d662a0..e3ca8e2f7cbe 100644
> > --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp
> > @@ -154,7 +154,7 @@ private:
> >       bool validateLensControls();
> >       void applyControls(const ControlList &controls);
> >       void prepare(const PrepareParams &params);
> > -     void reportMetadata(unsigned int ipaContext);
> > +     void reportMetadata(unsigned int ipaContext, ControlList *controls) override;
> >       void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> >       void process(unsigned int bufferId, unsigned int ipaContext);
> > @@ -565,7 +565,6 @@ void IPARPi::processStats(const ProcessParams &params)
> >       if (processPending_ && frameCount_ > mistrustCount_)
> >               process(params.buffers.stats, context);
> >
> > -     reportMetadata(context);
> >       processStatsComplete.emit(params.buffers);
> >  }
> >
> > @@ -586,9 +585,9 @@ void IPARPi::prepareIsp(const PrepareParams &params)
> >       prepareIspComplete.emit(params.buffers);
> >  }
> >
> > -void IPARPi::reportMetadata(unsigned int ipaContext)
> > +void IPARPi::reportMetadata(unsigned int ipaContext, ControlList *controls)
> >  {
> > -     RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > +     RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext % rpiMetadata_.size()];
> >       std::unique_lock<RPiController::Metadata> lock(rpiMetadata);
> >
> >       /*
> > @@ -697,7 +696,7 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
> >               libcameraMetadata_.set(controls::AfPauseState, p);
> >       }
> >
> > -     metadataReady.emit(libcameraMetadata_);
> > +     *controls = std::move(libcameraMetadata_);
> >  }
> >
> >  bool IPARPi::validateSensorControls()
> > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > index 30ce6feab0d1..bd66468683df 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > @@ -207,7 +207,6 @@ public:
> >       void enumerateVideoDevices(MediaLink *link);
> >
> >       void processStatsComplete(const ipa::RPi::BufferIds &buffers);
> > -     void metadataReady(const ControlList &metadata);
> >       void prepareIspComplete(const ipa::RPi::BufferIds &buffers);
> >       void setIspControls(const ControlList &controls);
> >       void setDelayedControls(const ControlList &controls, uint32_t delayContext);
> > @@ -1652,7 +1651,6 @@ int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)
> >
> >       ipa_->processStatsComplete.connect(this, &RPiCameraData::processStatsComplete);
> >       ipa_->prepareIspComplete.connect(this, &RPiCameraData::prepareIspComplete);
> > -     ipa_->metadataReady.connect(this, &RPiCameraData::metadataReady);
> >       ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> >       ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
> >       ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);
> > @@ -1892,17 +1890,12 @@ void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
> >       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(buffers.stats & RPi::MaskID);
> >
> >       handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> > -     state_ = State::IpaComplete;
> > -     handleState();
> > -}
> >
> > -void RPiCameraData::metadataReady(const ControlList &metadata)
> > -{
> > -     if (!isRunning())
> > -             return;
> > -
> > -     /* Add to the Request metadata buffer what the IPA has provided. */
> > +     /* Last thing to do is to fill up the request metadata. */
> >       Request *request = requestQueue_.front();
> > +     ControlList metadata;
> > +
> > +     ipa_->reportMetadata(request->sequence(), &metadata);
> >       request->metadata().merge(metadata);
> >
> >       /*
> > @@ -1923,6 +1916,9 @@ void RPiCameraData::metadataReady(const ControlList &metadata)
> >
> >               sensor_->setControls(&ctrls);
> >       }
> > +
> > +     state_ = State::IpaComplete;
> > +     handleState();
> >  }
> >
> >  void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
>
> --
> Regards,
>
> Laurent Pinchart