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

[Jacopo Mondi]

Hi Naush

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>

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

> ---
>  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);
>  };
>
>  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)
> --
> 2.34.1
>