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

[Laurent Pinchart]

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. 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