[libcamera-devel,06/13] pipeline: ipa: raspberrypi: Restructure the IPA mojom interface

18554 diff mbox series

Restructure the IPA mojom interface to be more consistent in the use
of the API. Function parameters are now grouped into *Params structures
and results are now returned in *Results structures.

The following pipeline -> IPA interfaces have been removed:

signalQueueRequest(libcamera.ControlList controls);
signalIspPrepare(ISPConfig data);
signalStatReady(uint32 bufferId, uint32 ipaContext);

and replaced with:

prepareIsp(PrepareParams params);
processStats(ProcessParams params);

signalQueueRequest() is now encompassed within signalPrepareIsp().

The following IPA -> pipeline interfaces have been removed:

runIsp(uint32 bufferId);
embeddedComplete(uint32 bufferId);
statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);

and replaced with the following async calls:

prepareIspComplete(BufferIds buffers);
processStatsComplete(BufferIds buffers);
metadataReady(libcamera.ControlList metadata);

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.mojom       | 127 +++++---------
 src/ipa/rpi/vc4/raspberrypi.cpp               | 102 +++++-------
 .../pipeline/rpi/vc4/raspberrypi.cpp          | 155 +++++++++---------
 3 files changed, 165 insertions(+), 219 deletions(-)

Hi Naush

On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:
> Restructure the IPA mojom interface to be more consistent in the use
> of the API. Function parameters are now grouped into *Params structures
> and results are now returned in *Results structures.
>
> The following pipeline -> IPA interfaces have been removed:
>
> signalQueueRequest(libcamera.ControlList controls);
> signalIspPrepare(ISPConfig data);
> signalStatReady(uint32 bufferId, uint32 ipaContext);
>
> and replaced with:
>
> prepareIsp(PrepareParams params);
> processStats(ProcessParams params);
>
> signalQueueRequest() is now encompassed within signalPrepareIsp().
>
> The following IPA -> pipeline interfaces have been removed:
>
> runIsp(uint32 bufferId);
> embeddedComplete(uint32 bufferId);
> statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
>
> and replaced with the following async calls:
>
> prepareIspComplete(BufferIds buffers);
> processStatsComplete(BufferIds buffers);
> metadataReady(libcamera.ControlList metadata);
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom       | 127 +++++---------
>  src/ipa/rpi/vc4/raspberrypi.cpp               | 102 +++++-------
>  .../pipeline/rpi/vc4/raspberrypi.cpp          | 155 +++++++++---------
>  3 files changed, 165 insertions(+), 219 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 80e0126618c8..7d56248f4ab3 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -8,7 +8,7 @@ module ipa.RPi;
>
>  import "include/libcamera/ipa/core.mojom";
>
> -/* Size of the LS grid allocation. */
> +/* Size of the LS grid allocation on VC4. */
>  const uint32 MaxLsGridSize = 0x8000;
>
>  struct SensorConfig {
> @@ -19,117 +19,78 @@ struct SensorConfig {
>  	uint32 sensorMetadata;
>  };
>
> -struct IPAInitResult {
> +struct InitParams {
> +	bool lensPresent;
> +};
> +
> +struct InitResult {
>  	SensorConfig sensorConfig;
>  	libcamera.ControlInfoMap controlInfo;
>  };
>
> -struct ISPConfig {
> -	uint32 embeddedBufferId;
> -	uint32 bayerBufferId;
> -	bool embeddedBufferPresent;
> -	libcamera.ControlList controls;
> -	uint32 ipaContext;
> -	uint32 delayContext;
> +struct BufferIds {
> +	uint32 bayer;
> +	uint32 embedded;
> +	uint32 stats;
>  };
>
> -struct IPAConfig {
> +struct ConfigParams {
>  	uint32 transform;
> -	libcamera.SharedFD lsTableHandle;
>  	libcamera.ControlInfoMap sensorControls;
>  	libcamera.ControlInfoMap ispControls;
>  	libcamera.ControlInfoMap lensControls;
> +        /* VC4 specifc */
> +	libcamera.SharedFD lsTableHandle;
>  };
>
> -struct IPAConfigResult {
> -       float modeSensitivity;
> -       libcamera.ControlInfoMap controlInfo;
> +struct ConfigResult {
> +	float modeSensitivity;
> +	libcamera.ControlInfoMap controlInfo;
> +	libcamera.ControlList controls;
>  };
>
> -struct StartConfig {
> +struct StartResult {
>  	libcamera.ControlList controls;
>  	int32 dropFrameCount;
>  };
>
> +struct PrepareParams {
> +	BufferIds buffers;
> +	libcamera.ControlList sensorControls;
> +	libcamera.ControlList requestControls;
> +	uint32 ipaContext;
> +	uint32 delayContext;
> +};
> +
> +struct ProcessParams {
> +	BufferIds buffers;
> +	uint32 ipaContext;
> +};
> +
>  interface IPARPiInterface {
> -	init(libcamera.IPASettings settings, bool lensPresent)
> -		=> (int32 ret, IPAInitResult result);
> -	start(libcamera.ControlList controls) => (StartConfig startConfig);
> +	init(libcamera.IPASettings settings, InitParams params)
> +		=> (int32 ret, InitResult result);
> +
> +	start(libcamera.ControlList controls) => (StartResult result);
>  	stop();
>
> -	/**
> -	 * \fn configure()

Is it intentional to drop documentation ? Was it used at all ?

> -	 * \brief Configure the IPA stream and sensor settings
> -	 * \param[in] sensorInfo Camera sensor information
> -	 * \param[in] ipaConfig Pipeline-handler-specific configuration data
> -	 * \param[out] controls Controls to apply by the pipeline entity
> -	 * \param[out] result Other results that the pipeline handler may require
> -	 *
> -	 * This function shall be called when the camera is configured to inform
> -	 * the IPA of the camera's streams and the sensor settings.
> -	 *
> -	 * The \a sensorInfo conveys information about the camera sensor settings that
> -	 * the pipeline handler has selected for the configuration.
> -	 *
> -	 * The \a ipaConfig and \a controls parameters carry data passed by the
> -	 * pipeline handler to the IPA and back.
> -	 */
> -	configure(libcamera.IPACameraSensorInfo sensorInfo,
> -		  IPAConfig ipaConfig)
> -		=> (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> -
> -	/**
> -	 * \fn mapBuffers()
> -	 * \brief Map buffers shared between the pipeline handler and the IPA
> -	 * \param[in] buffers List of buffers to map
> -	 *
> -	 * This function informs the IPA module of memory buffers set up by the
> -	 * pipeline handler that the IPA needs to access. It provides dmabuf
> -	 * file handles for each buffer, and associates the buffers with unique
> -	 * numerical IDs.
> -	 *
> -	 * IPAs shall map the dmabuf file handles to their address space and
> -	 * keep a cache of the mappings, indexed by the buffer numerical IDs.
> -	 * The IDs are used in all other IPA interface functions to refer to
> -	 * buffers, including the unmapBuffers() function.
> -	 *
> -	 * All buffers that the pipeline handler wishes to share with an IPA
> -	 * shall be mapped with this function. Buffers may be mapped all at once
> -	 * with a single call, or mapped and unmapped dynamically at runtime,
> -	 * depending on the IPA protocol. Regardless of the protocol, all
> -	 * buffers mapped at a given time shall have unique numerical IDs.
> -	 *
> -	 * The numerical IDs have no meaning defined by the IPA interface, and
> -	 * should be treated as opaque handles by IPAs, with the only exception
> -	 * that ID zero is invalid.
> -	 *
> -	 * \sa unmapBuffers()
> -	 */
> -	mapBuffers(array<libcamera.IPABuffer> buffers);
> +	configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)
> +		=> (int32 ret, ConfigResult result);
>
> -	/**
> -	 * \fn unmapBuffers()
> -	 * \brief Unmap buffers shared by the pipeline to the IPA
> -	 * \param[in] ids List of buffer IDs to unmap
> -	 *
> -	 * This function removes mappings set up with mapBuffers(). Numerical
> -	 * IDs of unmapped buffers may be reused when mapping new buffers.
> -	 *
> -	 * \sa mapBuffers()
> -	 */
> +	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
>
> -	[async] signalStatReady(uint32 bufferId, uint32 ipaContext);
> -	[async] signalQueueRequest(libcamera.ControlList controls);
> -	[async] signalIspPrepare(ISPConfig data);
> +	[async] prepareIsp(PrepareParams params);
> +	[async] processStats(ProcessParams params);
>  };
>
>  interface IPARPiEventInterface {
> -	statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> -	runIsp(uint32 bufferId);
> -	embeddedComplete(uint32 bufferId);
> +	prepareIspComplete(BufferIds buffers);
> +	processStatsComplete(BufferIds buffers);
> +	metadataReady(libcamera.ControlList metadata);
>  	setIspControls(libcamera.ControlList controls);
>  	setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
>  	setLensControls(libcamera.ControlList controls);
>  	setCameraTimeout(uint32 maxFrameLengthMs);
>  };
> +

Additional blank line

The rest looks good
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp
> index 841635ccde2b..d737f1d662a0 100644
> --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> +++ b/src/ipa/rpi/vc4/raspberrypi.cpp
> @@ -136,30 +136,28 @@ public:
>  			munmap(lsTable_, MaxLsGridSize);
>  	}
>
> -	int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;
> -	void start(const ControlList &controls, StartConfig *startConfig) override;
> +	int init(const IPASettings &settings, const InitParams &params, InitResult *result) override;
> +	void start(const ControlList &controls, StartResult *result) override;
>  	void stop() override {}
>
> -	int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,
> -		      ControlList *controls, IPAConfigResult *result) override;
> +	int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> +		      ConfigResult *result) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> -	void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;
> -	void signalQueueRequest(const ControlList &controls) override;
> -	void signalIspPrepare(const ISPConfig &data) override;
> +	void prepareIsp(const PrepareParams &params) override;
> +	void processStats(const ProcessParams &params) override;
>
>  private:
>  	void setMode(const IPACameraSensorInfo &sensorInfo);
>  	bool validateSensorControls();
>  	bool validateIspControls();
>  	bool validateLensControls();
> -	void queueRequest(const ControlList &controls);
> -	void returnEmbeddedBuffer(unsigned int bufferId);
> -	void prepareISP(const ISPConfig &data);
> +	void applyControls(const ControlList &controls);
> +	void prepare(const PrepareParams &params);
>  	void reportMetadata(unsigned int ipaContext);
>  	void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
>  	RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> -	void processStats(unsigned int bufferId, unsigned int ipaContext);
> +	void process(unsigned int bufferId, unsigned int ipaContext);
>  	void setCameraTimeoutValue();
>  	void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
>  	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> @@ -229,7 +227,7 @@ private:
>  	Duration lastTimeout_;
>  };
>
> -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> +int IPARPi::init(const IPASettings &settings, const InitParams &params, InitResult *result)
>  {
>  	/*
>  	 * Load the "helper" for this sensor. This tells us all the device specific stuff
> @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
>  		return -EINVAL;
>  	}
>
> -	lensPresent_ = lensPresent;
> +	lensPresent_ = params.lensPresent;
>
>  	controller_.initialise();
>
> @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
>  	return 0;
>  }
>
> -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> +void IPARPi::start(const ControlList &controls, StartResult *result)
>  {
>  	RPiController::Metadata metadata;
>
> -	ASSERT(startConfig);
>  	if (!controls.empty()) {
>  		/* We have been given some controls to action before start. */
> -		queueRequest(controls);
> +		applyControls(controls);
>  	}
>
>  	controller_.switchMode(mode_, &metadata);
> @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  	if (agcStatus.shutterTime && agcStatus.analogueGain) {
>  		ControlList ctrls(sensorCtrls_);
>  		applyAGC(&agcStatus, ctrls);
> -		startConfig->controls = std::move(ctrls);
> +		result->controls = std::move(ctrls);
>  		setCameraTimeoutValue();
>  	}
>
> @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
>  		mistrustCount_ = helper_->mistrustFramesModeSwitch();
>  	}
>
> -	startConfig->dropFrameCount = dropFrameCount_;
> +	result->dropFrameCount = dropFrameCount_;
>
>  	firstStart_ = false;
>  	lastRunTimestamp_ = 0;
> @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
>  			     mode_.minFrameDuration, mode_.maxFrameDuration);
>  }
>
> -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,
> -		      ControlList *controls, IPAConfigResult *result)
> +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> +		      ConfigResult *result)
>  {
> -	sensorCtrls_ = ipaConfig.sensorControls;
> -	ispCtrls_ = ipaConfig.ispControls;
> +	sensorCtrls_ = params.sensorControls;
> +	ispCtrls_ = params.ispControls;
>
>  	if (!validateSensorControls()) {
>  		LOG(IPARPI, Error) << "Sensor control validation failed.";
> @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>  	}
>
>  	if (lensPresent_) {
> -		lensCtrls_ = ipaConfig.lensControls;
> +		lensCtrls_ = params.lensControls;
>  		if (!validateLensControls()) {
>  			LOG(IPARPI, Warning) << "Lens validation failed, "
>  					     << "no lens control will be available.";
> @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>  	/* Re-assemble camera mode using the sensor info. */
>  	setMode(sensorInfo);
>
> -	mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform);
> +	mode_.transform = static_cast<libcamera::Transform>(params.transform);
>
>  	/* Store the lens shading table pointer and handle if available. */
> -	if (ipaConfig.lsTableHandle.isValid()) {
> +	if (params.lsTableHandle.isValid()) {
>  		/* Remove any previous table, if there was one. */
>  		if (lsTable_) {
>  			munmap(lsTable_, MaxLsGridSize);
> @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>  		}
>
>  		/* Map the LS table buffer into user space. */
> -		lsTableHandle_ = std::move(ipaConfig.lsTableHandle);
> +		lsTableHandle_ = std::move(params.lsTableHandle);
>  		if (lsTableHandle_.isValid()) {
>  			lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,
>  					MAP_SHARED, lsTableHandle_.get(), 0);
> @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
>  		applyAGC(&agcStatus, ctrls);
>  	}
>
> -	ASSERT(controls);
> -	*controls = std::move(ctrls);
> +	result->controls = std::move(ctrls);
>
>  	/*
>  	 * Apply the correct limits to the exposure, gain and frame duration controls
> @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>
> -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)
> +void IPARPi::processStats(const ProcessParams &params)
>  {
> -	unsigned int context = ipaContext % rpiMetadata_.size();
> +	unsigned int context = params.ipaContext % rpiMetadata_.size();
>
>  	if (++checkCount_ != frameCount_) /* assert here? */
>  		LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
>  	if (processPending_ && frameCount_ > mistrustCount_)
> -		processStats(bufferId, context);
> +		process(params.buffers.stats, context);
>
>  	reportMetadata(context);
> -
> -	statsMetadataComplete.emit(bufferId, libcameraMetadata_);
> +	processStatsComplete.emit(params.buffers);
>  }
>
> -void IPARPi::signalQueueRequest(const ControlList &controls)
> -{
> -	queueRequest(controls);
> -}
>
> -void IPARPi::signalIspPrepare(const ISPConfig &data)
> +void IPARPi::prepareIsp(const PrepareParams &params)
>  {
> +	applyControls(params.requestControls);
> +
>  	/*
>  	 * At start-up, or after a mode-switch, we may want to
>  	 * avoid running the control algos for a few frames in case
>  	 * they are "unreliable".
>  	 */
> -	prepareISP(data);
> +	prepare(params);
>  	frameCount_++;
>
>  	/* Ready to push the input buffer into the ISP. */
> -	runIsp.emit(data.bayerBufferId);
> +	prepareIspComplete.emit(params.buffers);
>  }
>
>  void IPARPi::reportMetadata(unsigned int ipaContext)
> @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
>  		libcameraMetadata_.set(controls::AfState, s);
>  		libcameraMetadata_.set(controls::AfPauseState, p);
>  	}
> +
> +	metadataReady.emit(libcameraMetadata_);
>  }
>
>  bool IPARPi::validateSensorControls()
> @@ -826,7 +821,7 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
>  	{ controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
>  };
>
> -void IPARPi::queueRequest(const ControlList &controls)
> +void IPARPi::applyControls(const ControlList &controls)
>  {
>  	using RPiController::AfAlgorithm;
>
> @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)
>  	}
>  }
>
> -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> +void IPARPi::prepare(const PrepareParams &params)
>  {
> -	embeddedComplete.emit(bufferId);
> -}
> -
> -void IPARPi::prepareISP(const ISPConfig &data)
> -{
> -	int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> -	unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();
> +	int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);
> +	unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
>  	RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
>  	Span<uint8_t> embeddedBuffer;
>
>  	rpiMetadata.clear();
> -	fillDeviceStatus(data.controls, ipaContext);
> +	fillDeviceStatus(params.sensorControls, ipaContext);
>
> -	if (data.embeddedBufferPresent) {
> +	if (params.buffers.embedded) {
>  		/*
>  		 * Pipeline handler has supplied us with an embedded data buffer,
>  		 * we must pass it to the CamHelper for parsing.
>  		 */
> -		auto it = buffers_.find(data.embeddedBufferId);
> +		auto it = buffers_.find(params.buffers.embedded);
>  		ASSERT(it != buffers_.end());
>  		embeddedBuffer = it->second.planes()[0];
>  	}
> @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
>  	 * metadata.
>  	 */
>  	AgcStatus agcStatus;
> -	RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];
> +	RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
>  	if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
>  		rpiMetadata.set("agc.delayed_status", agcStatus);
>
> @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)
>  	 */
>  	helper_->prepare(embeddedBuffer, rpiMetadata);
>
> -	/* Done with embedded data now, return to pipeline handler asap. */
> -	if (data.embeddedBufferPresent)
> -		returnEmbeddedBuffer(data.embeddedBufferId);
> -
>  	/* Allow a 10% margin on the comparison below. */
>  	Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
>  	if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
>  	return statistics;
>  }
>
> -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)
>  {
>  	RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
>
> diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> index e54bf1ef2c17..30ce6feab0d1 100644
> --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> @@ -200,15 +200,15 @@ public:
>  	void freeBuffers();
>  	void frameStarted(uint32_t sequence);
>
> -	int loadIPA(ipa::RPi::IPAInitResult *result);
> -	int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
> +	int loadIPA(ipa::RPi::InitResult *result);
> +	int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);
>  	int loadPipelineConfiguration();
>
>  	void enumerateVideoDevices(MediaLink *link);
>
> -	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> -	void runIsp(uint32_t bufferId);
> -	void embeddedComplete(uint32_t bufferId);
> +	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);
>  	void setLensControls(const ControlList &controls);
> @@ -238,7 +238,7 @@ public:
>  	/* The vector below is just for convenience when iterating over all streams. */
>  	std::vector<RPi::Stream *> streams_;
>  	/* Stores the ids of the buffers mapped in the IPA. */
> -	std::unordered_set<unsigned int> ipaBuffers_;
> +	std::unordered_set<unsigned int> bufferIds_;
>  	/*
>  	 * Stores a cascade of Video Mux or Bridge devices between the sensor and
>  	 * Unicam together with media link across the entities.
> @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>
> -	ipa::RPi::IPAConfigResult result;
> +	ipa::RPi::ConfigResult result;
>  	ret = data->configureIPA(config, &result);
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  		data->applyScalerCrop(*controls);
>
>  	/* Start the IPA. */
> -	ipa::RPi::StartConfig startConfig;
> +	ipa::RPi::StartResult result;
>  	data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> -			  &startConfig);
> +			  &result);
>
>  	/* Apply any gain/exposure settings that the IPA may have passed back. */
> -	if (!startConfig.controls.empty())
> -		data->setSensorControls(startConfig.controls);
> +	if (!result.controls.empty())
> +		data->setSensorControls(result.controls);
>
>  	/* Configure the number of dropped frames required on startup. */
>  	data->dropFrameCount_ = data->config_.disableStartupFrameDrops
> -			      ? 0 : startConfig.dropFrameCount;
> +				? 0 : result.dropFrameCount;
>
>  	for (auto const stream : data->streams_)
>  		stream->resetBuffers();
> @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>
>  	data->sensorFormats_ = populateSensorFormats(data->sensor_);
>
> -	ipa::RPi::IPAInitResult result;
> +	ipa::RPi::InitResult result;
>  	if (data->loadIPA(&result)) {
>  		LOG(RPI, Error) << "Failed to load a suitable IPA library";
>  		return -EINVAL;
> @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)
>  {
>  	RPiCameraData *data = cameraData(camera);
> -	std::vector<IPABuffer> ipaBuffers;
> +	std::vector<IPABuffer> bufferIds;
>  	/*
>  	 * Link the FrameBuffers with the id (key value) in the map stored in
>  	 * the RPi stream object - along with an identifier mask.
> @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
>  	 * handler and the IPA.
>  	 */
>  	for (auto const &it : buffers) {
> -		ipaBuffers.push_back(IPABuffer(mask | it.first,
> +		bufferIds.push_back(IPABuffer(mask | it.first,
>  					       it.second->planes()));
> -		data->ipaBuffers_.insert(mask | it.first);
> +		data->bufferIds_.insert(mask | it.first);
>  	}
>
> -	data->ipa_->mapBuffers(ipaBuffers);
> +	data->ipa_->mapBuffers(bufferIds);
>  }
>
>  void RPiCameraData::freeBuffers()
> @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()
>  		 * Copy the buffer ids from the unordered_set to a vector to
>  		 * pass to the IPA.
>  		 */
> -		std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(),
> -						     ipaBuffers_.end());
> -		ipa_->unmapBuffers(ipaBuffers);
> -		ipaBuffers_.clear();
> +		std::vector<unsigned int> bufferIds(bufferIds_.begin(),
> +						    bufferIds_.end());
> +		ipa_->unmapBuffers(bufferIds);
> +		bufferIds_.clear();
>  	}
>
>  	for (auto const stream : streams_)
> @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)
>  	delayedCtrls_->applyControls(sequence);
>  }
>
> -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)
>  {
>  	ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
>
>  	if (!ipa_)
>  		return -ENOENT;
>
> -	ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> -	ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> -	ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> +	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);
> @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
>  	}
>
>  	IPASettings settings(configurationFile, sensor_->model());
> +	ipa::RPi::InitParams params;
>
> -	return ipa_->init(settings, !!sensor_->focusLens(), result);
> +	params.lensPresent = !!sensor_->focusLens();
> +	return ipa_->init(settings, params, result);
>  }
>
> -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
> +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)
>  {
>  	std::map<unsigned int, ControlInfoMap> entityControls;
> -	ipa::RPi::IPAConfig ipaConfig;
> +	ipa::RPi::ConfigParams params;
>
>  	/* \todo Move passing of ispControls and lensControls to ipa::init() */
> -	ipaConfig.sensorControls = sensor_->controls();
> -	ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();
> +	params.sensorControls = sensor_->controls();
> +	params.ispControls = isp_[Isp::Input].dev()->controls();
>  	if (sensor_->focusLens())
> -		ipaConfig.lensControls = sensor_->focusLens()->controls();
> +		params.lensControls = sensor_->focusLens()->controls();
>
>  	/* Always send the user transform to the IPA. */
> -	ipaConfig.transform = static_cast<unsigned int>(config->transform);
> +	params.transform = static_cast<unsigned int>(config->transform);
>
>  	/* Allocate the lens shading table via dmaHeap and pass to the IPA. */
>  	if (!lsTable_.isValid()) {
> @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
>  		 * \todo Investigate if mapping the lens shading table buffer
>  		 * could be handled with mapBuffers().
>  		 */
> -		ipaConfig.lsTableHandle = lsTable_;
> +		params.lsTableHandle = lsTable_;
>  	}
>
>  	/* We store the IPACameraSensorInfo for digital zoom calculations. */
> @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
>  	}
>
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	ControlList controls;
> -	ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);
> +	ret = ipa_->configure(sensorInfo_, params, result);
>  	if (ret < 0) {
>  		LOG(RPI, Error) << "IPA configuration failed!";
>  		return -EPIPE;
>  	}
>
> -	if (!controls.empty())
> -		setSensorControls(controls);
> +	if (!result->controls.empty())
> +		setSensorControls(result->controls);
>
>  	return 0;
>  }
> @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)
>  	}
>  }
>
> -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
>  {
>  	if (!isRunning())
>  		return;
>
> -	FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);
> +	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. */
>  	Request *request = requestQueue_.front();
> -	request->metadata().merge(controls);
> +	request->metadata().merge(metadata);
>
>  	/*
>  	 * Inform the sensor of the latest colour gains if it has the
>  	 * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
>  	 */
> -	const auto &colourGains = controls.get(libcamera::controls::ColourGains);
> +	const auto &colourGains = metadata.get(libcamera::controls::ColourGains);
>  	if (notifyGainsUnity_ && colourGains) {
>  		/* The control wants linear gains in the order B, Gb, Gr, R. */
>  		ControlList ctrls(sensor_->controls());
> @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>
>  		sensor_->setControls(&ctrls);
>  	}
> -
> -	state_ = State::IpaComplete;
> -	handleState();
>  }
>
> -void RPiCameraData::runIsp(uint32_t bufferId)
> +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
>  {
> +	unsigned int embeddedId = buffers.embedded & RPi::MaskID;
> +	unsigned int bayer = buffers.bayer & RPi::MaskID;
> +	FrameBuffer *buffer;
> +
>  	if (!isRunning())
>  		return;
>
> -	FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);
> -
> -	LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bufferId & RPi::MaskID)
> +	buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);
> +	LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID)
>  			<< ", timestamp: " << buffer->metadata().timestamp;
>
>  	isp_[Isp::Input].queueBuffer(buffer);
>  	ispOutputCount_ = 0;
> -	handleState();
> -}
>
> -void RPiCameraData::embeddedComplete(uint32_t bufferId)
> -{
> -	if (!isRunning())
> -		return;
> +	if (sensorMetadata_ && embeddedId) {
> +		buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);
> +		handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> +	}
>
> -	FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);
> -	handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>  	handleState();
>  }
>
> @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  	 * application until after the IPA signals so.
>  	 */
>  	if (stream == &isp_[Isp::Stats]) {
> -		ipa_->signalStatReady(RPi::MaskStats | static_cast<unsigned int>(index),
> -				      requestQueue_.front()->sequence());
> +		ipa::RPi::ProcessParams params;
> +		params.buffers.stats = index | RPi::MaskStats;
> +		params.ipaContext = requestQueue_.front()->sequence();
> +		ipa_->processStats(params);
>  	} else {
>  		/* Any other ISP output can be handed back to the application now. */
>  		handleStreamBuffer(buffer, stream);
> @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()
>  	request->metadata().clear();
>  	fillRequestMetadata(bayerFrame.controls, request);
>
> -	/*
> -	 * Process all the user controls by the IPA. Once this is complete, we
> -	 * queue the ISP output buffer listed in the request to start the HW
> -	 * pipeline.
> -	 */
> -	ipa_->signalQueueRequest(request->controls());
> -
>  	/* Set our state to say the pipeline is active. */
>  	state_ = State::Busy;
>
> -	unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> +	unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
>
> -	LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> -			<< " Bayer buffer id: " << bayerId;
> +	LOG(RPI, Debug) << "Signalling prepareIsp:"
> +			<< " Bayer buffer id: " << bayer;
>
> -	ipa::RPi::ISPConfig ispPrepare;
> -	ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;
> -	ispPrepare.controls = std::move(bayerFrame.controls);
> -	ispPrepare.ipaContext = request->sequence();
> -	ispPrepare.delayContext = bayerFrame.delayContext;
> +	ipa::RPi::PrepareParams params;
> +	params.buffers.bayer = RPi::MaskBayerData | bayer;
> +	params.sensorControls = std::move(bayerFrame.controls);
> +	params.requestControls = request->controls();
> +	params.ipaContext = request->sequence();
> +	params.delayContext = bayerFrame.delayContext;
>
>  	if (embeddedBuffer) {
>  		unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>
> -		ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;
> -		ispPrepare.embeddedBufferPresent = true;
> -
> -		LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> +		params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;
> +		LOG(RPI, Debug) << "Signalling prepareIsp:"
>  				<< " Embedded buffer id: " << embeddedId;
>  	}
>
> -	ipa_->signalIspPrepare(ispPrepare);
> +	ipa_->prepareIsp(params);
>  }
>
>  bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)
> --
> 2.34.1
>

Hi Jacopo,

On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:
> > Restructure the IPA mojom interface to be more consistent in the use
> > of the API. Function parameters are now grouped into *Params structures
> > and results are now returned in *Results structures.
> >
> > The following pipeline -> IPA interfaces have been removed:
> >
> > signalQueueRequest(libcamera.ControlList controls);
> > signalIspPrepare(ISPConfig data);
> > signalStatReady(uint32 bufferId, uint32 ipaContext);
> >
> > and replaced with:
> >
> > prepareIsp(PrepareParams params);
> > processStats(ProcessParams params);
> >
> > signalQueueRequest() is now encompassed within signalPrepareIsp().
> >
> > The following IPA -> pipeline interfaces have been removed:
> >
> > runIsp(uint32 bufferId);
> > embeddedComplete(uint32 bufferId);
> > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> >
> > and replaced with the following async calls:
> >
> > prepareIspComplete(BufferIds buffers);
> > processStatsComplete(BufferIds buffers);
> > metadataReady(libcamera.ControlList metadata);
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom       | 127 +++++---------
> >  src/ipa/rpi/vc4/raspberrypi.cpp               | 102 +++++-------
> >  .../pipeline/rpi/vc4/raspberrypi.cpp          | 155 +++++++++---------
> >  3 files changed, 165 insertions(+), 219 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > index 80e0126618c8..7d56248f4ab3 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -8,7 +8,7 @@ module ipa.RPi;
> >
> >  import "include/libcamera/ipa/core.mojom";
> >
> > -/* Size of the LS grid allocation. */
> > +/* Size of the LS grid allocation on VC4. */
> >  const uint32 MaxLsGridSize = 0x8000;
> >
> >  struct SensorConfig {
> > @@ -19,117 +19,78 @@ struct SensorConfig {
> >       uint32 sensorMetadata;
> >  };
> >
> > -struct IPAInitResult {
> > +struct InitParams {
> > +     bool lensPresent;
> > +};
> > +
> > +struct InitResult {
> >       SensorConfig sensorConfig;
> >       libcamera.ControlInfoMap controlInfo;
> >  };
> >
> > -struct ISPConfig {
> > -     uint32 embeddedBufferId;
> > -     uint32 bayerBufferId;
> > -     bool embeddedBufferPresent;
> > -     libcamera.ControlList controls;
> > -     uint32 ipaContext;
> > -     uint32 delayContext;
> > +struct BufferIds {
> > +     uint32 bayer;
> > +     uint32 embedded;
> > +     uint32 stats;
> >  };
> >
> > -struct IPAConfig {
> > +struct ConfigParams {
> >       uint32 transform;
> > -     libcamera.SharedFD lsTableHandle;
> >       libcamera.ControlInfoMap sensorControls;
> >       libcamera.ControlInfoMap ispControls;
> >       libcamera.ControlInfoMap lensControls;
> > +        /* VC4 specifc */
> > +     libcamera.SharedFD lsTableHandle;
> >  };
> >
> > -struct IPAConfigResult {
> > -       float modeSensitivity;
> > -       libcamera.ControlInfoMap controlInfo;
> > +struct ConfigResult {
> > +     float modeSensitivity;
> > +     libcamera.ControlInfoMap controlInfo;
> > +     libcamera.ControlList controls;
> >  };
> >
> > -struct StartConfig {
> > +struct StartResult {
> >       libcamera.ControlList controls;
> >       int32 dropFrameCount;
> >  };
> >
> > +struct PrepareParams {
> > +     BufferIds buffers;
> > +     libcamera.ControlList sensorControls;
> > +     libcamera.ControlList requestControls;
> > +     uint32 ipaContext;
> > +     uint32 delayContext;
> > +};
> > +
> > +struct ProcessParams {
> > +     BufferIds buffers;
> > +     uint32 ipaContext;
> > +};
> > +
> >  interface IPARPiInterface {
> > -     init(libcamera.IPASettings settings, bool lensPresent)
> > -             => (int32 ret, IPAInitResult result);
> > -     start(libcamera.ControlList controls) => (StartConfig startConfig);
> > +     init(libcamera.IPASettings settings, InitParams params)
> > +             => (int32 ret, InitResult result);
> > +
> > +     start(libcamera.ControlList controls) => (StartResult result);
> >       stop();
> >
> > -     /**
> > -      * \fn configure()
>
> Is it intentional to drop documentation ? Was it used at all ?

I don't believe it is at all used.  I looked at ipu3 and rkisp1 and they have
no doc tags either.

>
> > -      * \brief Configure the IPA stream and sensor settings
> > -      * \param[in] sensorInfo Camera sensor information
> > -      * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > -      * \param[out] controls Controls to apply by the pipeline entity
> > -      * \param[out] result Other results that the pipeline handler may require
> > -      *
> > -      * This function shall be called when the camera is configured to inform
> > -      * the IPA of the camera's streams and the sensor settings.
> > -      *
> > -      * The \a sensorInfo conveys information about the camera sensor settings that
> > -      * the pipeline handler has selected for the configuration.
> > -      *
> > -      * The \a ipaConfig and \a controls parameters carry data passed by the
> > -      * pipeline handler to the IPA and back.
> > -      */
> > -     configure(libcamera.IPACameraSensorInfo sensorInfo,
> > -               IPAConfig ipaConfig)
> > -             => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > -
> > -     /**
> > -      * \fn mapBuffers()
> > -      * \brief Map buffers shared between the pipeline handler and the IPA
> > -      * \param[in] buffers List of buffers to map
> > -      *
> > -      * This function informs the IPA module of memory buffers set up by the
> > -      * pipeline handler that the IPA needs to access. It provides dmabuf
> > -      * file handles for each buffer, and associates the buffers with unique
> > -      * numerical IDs.
> > -      *
> > -      * IPAs shall map the dmabuf file handles to their address space and
> > -      * keep a cache of the mappings, indexed by the buffer numerical IDs.
> > -      * The IDs are used in all other IPA interface functions to refer to
> > -      * buffers, including the unmapBuffers() function.
> > -      *
> > -      * All buffers that the pipeline handler wishes to share with an IPA
> > -      * shall be mapped with this function. Buffers may be mapped all at once
> > -      * with a single call, or mapped and unmapped dynamically at runtime,
> > -      * depending on the IPA protocol. Regardless of the protocol, all
> > -      * buffers mapped at a given time shall have unique numerical IDs.
> > -      *
> > -      * The numerical IDs have no meaning defined by the IPA interface, and
> > -      * should be treated as opaque handles by IPAs, with the only exception
> > -      * that ID zero is invalid.
> > -      *
> > -      * \sa unmapBuffers()
> > -      */
> > -     mapBuffers(array<libcamera.IPABuffer> buffers);
> > +     configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)
> > +             => (int32 ret, ConfigResult result);
> >
> > -     /**
> > -      * \fn unmapBuffers()
> > -      * \brief Unmap buffers shared by the pipeline to the IPA
> > -      * \param[in] ids List of buffer IDs to unmap
> > -      *
> > -      * This function removes mappings set up with mapBuffers(). Numerical
> > -      * IDs of unmapped buffers may be reused when mapping new buffers.
> > -      *
> > -      * \sa mapBuffers()
> > -      */
> > +     mapBuffers(array<libcamera.IPABuffer> buffers);
> >       unmapBuffers(array<uint32> ids);
> >
> > -     [async] signalStatReady(uint32 bufferId, uint32 ipaContext);
> > -     [async] signalQueueRequest(libcamera.ControlList controls);
> > -     [async] signalIspPrepare(ISPConfig data);
> > +     [async] prepareIsp(PrepareParams params);
> > +     [async] processStats(ProcessParams params);
> >  };
> >
> >  interface IPARPiEventInterface {
> > -     statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > -     runIsp(uint32 bufferId);
> > -     embeddedComplete(uint32 bufferId);
> > +     prepareIspComplete(BufferIds buffers);
> > +     processStatsComplete(BufferIds buffers);
> > +     metadataReady(libcamera.ControlList metadata);
> >       setIspControls(libcamera.ControlList controls);
> >       setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
> >       setLensControls(libcamera.ControlList controls);
> >       setCameraTimeout(uint32 maxFrameLengthMs);
> >  };
> > +
>
> Additional blank line

Ack

Regards,
Naush

>
> The rest looks good
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>   j
>
> > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp
> > index 841635ccde2b..d737f1d662a0 100644
> > --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp
> > @@ -136,30 +136,28 @@ public:
> >                       munmap(lsTable_, MaxLsGridSize);
> >       }
> >
> > -     int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;
> > -     void start(const ControlList &controls, StartConfig *startConfig) override;
> > +     int init(const IPASettings &settings, const InitParams &params, InitResult *result) override;
> > +     void start(const ControlList &controls, StartResult *result) override;
> >       void stop() override {}
> >
> > -     int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,
> > -                   ControlList *controls, IPAConfigResult *result) override;
> > +     int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > +                   ConfigResult *result) override;
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > -     void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;
> > -     void signalQueueRequest(const ControlList &controls) override;
> > -     void signalIspPrepare(const ISPConfig &data) override;
> > +     void prepareIsp(const PrepareParams &params) override;
> > +     void processStats(const ProcessParams &params) override;
> >
> >  private:
> >       void setMode(const IPACameraSensorInfo &sensorInfo);
> >       bool validateSensorControls();
> >       bool validateIspControls();
> >       bool validateLensControls();
> > -     void queueRequest(const ControlList &controls);
> > -     void returnEmbeddedBuffer(unsigned int bufferId);
> > -     void prepareISP(const ISPConfig &data);
> > +     void applyControls(const ControlList &controls);
> > +     void prepare(const PrepareParams &params);
> >       void reportMetadata(unsigned int ipaContext);
> >       void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> > -     void processStats(unsigned int bufferId, unsigned int ipaContext);
> > +     void process(unsigned int bufferId, unsigned int ipaContext);
> >       void setCameraTimeoutValue();
> >       void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
> >       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> > @@ -229,7 +227,7 @@ private:
> >       Duration lastTimeout_;
> >  };
> >
> > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> > +int IPARPi::init(const IPASettings &settings, const InitParams &params, InitResult *result)
> >  {
> >       /*
> >        * Load the "helper" for this sensor. This tells us all the device specific stuff
> > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> >               return -EINVAL;
> >       }
> >
> > -     lensPresent_ = lensPresent;
> > +     lensPresent_ = params.lensPresent;
> >
> >       controller_.initialise();
> >
> > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> >       return 0;
> >  }
> >
> > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > +void IPARPi::start(const ControlList &controls, StartResult *result)
> >  {
> >       RPiController::Metadata metadata;
> >
> > -     ASSERT(startConfig);
> >       if (!controls.empty()) {
> >               /* We have been given some controls to action before start. */
> > -             queueRequest(controls);
> > +             applyControls(controls);
> >       }
> >
> >       controller_.switchMode(mode_, &metadata);
> > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> >       if (agcStatus.shutterTime && agcStatus.analogueGain) {
> >               ControlList ctrls(sensorCtrls_);
> >               applyAGC(&agcStatus, ctrls);
> > -             startConfig->controls = std::move(ctrls);
> > +             result->controls = std::move(ctrls);
> >               setCameraTimeoutValue();
> >       }
> >
> > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> >               mistrustCount_ = helper_->mistrustFramesModeSwitch();
> >       }
> >
> > -     startConfig->dropFrameCount = dropFrameCount_;
> > +     result->dropFrameCount = dropFrameCount_;
> >
> >       firstStart_ = false;
> >       lastRunTimestamp_ = 0;
> > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> >                            mode_.minFrameDuration, mode_.maxFrameDuration);
> >  }
> >
> > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,
> > -                   ControlList *controls, IPAConfigResult *result)
> > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > +                   ConfigResult *result)
> >  {
> > -     sensorCtrls_ = ipaConfig.sensorControls;
> > -     ispCtrls_ = ipaConfig.ispControls;
> > +     sensorCtrls_ = params.sensorControls;
> > +     ispCtrls_ = params.ispControls;
> >
> >       if (!validateSensorControls()) {
> >               LOG(IPARPI, Error) << "Sensor control validation failed.";
> > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >       }
> >
> >       if (lensPresent_) {
> > -             lensCtrls_ = ipaConfig.lensControls;
> > +             lensCtrls_ = params.lensControls;
> >               if (!validateLensControls()) {
> >                       LOG(IPARPI, Warning) << "Lens validation failed, "
> >                                            << "no lens control will be available.";
> > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >       /* Re-assemble camera mode using the sensor info. */
> >       setMode(sensorInfo);
> >
> > -     mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform);
> > +     mode_.transform = static_cast<libcamera::Transform>(params.transform);
> >
> >       /* Store the lens shading table pointer and handle if available. */
> > -     if (ipaConfig.lsTableHandle.isValid()) {
> > +     if (params.lsTableHandle.isValid()) {
> >               /* Remove any previous table, if there was one. */
> >               if (lsTable_) {
> >                       munmap(lsTable_, MaxLsGridSize);
> > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >               }
> >
> >               /* Map the LS table buffer into user space. */
> > -             lsTableHandle_ = std::move(ipaConfig.lsTableHandle);
> > +             lsTableHandle_ = std::move(params.lsTableHandle);
> >               if (lsTableHandle_.isValid()) {
> >                       lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,
> >                                       MAP_SHARED, lsTableHandle_.get(), 0);
> > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >               applyAGC(&agcStatus, ctrls);
> >       }
> >
> > -     ASSERT(controls);
> > -     *controls = std::move(ctrls);
> > +     result->controls = std::move(ctrls);
> >
> >       /*
> >        * Apply the correct limits to the exposure, gain and frame duration controls
> > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
> >       }
> >  }
> >
> > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)
> > +void IPARPi::processStats(const ProcessParams &params)
> >  {
> > -     unsigned int context = ipaContext % rpiMetadata_.size();
> > +     unsigned int context = params.ipaContext % rpiMetadata_.size();
> >
> >       if (++checkCount_ != frameCount_) /* assert here? */
> >               LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> >       if (processPending_ && frameCount_ > mistrustCount_)
> > -             processStats(bufferId, context);
> > +             process(params.buffers.stats, context);
> >
> >       reportMetadata(context);
> > -
> > -     statsMetadataComplete.emit(bufferId, libcameraMetadata_);
> > +     processStatsComplete.emit(params.buffers);
> >  }
> >
> > -void IPARPi::signalQueueRequest(const ControlList &controls)
> > -{
> > -     queueRequest(controls);
> > -}
> >
> > -void IPARPi::signalIspPrepare(const ISPConfig &data)
> > +void IPARPi::prepareIsp(const PrepareParams &params)
> >  {
> > +     applyControls(params.requestControls);
> > +
> >       /*
> >        * At start-up, or after a mode-switch, we may want to
> >        * avoid running the control algos for a few frames in case
> >        * they are "unreliable".
> >        */
> > -     prepareISP(data);
> > +     prepare(params);
> >       frameCount_++;
> >
> >       /* Ready to push the input buffer into the ISP. */
> > -     runIsp.emit(data.bayerBufferId);
> > +     prepareIspComplete.emit(params.buffers);
> >  }
> >
> >  void IPARPi::reportMetadata(unsigned int ipaContext)
> > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
> >               libcameraMetadata_.set(controls::AfState, s);
> >               libcameraMetadata_.set(controls::AfPauseState, p);
> >       }
> > +
> > +     metadataReady.emit(libcameraMetadata_);
> >  }
> >
> >  bool IPARPi::validateSensorControls()
> > @@ -826,7 +821,7 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
> >       { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
> >  };
> >
> > -void IPARPi::queueRequest(const ControlList &controls)
> > +void IPARPi::applyControls(const ControlList &controls)
> >  {
> >       using RPiController::AfAlgorithm;
> >
> > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)
> >       }
> >  }
> >
> > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > +void IPARPi::prepare(const PrepareParams &params)
> >  {
> > -     embeddedComplete.emit(bufferId);
> > -}
> > -
> > -void IPARPi::prepareISP(const ISPConfig &data)
> > -{
> > -     int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> > -     unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();
> > +     int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);
> > +     unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
> >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> >       Span<uint8_t> embeddedBuffer;
> >
> >       rpiMetadata.clear();
> > -     fillDeviceStatus(data.controls, ipaContext);
> > +     fillDeviceStatus(params.sensorControls, ipaContext);
> >
> > -     if (data.embeddedBufferPresent) {
> > +     if (params.buffers.embedded) {
> >               /*
> >                * Pipeline handler has supplied us with an embedded data buffer,
> >                * we must pass it to the CamHelper for parsing.
> >                */
> > -             auto it = buffers_.find(data.embeddedBufferId);
> > +             auto it = buffers_.find(params.buffers.embedded);
> >               ASSERT(it != buffers_.end());
> >               embeddedBuffer = it->second.planes()[0];
> >       }
> > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
> >        * metadata.
> >        */
> >       AgcStatus agcStatus;
> > -     RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];
> > +     RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
> >       if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> >               rpiMetadata.set("agc.delayed_status", agcStatus);
> >
> > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)
> >        */
> >       helper_->prepare(embeddedBuffer, rpiMetadata);
> >
> > -     /* Done with embedded data now, return to pipeline handler asap. */
> > -     if (data.embeddedBufferPresent)
> > -             returnEmbeddedBuffer(data.embeddedBufferId);
> > -
> >       /* Allow a 10% margin on the comparison below. */
> >       Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> >       if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
> >       return statistics;
> >  }
> >
> > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)
> >  {
> >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> >
> > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > index e54bf1ef2c17..30ce6feab0d1 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > @@ -200,15 +200,15 @@ public:
> >       void freeBuffers();
> >       void frameStarted(uint32_t sequence);
> >
> > -     int loadIPA(ipa::RPi::IPAInitResult *result);
> > -     int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
> > +     int loadIPA(ipa::RPi::InitResult *result);
> > +     int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);
> >       int loadPipelineConfiguration();
> >
> >       void enumerateVideoDevices(MediaLink *link);
> >
> > -     void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > -     void runIsp(uint32_t bufferId);
> > -     void embeddedComplete(uint32_t bufferId);
> > +     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);
> >       void setLensControls(const ControlList &controls);
> > @@ -238,7 +238,7 @@ public:
> >       /* The vector below is just for convenience when iterating over all streams. */
> >       std::vector<RPi::Stream *> streams_;
> >       /* Stores the ids of the buffers mapped in the IPA. */
> > -     std::unordered_set<unsigned int> ipaBuffers_;
> > +     std::unordered_set<unsigned int> bufferIds_;
> >       /*
> >        * Stores a cascade of Video Mux or Bridge devices between the sensor and
> >        * Unicam together with media link across the entities.
> > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >
> >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> >
> > -     ipa::RPi::IPAConfigResult result;
> > +     ipa::RPi::ConfigResult result;
> >       ret = data->configureIPA(config, &result);
> >       if (ret)
> >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> >               data->applyScalerCrop(*controls);
> >
> >       /* Start the IPA. */
> > -     ipa::RPi::StartConfig startConfig;
> > +     ipa::RPi::StartResult result;
> >       data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> > -                       &startConfig);
> > +                       &result);
> >
> >       /* Apply any gain/exposure settings that the IPA may have passed back. */
> > -     if (!startConfig.controls.empty())
> > -             data->setSensorControls(startConfig.controls);
> > +     if (!result.controls.empty())
> > +             data->setSensorControls(result.controls);
> >
> >       /* Configure the number of dropped frames required on startup. */
> >       data->dropFrameCount_ = data->config_.disableStartupFrameDrops
> > -                           ? 0 : startConfig.dropFrameCount;
> > +                             ? 0 : result.dropFrameCount;
> >
> >       for (auto const stream : data->streams_)
> >               stream->resetBuffers();
> > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >
> >       data->sensorFormats_ = populateSensorFormats(data->sensor_);
> >
> > -     ipa::RPi::IPAInitResult result;
> > +     ipa::RPi::InitResult result;
> >       if (data->loadIPA(&result)) {
> >               LOG(RPI, Error) << "Failed to load a suitable IPA library";
> >               return -EINVAL;
> > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >  void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)
> >  {
> >       RPiCameraData *data = cameraData(camera);
> > -     std::vector<IPABuffer> ipaBuffers;
> > +     std::vector<IPABuffer> bufferIds;
> >       /*
> >        * Link the FrameBuffers with the id (key value) in the map stored in
> >        * the RPi stream object - along with an identifier mask.
> > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
> >        * handler and the IPA.
> >        */
> >       for (auto const &it : buffers) {
> > -             ipaBuffers.push_back(IPABuffer(mask | it.first,
> > +             bufferIds.push_back(IPABuffer(mask | it.first,
> >                                              it.second->planes()));
> > -             data->ipaBuffers_.insert(mask | it.first);
> > +             data->bufferIds_.insert(mask | it.first);
> >       }
> >
> > -     data->ipa_->mapBuffers(ipaBuffers);
> > +     data->ipa_->mapBuffers(bufferIds);
> >  }
> >
> >  void RPiCameraData::freeBuffers()
> > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()
> >                * Copy the buffer ids from the unordered_set to a vector to
> >                * pass to the IPA.
> >                */
> > -             std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(),
> > -                                                  ipaBuffers_.end());
> > -             ipa_->unmapBuffers(ipaBuffers);
> > -             ipaBuffers_.clear();
> > +             std::vector<unsigned int> bufferIds(bufferIds_.begin(),
> > +                                                 bufferIds_.end());
> > +             ipa_->unmapBuffers(bufferIds);
> > +             bufferIds_.clear();
> >       }
> >
> >       for (auto const stream : streams_)
> > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> >       delayedCtrls_->applyControls(sequence);
> >  }
> >
> > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)
> >  {
> >       ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
> >
> >       if (!ipa_)
> >               return -ENOENT;
> >
> > -     ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> > -     ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> > -     ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > +     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);
> > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> >       }
> >
> >       IPASettings settings(configurationFile, sensor_->model());
> > +     ipa::RPi::InitParams params;
> >
> > -     return ipa_->init(settings, !!sensor_->focusLens(), result);
> > +     params.lensPresent = !!sensor_->focusLens();
> > +     return ipa_->init(settings, params, result);
> >  }
> >
> > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
> > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)
> >  {
> >       std::map<unsigned int, ControlInfoMap> entityControls;
> > -     ipa::RPi::IPAConfig ipaConfig;
> > +     ipa::RPi::ConfigParams params;
> >
> >       /* \todo Move passing of ispControls and lensControls to ipa::init() */
> > -     ipaConfig.sensorControls = sensor_->controls();
> > -     ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();
> > +     params.sensorControls = sensor_->controls();
> > +     params.ispControls = isp_[Isp::Input].dev()->controls();
> >       if (sensor_->focusLens())
> > -             ipaConfig.lensControls = sensor_->focusLens()->controls();
> > +             params.lensControls = sensor_->focusLens()->controls();
> >
> >       /* Always send the user transform to the IPA. */
> > -     ipaConfig.transform = static_cast<unsigned int>(config->transform);
> > +     params.transform = static_cast<unsigned int>(config->transform);
> >
> >       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> >       if (!lsTable_.isValid()) {
> > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> >                * \todo Investigate if mapping the lens shading table buffer
> >                * could be handled with mapBuffers().
> >                */
> > -             ipaConfig.lsTableHandle = lsTable_;
> > +             params.lsTableHandle = lsTable_;
> >       }
> >
> >       /* We store the IPACameraSensorInfo for digital zoom calculations. */
> > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> >       }
> >
> >       /* Ready the IPA - it must know about the sensor resolution. */
> > -     ControlList controls;
> > -     ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);
> > +     ret = ipa_->configure(sensorInfo_, params, result);
> >       if (ret < 0) {
> >               LOG(RPI, Error) << "IPA configuration failed!";
> >               return -EPIPE;
> >       }
> >
> > -     if (!controls.empty())
> > -             setSensorControls(controls);
> > +     if (!result->controls.empty())
> > +             setSensorControls(result->controls);
> >
> >       return 0;
> >  }
> > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)
> >       }
> >  }
> >
> > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
> >  {
> >       if (!isRunning())
> >               return;
> >
> > -     FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);
> > +     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. */
> >       Request *request = requestQueue_.front();
> > -     request->metadata().merge(controls);
> > +     request->metadata().merge(metadata);
> >
> >       /*
> >        * Inform the sensor of the latest colour gains if it has the
> >        * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
> >        */
> > -     const auto &colourGains = controls.get(libcamera::controls::ColourGains);
> > +     const auto &colourGains = metadata.get(libcamera::controls::ColourGains);
> >       if (notifyGainsUnity_ && colourGains) {
> >               /* The control wants linear gains in the order B, Gb, Gr, R. */
> >               ControlList ctrls(sensor_->controls());
> > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> >
> >               sensor_->setControls(&ctrls);
> >       }
> > -
> > -     state_ = State::IpaComplete;
> > -     handleState();
> >  }
> >
> > -void RPiCameraData::runIsp(uint32_t bufferId)
> > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
> >  {
> > +     unsigned int embeddedId = buffers.embedded & RPi::MaskID;
> > +     unsigned int bayer = buffers.bayer & RPi::MaskID;
> > +     FrameBuffer *buffer;
> > +
> >       if (!isRunning())
> >               return;
> >
> > -     FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);
> > -
> > -     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bufferId & RPi::MaskID)
> > +     buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);
> > +     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID)
> >                       << ", timestamp: " << buffer->metadata().timestamp;
> >
> >       isp_[Isp::Input].queueBuffer(buffer);
> >       ispOutputCount_ = 0;
> > -     handleState();
> > -}
> >
> > -void RPiCameraData::embeddedComplete(uint32_t bufferId)
> > -{
> > -     if (!isRunning())
> > -             return;
> > +     if (sensorMetadata_ && embeddedId) {
> > +             buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);
> > +             handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > +     }
> >
> > -     FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);
> > -     handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >       handleState();
> >  }
> >
> > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >        * application until after the IPA signals so.
> >        */
> >       if (stream == &isp_[Isp::Stats]) {
> > -             ipa_->signalStatReady(RPi::MaskStats | static_cast<unsigned int>(index),
> > -                                   requestQueue_.front()->sequence());
> > +             ipa::RPi::ProcessParams params;
> > +             params.buffers.stats = index | RPi::MaskStats;
> > +             params.ipaContext = requestQueue_.front()->sequence();
> > +             ipa_->processStats(params);
> >       } else {
> >               /* Any other ISP output can be handed back to the application now. */
> >               handleStreamBuffer(buffer, stream);
> > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()
> >       request->metadata().clear();
> >       fillRequestMetadata(bayerFrame.controls, request);
> >
> > -     /*
> > -      * Process all the user controls by the IPA. Once this is complete, we
> > -      * queue the ISP output buffer listed in the request to start the HW
> > -      * pipeline.
> > -      */
> > -     ipa_->signalQueueRequest(request->controls());
> > -
> >       /* Set our state to say the pipeline is active. */
> >       state_ = State::Busy;
> >
> > -     unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > +     unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> >
> > -     LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > -                     << " Bayer buffer id: " << bayerId;
> > +     LOG(RPI, Debug) << "Signalling prepareIsp:"
> > +                     << " Bayer buffer id: " << bayer;
> >
> > -     ipa::RPi::ISPConfig ispPrepare;
> > -     ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;
> > -     ispPrepare.controls = std::move(bayerFrame.controls);
> > -     ispPrepare.ipaContext = request->sequence();
> > -     ispPrepare.delayContext = bayerFrame.delayContext;
> > +     ipa::RPi::PrepareParams params;
> > +     params.buffers.bayer = RPi::MaskBayerData | bayer;
> > +     params.sensorControls = std::move(bayerFrame.controls);
> > +     params.requestControls = request->controls();
> > +     params.ipaContext = request->sequence();
> > +     params.delayContext = bayerFrame.delayContext;
> >
> >       if (embeddedBuffer) {
> >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> >
> > -             ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;
> > -             ispPrepare.embeddedBufferPresent = true;
> > -
> > -             LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > +             params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;
> > +             LOG(RPI, Debug) << "Signalling prepareIsp:"
> >                               << " Embedded buffer id: " << embeddedId;
> >       }
> >
> > -     ipa_->signalIspPrepare(ispPrepare);
> > +     ipa_->prepareIsp(params);
> >  }
> >
> >  bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)
> > --
> > 2.34.1
> >

Hi Naush,

Thank you for the patch.

On Thu, Apr 27, 2023 at 10:53:25AM +0100, Naushir Patuck via libcamera-devel wrote:
> On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi wrote:
> > On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > Restructure the IPA mojom interface to be more consistent in the use
> > > of the API. Function parameters are now grouped into *Params structures
> > > and results are now returned in *Results structures.
> > >
> > > The following pipeline -> IPA interfaces have been removed:
> > >
> > > signalQueueRequest(libcamera.ControlList controls);
> > > signalIspPrepare(ISPConfig data);
> > > signalStatReady(uint32 bufferId, uint32 ipaContext);
> > >
> > > and replaced with:
> > >
> > > prepareIsp(PrepareParams params);
> > > processStats(ProcessParams params);
> > >
> > > signalQueueRequest() is now encompassed within signalPrepareIsp().

I think you meant "within prepareIsp()" here.

> > >
> > > The following IPA -> pipeline interfaces have been removed:
> > >
> > > runIsp(uint32 bufferId);
> > > embeddedComplete(uint32 bufferId);
> > > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > >
> > > and replaced with the following async calls:
> > >
> > > prepareIspComplete(BufferIds buffers);
> > > processStatsComplete(BufferIds buffers);
> > > metadataReady(libcamera.ControlList metadata);
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.mojom       | 127 +++++---------
> > >  src/ipa/rpi/vc4/raspberrypi.cpp               | 102 +++++-------
> > >  .../pipeline/rpi/vc4/raspberrypi.cpp          | 155 +++++++++---------
> > >  3 files changed, 165 insertions(+), 219 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > index 80e0126618c8..7d56248f4ab3 100644
> > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -8,7 +8,7 @@ module ipa.RPi;
> > >
> > >  import "include/libcamera/ipa/core.mojom";
> > >
> > > -/* Size of the LS grid allocation. */
> > > +/* Size of the LS grid allocation on VC4. */
> > >  const uint32 MaxLsGridSize = 0x8000;
> > >
> > >  struct SensorConfig {
> > > @@ -19,117 +19,78 @@ struct SensorConfig {
> > >       uint32 sensorMetadata;
> > >  };
> > >
> > > -struct IPAInitResult {
> > > +struct InitParams {
> > > +     bool lensPresent;
> > > +};
> > > +
> > > +struct InitResult {
> > >       SensorConfig sensorConfig;
> > >       libcamera.ControlInfoMap controlInfo;
> > >  };
> > >
> > > -struct ISPConfig {
> > > -     uint32 embeddedBufferId;
> > > -     uint32 bayerBufferId;
> > > -     bool embeddedBufferPresent;
> > > -     libcamera.ControlList controls;
> > > -     uint32 ipaContext;
> > > -     uint32 delayContext;
> > > +struct BufferIds {
> > > +     uint32 bayer;
> > > +     uint32 embedded;
> > > +     uint32 stats;
> > >  };
> > >
> > > -struct IPAConfig {
> > > +struct ConfigParams {
> > >       uint32 transform;
> > > -     libcamera.SharedFD lsTableHandle;
> > >       libcamera.ControlInfoMap sensorControls;
> > >       libcamera.ControlInfoMap ispControls;
> > >       libcamera.ControlInfoMap lensControls;
> > > +        /* VC4 specifc */
> > > +     libcamera.SharedFD lsTableHandle;
> > >  };
> > >
> > > -struct IPAConfigResult {
> > > -       float modeSensitivity;
> > > -       libcamera.ControlInfoMap controlInfo;
> > > +struct ConfigResult {
> > > +     float modeSensitivity;
> > > +     libcamera.ControlInfoMap controlInfo;
> > > +     libcamera.ControlList controls;
> > >  };
> > >
> > > -struct StartConfig {
> > > +struct StartResult {
> > >       libcamera.ControlList controls;
> > >       int32 dropFrameCount;
> > >  };
> > >
> > > +struct PrepareParams {
> > > +     BufferIds buffers;

The only part that bothers me a bit is usage of BufferIds here, as the
stats buffer isn't used.

> > > +     libcamera.ControlList sensorControls;
> > > +     libcamera.ControlList requestControls;
> > > +     uint32 ipaContext;
> > > +     uint32 delayContext;
> > > +};
> > > +
> > > +struct ProcessParams {
> > > +     BufferIds buffers;
> > > +     uint32 ipaContext;
> > > +};
> > > +
> > >  interface IPARPiInterface {
> > > -     init(libcamera.IPASettings settings, bool lensPresent)
> > > -             => (int32 ret, IPAInitResult result);
> > > -     start(libcamera.ControlList controls) => (StartConfig startConfig);
> > > +     init(libcamera.IPASettings settings, InitParams params)
> > > +             => (int32 ret, InitResult result);
> > > +
> > > +     start(libcamera.ControlList controls) => (StartResult result);
> > >       stop();
> > >
> > > -     /**
> > > -      * \fn configure()
> >
> > Is it intentional to drop documentation ? Was it used at all ?
> 
> I don't believe it is at all used.  I looked at ipu3 and rkisp1 and they have
> no doc tags either.

It's not mandatory, but it's nice to have. Could you keep it if that's
not too much trouble ?

> > > -      * \brief Configure the IPA stream and sensor settings
> > > -      * \param[in] sensorInfo Camera sensor information
> > > -      * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > -      * \param[out] controls Controls to apply by the pipeline entity
> > > -      * \param[out] result Other results that the pipeline handler may require
> > > -      *
> > > -      * This function shall be called when the camera is configured to inform
> > > -      * the IPA of the camera's streams and the sensor settings.
> > > -      *
> > > -      * The \a sensorInfo conveys information about the camera sensor settings that
> > > -      * the pipeline handler has selected for the configuration.
> > > -      *
> > > -      * The \a ipaConfig and \a controls parameters carry data passed by the
> > > -      * pipeline handler to the IPA and back.
> > > -      */
> > > -     configure(libcamera.IPACameraSensorInfo sensorInfo,
> > > -               IPAConfig ipaConfig)
> > > -             => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > -
> > > -     /**
> > > -      * \fn mapBuffers()
> > > -      * \brief Map buffers shared between the pipeline handler and the IPA
> > > -      * \param[in] buffers List of buffers to map
> > > -      *
> > > -      * This function informs the IPA module of memory buffers set up by the
> > > -      * pipeline handler that the IPA needs to access. It provides dmabuf
> > > -      * file handles for each buffer, and associates the buffers with unique
> > > -      * numerical IDs.
> > > -      *
> > > -      * IPAs shall map the dmabuf file handles to their address space and
> > > -      * keep a cache of the mappings, indexed by the buffer numerical IDs.
> > > -      * The IDs are used in all other IPA interface functions to refer to
> > > -      * buffers, including the unmapBuffers() function.
> > > -      *
> > > -      * All buffers that the pipeline handler wishes to share with an IPA
> > > -      * shall be mapped with this function. Buffers may be mapped all at once
> > > -      * with a single call, or mapped and unmapped dynamically at runtime,
> > > -      * depending on the IPA protocol. Regardless of the protocol, all
> > > -      * buffers mapped at a given time shall have unique numerical IDs.
> > > -      *
> > > -      * The numerical IDs have no meaning defined by the IPA interface, and
> > > -      * should be treated as opaque handles by IPAs, with the only exception
> > > -      * that ID zero is invalid.
> > > -      *
> > > -      * \sa unmapBuffers()
> > > -      */
> > > -     mapBuffers(array<libcamera.IPABuffer> buffers);
> > > +     configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)
> > > +             => (int32 ret, ConfigResult result);
> > >
> > > -     /**
> > > -      * \fn unmapBuffers()
> > > -      * \brief Unmap buffers shared by the pipeline to the IPA
> > > -      * \param[in] ids List of buffer IDs to unmap
> > > -      *
> > > -      * This function removes mappings set up with mapBuffers(). Numerical
> > > -      * IDs of unmapped buffers may be reused when mapping new buffers.
> > > -      *
> > > -      * \sa mapBuffers()
> > > -      */
> > > +     mapBuffers(array<libcamera.IPABuffer> buffers);
> > >       unmapBuffers(array<uint32> ids);
> > >
> > > -     [async] signalStatReady(uint32 bufferId, uint32 ipaContext);
> > > -     [async] signalQueueRequest(libcamera.ControlList controls);
> > > -     [async] signalIspPrepare(ISPConfig data);
> > > +     [async] prepareIsp(PrepareParams params);
> > > +     [async] processStats(ProcessParams params);
> > >  };
> > >
> > >  interface IPARPiEventInterface {
> > > -     statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > > -     runIsp(uint32 bufferId);
> > > -     embeddedComplete(uint32 bufferId);
> > > +     prepareIspComplete(BufferIds buffers);
> > > +     processStatsComplete(BufferIds buffers);
> > > +     metadataReady(libcamera.ControlList metadata);
> > >       setIspControls(libcamera.ControlList controls);
> > >       setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
> > >       setLensControls(libcamera.ControlList controls);
> > >       setCameraTimeout(uint32 maxFrameLengthMs);
> > >  };
> > > +
> >
> > Additional blank line
> 
> Ack
> 
> > The rest looks good
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp
> > > index 841635ccde2b..d737f1d662a0 100644
> > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> > > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp
> > > @@ -136,30 +136,28 @@ public:
> > >                       munmap(lsTable_, MaxLsGridSize);
> > >       }
> > >
> > > -     int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;
> > > -     void start(const ControlList &controls, StartConfig *startConfig) override;
> > > +     int init(const IPASettings &settings, const InitParams &params, InitResult *result) override;
> > > +     void start(const ControlList &controls, StartResult *result) override;
> > >       void stop() override {}
> > >
> > > -     int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,
> > > -                   ControlList *controls, IPAConfigResult *result) override;
> > > +     int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > > +                   ConfigResult *result) override;
> > >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > -     void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;
> > > -     void signalQueueRequest(const ControlList &controls) override;
> > > -     void signalIspPrepare(const ISPConfig &data) override;
> > > +     void prepareIsp(const PrepareParams &params) override;
> > > +     void processStats(const ProcessParams &params) override;
> > >
> > >  private:
> > >       void setMode(const IPACameraSensorInfo &sensorInfo);
> > >       bool validateSensorControls();
> > >       bool validateIspControls();
> > >       bool validateLensControls();
> > > -     void queueRequest(const ControlList &controls);
> > > -     void returnEmbeddedBuffer(unsigned int bufferId);
> > > -     void prepareISP(const ISPConfig &data);
> > > +     void applyControls(const ControlList &controls);
> > > +     void prepare(const PrepareParams &params);
> > >       void reportMetadata(unsigned int ipaContext);
> > >       void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> > >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> > > -     void processStats(unsigned int bufferId, unsigned int ipaContext);
> > > +     void process(unsigned int bufferId, unsigned int ipaContext);
> > >       void setCameraTimeoutValue();
> > >       void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
> > >       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> > > @@ -229,7 +227,7 @@ private:
> > >       Duration lastTimeout_;
> > >  };
> > >
> > > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> > > +int IPARPi::init(const IPASettings &settings, const InitParams &params, InitResult *result)
> > >  {
> > >       /*
> > >        * Load the "helper" for this sensor. This tells us all the device specific stuff
> > > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> > >               return -EINVAL;
> > >       }
> > >
> > > -     lensPresent_ = lensPresent;
> > > +     lensPresent_ = params.lensPresent;
> > >
> > >       controller_.initialise();
> > >
> > > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> > >       return 0;
> > >  }
> > >
> > > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > +void IPARPi::start(const ControlList &controls, StartResult *result)
> > >  {
> > >       RPiController::Metadata metadata;
> > >
> > > -     ASSERT(startConfig);
> > >       if (!controls.empty()) {
> > >               /* We have been given some controls to action before start. */
> > > -             queueRequest(controls);
> > > +             applyControls(controls);
> > >       }
> > >
> > >       controller_.switchMode(mode_, &metadata);
> > > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > >       if (agcStatus.shutterTime && agcStatus.analogueGain) {
> > >               ControlList ctrls(sensorCtrls_);
> > >               applyAGC(&agcStatus, ctrls);
> > > -             startConfig->controls = std::move(ctrls);
> > > +             result->controls = std::move(ctrls);
> > >               setCameraTimeoutValue();
> > >       }
> > >
> > > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > >               mistrustCount_ = helper_->mistrustFramesModeSwitch();
> > >       }
> > >
> > > -     startConfig->dropFrameCount = dropFrameCount_;
> > > +     result->dropFrameCount = dropFrameCount_;
> > >
> > >       firstStart_ = false;
> > >       lastRunTimestamp_ = 0;
> > > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> > >                            mode_.minFrameDuration, mode_.maxFrameDuration);
> > >  }
> > >
> > > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,
> > > -                   ControlList *controls, IPAConfigResult *result)
> > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > > +                   ConfigResult *result)
> > >  {
> > > -     sensorCtrls_ = ipaConfig.sensorControls;
> > > -     ispCtrls_ = ipaConfig.ispControls;
> > > +     sensorCtrls_ = params.sensorControls;
> > > +     ispCtrls_ = params.ispControls;
> > >
> > >       if (!validateSensorControls()) {
> > >               LOG(IPARPI, Error) << "Sensor control validation failed.";
> > > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > >       }
> > >
> > >       if (lensPresent_) {
> > > -             lensCtrls_ = ipaConfig.lensControls;
> > > +             lensCtrls_ = params.lensControls;
> > >               if (!validateLensControls()) {
> > >                       LOG(IPARPI, Warning) << "Lens validation failed, "
> > >                                            << "no lens control will be available.";
> > > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > >       /* Re-assemble camera mode using the sensor info. */
> > >       setMode(sensorInfo);
> > >
> > > -     mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform);
> > > +     mode_.transform = static_cast<libcamera::Transform>(params.transform);
> > >
> > >       /* Store the lens shading table pointer and handle if available. */
> > > -     if (ipaConfig.lsTableHandle.isValid()) {
> > > +     if (params.lsTableHandle.isValid()) {
> > >               /* Remove any previous table, if there was one. */
> > >               if (lsTable_) {
> > >                       munmap(lsTable_, MaxLsGridSize);
> > > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > >               }
> > >
> > >               /* Map the LS table buffer into user space. */
> > > -             lsTableHandle_ = std::move(ipaConfig.lsTableHandle);
> > > +             lsTableHandle_ = std::move(params.lsTableHandle);
> > >               if (lsTableHandle_.isValid()) {
> > >                       lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,
> > >                                       MAP_SHARED, lsTableHandle_.get(), 0);
> > > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > >               applyAGC(&agcStatus, ctrls);
> > >       }
> > >
> > > -     ASSERT(controls);
> > > -     *controls = std::move(ctrls);
> > > +     result->controls = std::move(ctrls);
> > >
> > >       /*
> > >        * Apply the correct limits to the exposure, gain and frame duration controls
> > > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
> > >       }
> > >  }
> > >
> > > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)
> > > +void IPARPi::processStats(const ProcessParams &params)
> > >  {
> > > -     unsigned int context = ipaContext % rpiMetadata_.size();
> > > +     unsigned int context = params.ipaContext % rpiMetadata_.size();
> > >
> > >       if (++checkCount_ != frameCount_) /* assert here? */
> > >               LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > >       if (processPending_ && frameCount_ > mistrustCount_)
> > > -             processStats(bufferId, context);
> > > +             process(params.buffers.stats, context);
> > >
> > >       reportMetadata(context);
> > > -
> > > -     statsMetadataComplete.emit(bufferId, libcameraMetadata_);
> > > +     processStatsComplete.emit(params.buffers);
> > >  }
> > >
> > > -void IPARPi::signalQueueRequest(const ControlList &controls)
> > > -{
> > > -     queueRequest(controls);
> > > -}
> > >
> > > -void IPARPi::signalIspPrepare(const ISPConfig &data)
> > > +void IPARPi::prepareIsp(const PrepareParams &params)
> > >  {
> > > +     applyControls(params.requestControls);
> > > +
> > >       /*
> > >        * At start-up, or after a mode-switch, we may want to
> > >        * avoid running the control algos for a few frames in case
> > >        * they are "unreliable".
> > >        */
> > > -     prepareISP(data);
> > > +     prepare(params);
> > >       frameCount_++;
> > >
> > >       /* Ready to push the input buffer into the ISP. */
> > > -     runIsp.emit(data.bayerBufferId);
> > > +     prepareIspComplete.emit(params.buffers);
> > >  }
> > >
> > >  void IPARPi::reportMetadata(unsigned int ipaContext)
> > > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
> > >               libcameraMetadata_.set(controls::AfState, s);
> > >               libcameraMetadata_.set(controls::AfPauseState, p);
> > >       }
> > > +
> > > +     metadataReady.emit(libcameraMetadata_);
> > >  }
> > >
> > >  bool IPARPi::validateSensorControls()
> > > @@ -826,7 +821,7 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
> > >       { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
> > >  };
> > >
> > > -void IPARPi::queueRequest(const ControlList &controls)
> > > +void IPARPi::applyControls(const ControlList &controls)
> > >  {
> > >       using RPiController::AfAlgorithm;
> > >
> > > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >       }
> > >  }
> > >
> > > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > > +void IPARPi::prepare(const PrepareParams &params)
> > >  {
> > > -     embeddedComplete.emit(bufferId);
> > > -}
> > > -
> > > -void IPARPi::prepareISP(const ISPConfig &data)
> > > -{
> > > -     int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> > > -     unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();
> > > +     int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);
> > > +     unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
> > >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > >       Span<uint8_t> embeddedBuffer;
> > >
> > >       rpiMetadata.clear();
> > > -     fillDeviceStatus(data.controls, ipaContext);
> > > +     fillDeviceStatus(params.sensorControls, ipaContext);
> > >
> > > -     if (data.embeddedBufferPresent) {
> > > +     if (params.buffers.embedded) {
> > >               /*
> > >                * Pipeline handler has supplied us with an embedded data buffer,
> > >                * we must pass it to the CamHelper for parsing.
> > >                */
> > > -             auto it = buffers_.find(data.embeddedBufferId);
> > > +             auto it = buffers_.find(params.buffers.embedded);
> > >               ASSERT(it != buffers_.end());
> > >               embeddedBuffer = it->second.planes()[0];
> > >       }
> > > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
> > >        * metadata.
> > >        */
> > >       AgcStatus agcStatus;
> > > -     RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];
> > > +     RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
> > >       if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> > >               rpiMetadata.set("agc.delayed_status", agcStatus);
> > >
> > > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)
> > >        */
> > >       helper_->prepare(embeddedBuffer, rpiMetadata);
> > >
> > > -     /* Done with embedded data now, return to pipeline handler asap. */
> > > -     if (data.embeddedBufferPresent)
> > > -             returnEmbeddedBuffer(data.embeddedBufferId);
> > > -
> > >       /* Allow a 10% margin on the comparison below. */
> > >       Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> > >       if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
> > >       return statistics;
> > >  }
> > >
> > > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> > > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)
> > >  {
> > >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > index e54bf1ef2c17..30ce6feab0d1 100644
> > > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > @@ -200,15 +200,15 @@ public:
> > >       void freeBuffers();
> > >       void frameStarted(uint32_t sequence);
> > >
> > > -     int loadIPA(ipa::RPi::IPAInitResult *result);
> > > -     int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
> > > +     int loadIPA(ipa::RPi::InitResult *result);
> > > +     int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);
> > >       int loadPipelineConfiguration();
> > >
> > >       void enumerateVideoDevices(MediaLink *link);
> > >
> > > -     void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > -     void runIsp(uint32_t bufferId);
> > > -     void embeddedComplete(uint32_t bufferId);
> > > +     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);
> > >       void setLensControls(const ControlList &controls);
> > > @@ -238,7 +238,7 @@ public:
> > >       /* The vector below is just for convenience when iterating over all streams. */
> > >       std::vector<RPi::Stream *> streams_;
> > >       /* Stores the ids of the buffers mapped in the IPA. */
> > > -     std::unordered_set<unsigned int> ipaBuffers_;
> > > +     std::unordered_set<unsigned int> bufferIds_;
> > >       /*
> > >        * Stores a cascade of Video Mux or Bridge devices between the sensor and
> > >        * Unicam together with media link across the entities.
> > > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >
> > >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > >
> > > -     ipa::RPi::IPAConfigResult result;
> > > +     ipa::RPi::ConfigResult result;
> > >       ret = data->configureIPA(config, &result);
> > >       if (ret)
> > >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> > > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > >               data->applyScalerCrop(*controls);
> > >
> > >       /* Start the IPA. */
> > > -     ipa::RPi::StartConfig startConfig;
> > > +     ipa::RPi::StartResult result;
> > >       data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> > > -                       &startConfig);
> > > +                       &result);
> > >
> > >       /* Apply any gain/exposure settings that the IPA may have passed back. */
> > > -     if (!startConfig.controls.empty())
> > > -             data->setSensorControls(startConfig.controls);
> > > +     if (!result.controls.empty())
> > > +             data->setSensorControls(result.controls);
> > >
> > >       /* Configure the number of dropped frames required on startup. */
> > >       data->dropFrameCount_ = data->config_.disableStartupFrameDrops
> > > -                           ? 0 : startConfig.dropFrameCount;
> > > +                             ? 0 : result.dropFrameCount;
> > >
> > >       for (auto const stream : data->streams_)
> > >               stream->resetBuffers();
> > > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > >
> > >       data->sensorFormats_ = populateSensorFormats(data->sensor_);
> > >
> > > -     ipa::RPi::IPAInitResult result;
> > > +     ipa::RPi::InitResult result;
> > >       if (data->loadIPA(&result)) {
> > >               LOG(RPI, Error) << "Failed to load a suitable IPA library";
> > >               return -EINVAL;
> > > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > >  void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)
> > >  {
> > >       RPiCameraData *data = cameraData(camera);
> > > -     std::vector<IPABuffer> ipaBuffers;
> > > +     std::vector<IPABuffer> bufferIds;
> > >       /*
> > >        * Link the FrameBuffers with the id (key value) in the map stored in
> > >        * the RPi stream object - along with an identifier mask.
> > > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
> > >        * handler and the IPA.
> > >        */
> > >       for (auto const &it : buffers) {
> > > -             ipaBuffers.push_back(IPABuffer(mask | it.first,
> > > +             bufferIds.push_back(IPABuffer(mask | it.first,
> > >                                              it.second->planes()));
> > > -             data->ipaBuffers_.insert(mask | it.first);
> > > +             data->bufferIds_.insert(mask | it.first);
> > >       }
> > >
> > > -     data->ipa_->mapBuffers(ipaBuffers);
> > > +     data->ipa_->mapBuffers(bufferIds);
> > >  }
> > >
> > >  void RPiCameraData::freeBuffers()
> > > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()
> > >                * Copy the buffer ids from the unordered_set to a vector to
> > >                * pass to the IPA.
> > >                */
> > > -             std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(),
> > > -                                                  ipaBuffers_.end());
> > > -             ipa_->unmapBuffers(ipaBuffers);
> > > -             ipaBuffers_.clear();
> > > +             std::vector<unsigned int> bufferIds(bufferIds_.begin(),
> > > +                                                 bufferIds_.end());
> > > +             ipa_->unmapBuffers(bufferIds);
> > > +             bufferIds_.clear();
> > >       }
> > >
> > >       for (auto const stream : streams_)
> > > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> > >       delayedCtrls_->applyControls(sequence);
> > >  }
> > >
> > > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)
> > >  {
> > >       ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
> > >
> > >       if (!ipa_)
> > >               return -ENOENT;
> > >
> > > -     ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> > > -     ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> > > -     ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > > +     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);
> > > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > >       }
> > >
> > >       IPASettings settings(configurationFile, sensor_->model());
> > > +     ipa::RPi::InitParams params;
> > >
> > > -     return ipa_->init(settings, !!sensor_->focusLens(), result);
> > > +     params.lensPresent = !!sensor_->focusLens();
> > > +     return ipa_->init(settings, params, result);
> > >  }
> > >
> > > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
> > > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)
> > >  {
> > >       std::map<unsigned int, ControlInfoMap> entityControls;
> > > -     ipa::RPi::IPAConfig ipaConfig;
> > > +     ipa::RPi::ConfigParams params;
> > >
> > >       /* \todo Move passing of ispControls and lensControls to ipa::init() */
> > > -     ipaConfig.sensorControls = sensor_->controls();
> > > -     ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();
> > > +     params.sensorControls = sensor_->controls();
> > > +     params.ispControls = isp_[Isp::Input].dev()->controls();
> > >       if (sensor_->focusLens())
> > > -             ipaConfig.lensControls = sensor_->focusLens()->controls();
> > > +             params.lensControls = sensor_->focusLens()->controls();
> > >
> > >       /* Always send the user transform to the IPA. */
> > > -     ipaConfig.transform = static_cast<unsigned int>(config->transform);
> > > +     params.transform = static_cast<unsigned int>(config->transform);
> > >
> > >       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> > >       if (!lsTable_.isValid()) {
> > > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > >                * \todo Investigate if mapping the lens shading table buffer
> > >                * could be handled with mapBuffers().
> > >                */
> > > -             ipaConfig.lsTableHandle = lsTable_;
> > > +             params.lsTableHandle = lsTable_;
> > >       }
> > >
> > >       /* We store the IPACameraSensorInfo for digital zoom calculations. */
> > > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > >       }
> > >
> > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > -     ControlList controls;
> > > -     ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);
> > > +     ret = ipa_->configure(sensorInfo_, params, result);
> > >       if (ret < 0) {
> > >               LOG(RPI, Error) << "IPA configuration failed!";
> > >               return -EPIPE;
> > >       }
> > >
> > > -     if (!controls.empty())
> > > -             setSensorControls(controls);
> > > +     if (!result->controls.empty())
> > > +             setSensorControls(result->controls);
> > >
> > >       return 0;
> > >  }
> > > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)
> > >       }
> > >  }
> > >
> > > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> > > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
> > >  {
> > >       if (!isRunning())
> > >               return;
> > >
> > > -     FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);
> > > +     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. */
> > >       Request *request = requestQueue_.front();
> > > -     request->metadata().merge(controls);
> > > +     request->metadata().merge(metadata);
> > >
> > >       /*
> > >        * Inform the sensor of the latest colour gains if it has the
> > >        * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
> > >        */
> > > -     const auto &colourGains = controls.get(libcamera::controls::ColourGains);
> > > +     const auto &colourGains = metadata.get(libcamera::controls::ColourGains);
> > >       if (notifyGainsUnity_ && colourGains) {
> > >               /* The control wants linear gains in the order B, Gb, Gr, R. */
> > >               ControlList ctrls(sensor_->controls());
> > > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> > >
> > >               sensor_->setControls(&ctrls);
> > >       }
> > > -
> > > -     state_ = State::IpaComplete;
> > > -     handleState();
> > >  }
> > >
> > > -void RPiCameraData::runIsp(uint32_t bufferId)
> > > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
> > >  {
> > > +     unsigned int embeddedId = buffers.embedded & RPi::MaskID;
> > > +     unsigned int bayer = buffers.bayer & RPi::MaskID;
> > > +     FrameBuffer *buffer;
> > > +
> > >       if (!isRunning())
> > >               return;
> > >
> > > -     FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);
> > > -
> > > -     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bufferId & RPi::MaskID)
> > > +     buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);
> > > +     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID)
> > >                       << ", timestamp: " << buffer->metadata().timestamp;
> > >
> > >       isp_[Isp::Input].queueBuffer(buffer);
> > >       ispOutputCount_ = 0;
> > > -     handleState();
> > > -}
> > >
> > > -void RPiCameraData::embeddedComplete(uint32_t bufferId)
> > > -{
> > > -     if (!isRunning())
> > > -             return;
> > > +     if (sensorMetadata_ && embeddedId) {
> > > +             buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);
> > > +             handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > > +     }
> > >
> > > -     FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);
> > > -     handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > >       handleState();
> > >  }
> > >
> > > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > >        * application until after the IPA signals so.
> > >        */
> > >       if (stream == &isp_[Isp::Stats]) {
> > > -             ipa_->signalStatReady(RPi::MaskStats | static_cast<unsigned int>(index),
> > > -                                   requestQueue_.front()->sequence());
> > > +             ipa::RPi::ProcessParams params;
> > > +             params.buffers.stats = index | RPi::MaskStats;
> > > +             params.ipaContext = requestQueue_.front()->sequence();
> > > +             ipa_->processStats(params);
> > >       } else {
> > >               /* Any other ISP output can be handed back to the application now. */
> > >               handleStreamBuffer(buffer, stream);
> > > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()
> > >       request->metadata().clear();
> > >       fillRequestMetadata(bayerFrame.controls, request);
> > >
> > > -     /*
> > > -      * Process all the user controls by the IPA. Once this is complete, we
> > > -      * queue the ISP output buffer listed in the request to start the HW
> > > -      * pipeline.
> > > -      */
> > > -     ipa_->signalQueueRequest(request->controls());
> > > -
> > >       /* Set our state to say the pipeline is active. */
> > >       state_ = State::Busy;
> > >
> > > -     unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > > +     unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > >
> > > -     LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > > -                     << " Bayer buffer id: " << bayerId;
> > > +     LOG(RPI, Debug) << "Signalling prepareIsp:"
> > > +                     << " Bayer buffer id: " << bayer;
> > >
> > > -     ipa::RPi::ISPConfig ispPrepare;
> > > -     ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;
> > > -     ispPrepare.controls = std::move(bayerFrame.controls);
> > > -     ispPrepare.ipaContext = request->sequence();
> > > -     ispPrepare.delayContext = bayerFrame.delayContext;
> > > +     ipa::RPi::PrepareParams params;
> > > +     params.buffers.bayer = RPi::MaskBayerData | bayer;
> > > +     params.sensorControls = std::move(bayerFrame.controls);
> > > +     params.requestControls = request->controls();
> > > +     params.ipaContext = request->sequence();
> > > +     params.delayContext = bayerFrame.delayContext;
> > >
> > >       if (embeddedBuffer) {
> > >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> > >
> > > -             ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;
> > > -             ispPrepare.embeddedBufferPresent = true;
> > > -
> > > -             LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > > +             params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;
> > > +             LOG(RPI, Debug) << "Signalling prepareIsp:"
> > >                               << " Embedded buffer id: " << embeddedId;
> > >       }
> > >
> > > -     ipa_->signalIspPrepare(ispPrepare);
> > > +     ipa_->prepareIsp(params);
> > >  }
> > >
> > >  bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)

Hi Laurent,

On Thu, 27 Apr 2023 at 18:15, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Apr 27, 2023 at 10:53:25AM +0100, Naushir Patuck via libcamera-devel wrote:
> > On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi wrote:
> > > On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > > Restructure the IPA mojom interface to be more consistent in the use
> > > > of the API. Function parameters are now grouped into *Params structures
> > > > and results are now returned in *Results structures.
> > > >
> > > > The following pipeline -> IPA interfaces have been removed:
> > > >
> > > > signalQueueRequest(libcamera.ControlList controls);
> > > > signalIspPrepare(ISPConfig data);
> > > > signalStatReady(uint32 bufferId, uint32 ipaContext);
> > > >
> > > > and replaced with:
> > > >
> > > > prepareIsp(PrepareParams params);
> > > > processStats(ProcessParams params);
> > > >
> > > > signalQueueRequest() is now encompassed within signalPrepareIsp().
>
> I think you meant "within prepareIsp()" here.
>
> > > >
> > > > The following IPA -> pipeline interfaces have been removed:
> > > >
> > > > runIsp(uint32 bufferId);
> > > > embeddedComplete(uint32 bufferId);
> > > > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > > >
> > > > and replaced with the following async calls:
> > > >
> > > > prepareIspComplete(BufferIds buffers);
> > > > processStatsComplete(BufferIds buffers);
> > > > metadataReady(libcamera.ControlList metadata);
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > ---
> > > >  include/libcamera/ipa/raspberrypi.mojom       | 127 +++++---------
> > > >  src/ipa/rpi/vc4/raspberrypi.cpp               | 102 +++++-------
> > > >  .../pipeline/rpi/vc4/raspberrypi.cpp          | 155 +++++++++---------
> > > >  3 files changed, 165 insertions(+), 219 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > index 80e0126618c8..7d56248f4ab3 100644
> > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > @@ -8,7 +8,7 @@ module ipa.RPi;
> > > >
> > > >  import "include/libcamera/ipa/core.mojom";
> > > >
> > > > -/* Size of the LS grid allocation. */
> > > > +/* Size of the LS grid allocation on VC4. */
> > > >  const uint32 MaxLsGridSize = 0x8000;
> > > >
> > > >  struct SensorConfig {
> > > > @@ -19,117 +19,78 @@ struct SensorConfig {
> > > >       uint32 sensorMetadata;
> > > >  };
> > > >
> > > > -struct IPAInitResult {
> > > > +struct InitParams {
> > > > +     bool lensPresent;
> > > > +};
> > > > +
> > > > +struct InitResult {
> > > >       SensorConfig sensorConfig;
> > > >       libcamera.ControlInfoMap controlInfo;
> > > >  };
> > > >
> > > > -struct ISPConfig {
> > > > -     uint32 embeddedBufferId;
> > > > -     uint32 bayerBufferId;
> > > > -     bool embeddedBufferPresent;
> > > > -     libcamera.ControlList controls;
> > > > -     uint32 ipaContext;
> > > > -     uint32 delayContext;
> > > > +struct BufferIds {
> > > > +     uint32 bayer;
> > > > +     uint32 embedded;
> > > > +     uint32 stats;
> > > >  };
> > > >
> > > > -struct IPAConfig {
> > > > +struct ConfigParams {
> > > >       uint32 transform;
> > > > -     libcamera.SharedFD lsTableHandle;
> > > >       libcamera.ControlInfoMap sensorControls;
> > > >       libcamera.ControlInfoMap ispControls;
> > > >       libcamera.ControlInfoMap lensControls;
> > > > +        /* VC4 specifc */
> > > > +     libcamera.SharedFD lsTableHandle;
> > > >  };
> > > >
> > > > -struct IPAConfigResult {
> > > > -       float modeSensitivity;
> > > > -       libcamera.ControlInfoMap controlInfo;
> > > > +struct ConfigResult {
> > > > +     float modeSensitivity;
> > > > +     libcamera.ControlInfoMap controlInfo;
> > > > +     libcamera.ControlList controls;
> > > >  };
> > > >
> > > > -struct StartConfig {
> > > > +struct StartResult {
> > > >       libcamera.ControlList controls;
> > > >       int32 dropFrameCount;
> > > >  };
> > > >
> > > > +struct PrepareParams {
> > > > +     BufferIds buffers;
>
> The only part that bothers me a bit is usage of BufferIds here, as the
> stats buffer isn't used.

Adding the stats buffer to BufferIds allows me to generalise the structure and
use it throughout the interface.

It is also sort-of ' used right now, in that we see the absence of the stats
buffer to "know" that the stats are not in-line with prepareISP.  However, this
is not intended to stay that way, we will have a separate parameter to signal
this if needed.

Regards,
Naush

>
> > > > +     libcamera.ControlList sensorControls;
> > > > +     libcamera.ControlList requestControls;
> > > > +     uint32 ipaContext;
> > > > +     uint32 delayContext;
> > > > +};
> > > > +
> > > > +struct ProcessParams {
> > > > +     BufferIds buffers;
> > > > +     uint32 ipaContext;
> > > > +};
> > > > +
> > > >  interface IPARPiInterface {
> > > > -     init(libcamera.IPASettings settings, bool lensPresent)
> > > > -             => (int32 ret, IPAInitResult result);
> > > > -     start(libcamera.ControlList controls) => (StartConfig startConfig);
> > > > +     init(libcamera.IPASettings settings, InitParams params)
> > > > +             => (int32 ret, InitResult result);
> > > > +
> > > > +     start(libcamera.ControlList controls) => (StartResult result);
> > > >       stop();
> > > >
> > > > -     /**
> > > > -      * \fn configure()
> > >
> > > Is it intentional to drop documentation ? Was it used at all ?
> >
> > I don't believe it is at all used.  I looked at ipu3 and rkisp1 and they have
> > no doc tags either.
>
> It's not mandatory, but it's nice to have. Could you keep it if that's
> not too much trouble ?

Ack

>
> > > > -      * \brief Configure the IPA stream and sensor settings
> > > > -      * \param[in] sensorInfo Camera sensor information
> > > > -      * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > -      * \param[out] controls Controls to apply by the pipeline entity
> > > > -      * \param[out] result Other results that the pipeline handler may require
> > > > -      *
> > > > -      * This function shall be called when the camera is configured to inform
> > > > -      * the IPA of the camera's streams and the sensor settings.
> > > > -      *
> > > > -      * The \a sensorInfo conveys information about the camera sensor settings that
> > > > -      * the pipeline handler has selected for the configuration.
> > > > -      *
> > > > -      * The \a ipaConfig and \a controls parameters carry data passed by the
> > > > -      * pipeline handler to the IPA and back.
> > > > -      */
> > > > -     configure(libcamera.IPACameraSensorInfo sensorInfo,
> > > > -               IPAConfig ipaConfig)
> > > > -             => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > > -
> > > > -     /**
> > > > -      * \fn mapBuffers()
> > > > -      * \brief Map buffers shared between the pipeline handler and the IPA
> > > > -      * \param[in] buffers List of buffers to map
> > > > -      *
> > > > -      * This function informs the IPA module of memory buffers set up by the
> > > > -      * pipeline handler that the IPA needs to access. It provides dmabuf
> > > > -      * file handles for each buffer, and associates the buffers with unique
> > > > -      * numerical IDs.
> > > > -      *
> > > > -      * IPAs shall map the dmabuf file handles to their address space and
> > > > -      * keep a cache of the mappings, indexed by the buffer numerical IDs.
> > > > -      * The IDs are used in all other IPA interface functions to refer to
> > > > -      * buffers, including the unmapBuffers() function.
> > > > -      *
> > > > -      * All buffers that the pipeline handler wishes to share with an IPA
> > > > -      * shall be mapped with this function. Buffers may be mapped all at once
> > > > -      * with a single call, or mapped and unmapped dynamically at runtime,
> > > > -      * depending on the IPA protocol. Regardless of the protocol, all
> > > > -      * buffers mapped at a given time shall have unique numerical IDs.
> > > > -      *
> > > > -      * The numerical IDs have no meaning defined by the IPA interface, and
> > > > -      * should be treated as opaque handles by IPAs, with the only exception
> > > > -      * that ID zero is invalid.
> > > > -      *
> > > > -      * \sa unmapBuffers()
> > > > -      */
> > > > -     mapBuffers(array<libcamera.IPABuffer> buffers);
> > > > +     configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)
> > > > +             => (int32 ret, ConfigResult result);
> > > >
> > > > -     /**
> > > > -      * \fn unmapBuffers()
> > > > -      * \brief Unmap buffers shared by the pipeline to the IPA
> > > > -      * \param[in] ids List of buffer IDs to unmap
> > > > -      *
> > > > -      * This function removes mappings set up with mapBuffers(). Numerical
> > > > -      * IDs of unmapped buffers may be reused when mapping new buffers.
> > > > -      *
> > > > -      * \sa mapBuffers()
> > > > -      */
> > > > +     mapBuffers(array<libcamera.IPABuffer> buffers);
> > > >       unmapBuffers(array<uint32> ids);
> > > >
> > > > -     [async] signalStatReady(uint32 bufferId, uint32 ipaContext);
> > > > -     [async] signalQueueRequest(libcamera.ControlList controls);
> > > > -     [async] signalIspPrepare(ISPConfig data);
> > > > +     [async] prepareIsp(PrepareParams params);
> > > > +     [async] processStats(ProcessParams params);
> > > >  };
> > > >
> > > >  interface IPARPiEventInterface {
> > > > -     statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > > > -     runIsp(uint32 bufferId);
> > > > -     embeddedComplete(uint32 bufferId);
> > > > +     prepareIspComplete(BufferIds buffers);
> > > > +     processStatsComplete(BufferIds buffers);
> > > > +     metadataReady(libcamera.ControlList metadata);
> > > >       setIspControls(libcamera.ControlList controls);
> > > >       setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
> > > >       setLensControls(libcamera.ControlList controls);
> > > >       setCameraTimeout(uint32 maxFrameLengthMs);
> > > >  };
> > > > +
> > >
> > > Additional blank line
> >
> > Ack
> >
> > > The rest looks good
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > >
> > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp
> > > > index 841635ccde2b..d737f1d662a0 100644
> > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> > > > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp
> > > > @@ -136,30 +136,28 @@ public:
> > > >                       munmap(lsTable_, MaxLsGridSize);
> > > >       }
> > > >
> > > > -     int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;
> > > > -     void start(const ControlList &controls, StartConfig *startConfig) override;
> > > > +     int init(const IPASettings &settings, const InitParams &params, InitResult *result) override;
> > > > +     void start(const ControlList &controls, StartResult *result) override;
> > > >       void stop() override {}
> > > >
> > > > -     int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,
> > > > -                   ControlList *controls, IPAConfigResult *result) override;
> > > > +     int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > > > +                   ConfigResult *result) override;
> > > >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > -     void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;
> > > > -     void signalQueueRequest(const ControlList &controls) override;
> > > > -     void signalIspPrepare(const ISPConfig &data) override;
> > > > +     void prepareIsp(const PrepareParams &params) override;
> > > > +     void processStats(const ProcessParams &params) override;
> > > >
> > > >  private:
> > > >       void setMode(const IPACameraSensorInfo &sensorInfo);
> > > >       bool validateSensorControls();
> > > >       bool validateIspControls();
> > > >       bool validateLensControls();
> > > > -     void queueRequest(const ControlList &controls);
> > > > -     void returnEmbeddedBuffer(unsigned int bufferId);
> > > > -     void prepareISP(const ISPConfig &data);
> > > > +     void applyControls(const ControlList &controls);
> > > > +     void prepare(const PrepareParams &params);
> > > >       void reportMetadata(unsigned int ipaContext);
> > > >       void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> > > >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> > > > -     void processStats(unsigned int bufferId, unsigned int ipaContext);
> > > > +     void process(unsigned int bufferId, unsigned int ipaContext);
> > > >       void setCameraTimeoutValue();
> > > >       void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
> > > >       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> > > > @@ -229,7 +227,7 @@ private:
> > > >       Duration lastTimeout_;
> > > >  };
> > > >
> > > > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> > > > +int IPARPi::init(const IPASettings &settings, const InitParams &params, InitResult *result)
> > > >  {
> > > >       /*
> > > >        * Load the "helper" for this sensor. This tells us all the device specific stuff
> > > > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> > > >               return -EINVAL;
> > > >       }
> > > >
> > > > -     lensPresent_ = lensPresent;
> > > > +     lensPresent_ = params.lensPresent;
> > > >
> > > >       controller_.initialise();
> > > >
> > > > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> > > >       return 0;
> > > >  }
> > > >
> > > > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > > +void IPARPi::start(const ControlList &controls, StartResult *result)
> > > >  {
> > > >       RPiController::Metadata metadata;
> > > >
> > > > -     ASSERT(startConfig);
> > > >       if (!controls.empty()) {
> > > >               /* We have been given some controls to action before start. */
> > > > -             queueRequest(controls);
> > > > +             applyControls(controls);
> > > >       }
> > > >
> > > >       controller_.switchMode(mode_, &metadata);
> > > > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > >       if (agcStatus.shutterTime && agcStatus.analogueGain) {
> > > >               ControlList ctrls(sensorCtrls_);
> > > >               applyAGC(&agcStatus, ctrls);
> > > > -             startConfig->controls = std::move(ctrls);
> > > > +             result->controls = std::move(ctrls);
> > > >               setCameraTimeoutValue();
> > > >       }
> > > >
> > > > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > >               mistrustCount_ = helper_->mistrustFramesModeSwitch();
> > > >       }
> > > >
> > > > -     startConfig->dropFrameCount = dropFrameCount_;
> > > > +     result->dropFrameCount = dropFrameCount_;
> > > >
> > > >       firstStart_ = false;
> > > >       lastRunTimestamp_ = 0;
> > > > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> > > >                            mode_.minFrameDuration, mode_.maxFrameDuration);
> > > >  }
> > > >
> > > > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,
> > > > -                   ControlList *controls, IPAConfigResult *result)
> > > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > > > +                   ConfigResult *result)
> > > >  {
> > > > -     sensorCtrls_ = ipaConfig.sensorControls;
> > > > -     ispCtrls_ = ipaConfig.ispControls;
> > > > +     sensorCtrls_ = params.sensorControls;
> > > > +     ispCtrls_ = params.ispControls;
> > > >
> > > >       if (!validateSensorControls()) {
> > > >               LOG(IPARPI, Error) << "Sensor control validation failed.";
> > > > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > >       }
> > > >
> > > >       if (lensPresent_) {
> > > > -             lensCtrls_ = ipaConfig.lensControls;
> > > > +             lensCtrls_ = params.lensControls;
> > > >               if (!validateLensControls()) {
> > > >                       LOG(IPARPI, Warning) << "Lens validation failed, "
> > > >                                            << "no lens control will be available.";
> > > > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > >       /* Re-assemble camera mode using the sensor info. */
> > > >       setMode(sensorInfo);
> > > >
> > > > -     mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform);
> > > > +     mode_.transform = static_cast<libcamera::Transform>(params.transform);
> > > >
> > > >       /* Store the lens shading table pointer and handle if available. */
> > > > -     if (ipaConfig.lsTableHandle.isValid()) {
> > > > +     if (params.lsTableHandle.isValid()) {
> > > >               /* Remove any previous table, if there was one. */
> > > >               if (lsTable_) {
> > > >                       munmap(lsTable_, MaxLsGridSize);
> > > > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > >               }
> > > >
> > > >               /* Map the LS table buffer into user space. */
> > > > -             lsTableHandle_ = std::move(ipaConfig.lsTableHandle);
> > > > +             lsTableHandle_ = std::move(params.lsTableHandle);
> > > >               if (lsTableHandle_.isValid()) {
> > > >                       lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,
> > > >                                       MAP_SHARED, lsTableHandle_.get(), 0);
> > > > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > >               applyAGC(&agcStatus, ctrls);
> > > >       }
> > > >
> > > > -     ASSERT(controls);
> > > > -     *controls = std::move(ctrls);
> > > > +     result->controls = std::move(ctrls);
> > > >
> > > >       /*
> > > >        * Apply the correct limits to the exposure, gain and frame duration controls
> > > > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
> > > >       }
> > > >  }
> > > >
> > > > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)
> > > > +void IPARPi::processStats(const ProcessParams &params)
> > > >  {
> > > > -     unsigned int context = ipaContext % rpiMetadata_.size();
> > > > +     unsigned int context = params.ipaContext % rpiMetadata_.size();
> > > >
> > > >       if (++checkCount_ != frameCount_) /* assert here? */
> > > >               LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > > >       if (processPending_ && frameCount_ > mistrustCount_)
> > > > -             processStats(bufferId, context);
> > > > +             process(params.buffers.stats, context);
> > > >
> > > >       reportMetadata(context);
> > > > -
> > > > -     statsMetadataComplete.emit(bufferId, libcameraMetadata_);
> > > > +     processStatsComplete.emit(params.buffers);
> > > >  }
> > > >
> > > > -void IPARPi::signalQueueRequest(const ControlList &controls)
> > > > -{
> > > > -     queueRequest(controls);
> > > > -}
> > > >
> > > > -void IPARPi::signalIspPrepare(const ISPConfig &data)
> > > > +void IPARPi::prepareIsp(const PrepareParams &params)
> > > >  {
> > > > +     applyControls(params.requestControls);
> > > > +
> > > >       /*
> > > >        * At start-up, or after a mode-switch, we may want to
> > > >        * avoid running the control algos for a few frames in case
> > > >        * they are "unreliable".
> > > >        */
> > > > -     prepareISP(data);
> > > > +     prepare(params);
> > > >       frameCount_++;
> > > >
> > > >       /* Ready to push the input buffer into the ISP. */
> > > > -     runIsp.emit(data.bayerBufferId);
> > > > +     prepareIspComplete.emit(params.buffers);
> > > >  }
> > > >
> > > >  void IPARPi::reportMetadata(unsigned int ipaContext)
> > > > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
> > > >               libcameraMetadata_.set(controls::AfState, s);
> > > >               libcameraMetadata_.set(controls::AfPauseState, p);
> > > >       }
> > > > +
> > > > +     metadataReady.emit(libcameraMetadata_);
> > > >  }
> > > >
> > > >  bool IPARPi::validateSensorControls()
> > > > @@ -826,7 +821,7 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
> > > >       { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
> > > >  };
> > > >
> > > > -void IPARPi::queueRequest(const ControlList &controls)
> > > > +void IPARPi::applyControls(const ControlList &controls)
> > > >  {
> > > >       using RPiController::AfAlgorithm;
> > > >
> > > > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > >       }
> > > >  }
> > > >
> > > > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > > > +void IPARPi::prepare(const PrepareParams &params)
> > > >  {
> > > > -     embeddedComplete.emit(bufferId);
> > > > -}
> > > > -
> > > > -void IPARPi::prepareISP(const ISPConfig &data)
> > > > -{
> > > > -     int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> > > > -     unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();
> > > > +     int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);
> > > > +     unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
> > > >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > > >       Span<uint8_t> embeddedBuffer;
> > > >
> > > >       rpiMetadata.clear();
> > > > -     fillDeviceStatus(data.controls, ipaContext);
> > > > +     fillDeviceStatus(params.sensorControls, ipaContext);
> > > >
> > > > -     if (data.embeddedBufferPresent) {
> > > > +     if (params.buffers.embedded) {
> > > >               /*
> > > >                * Pipeline handler has supplied us with an embedded data buffer,
> > > >                * we must pass it to the CamHelper for parsing.
> > > >                */
> > > > -             auto it = buffers_.find(data.embeddedBufferId);
> > > > +             auto it = buffers_.find(params.buffers.embedded);
> > > >               ASSERT(it != buffers_.end());
> > > >               embeddedBuffer = it->second.planes()[0];
> > > >       }
> > > > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
> > > >        * metadata.
> > > >        */
> > > >       AgcStatus agcStatus;
> > > > -     RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];
> > > > +     RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
> > > >       if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> > > >               rpiMetadata.set("agc.delayed_status", agcStatus);
> > > >
> > > > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)
> > > >        */
> > > >       helper_->prepare(embeddedBuffer, rpiMetadata);
> > > >
> > > > -     /* Done with embedded data now, return to pipeline handler asap. */
> > > > -     if (data.embeddedBufferPresent)
> > > > -             returnEmbeddedBuffer(data.embeddedBufferId);
> > > > -
> > > >       /* Allow a 10% margin on the comparison below. */
> > > >       Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> > > >       if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > > > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
> > > >       return statistics;
> > > >  }
> > > >
> > > > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> > > > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)
> > > >  {
> > > >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > > index e54bf1ef2c17..30ce6feab0d1 100644
> > > > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > > @@ -200,15 +200,15 @@ public:
> > > >       void freeBuffers();
> > > >       void frameStarted(uint32_t sequence);
> > > >
> > > > -     int loadIPA(ipa::RPi::IPAInitResult *result);
> > > > -     int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
> > > > +     int loadIPA(ipa::RPi::InitResult *result);
> > > > +     int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);
> > > >       int loadPipelineConfiguration();
> > > >
> > > >       void enumerateVideoDevices(MediaLink *link);
> > > >
> > > > -     void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > > -     void runIsp(uint32_t bufferId);
> > > > -     void embeddedComplete(uint32_t bufferId);
> > > > +     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);
> > > >       void setLensControls(const ControlList &controls);
> > > > @@ -238,7 +238,7 @@ public:
> > > >       /* The vector below is just for convenience when iterating over all streams. */
> > > >       std::vector<RPi::Stream *> streams_;
> > > >       /* Stores the ids of the buffers mapped in the IPA. */
> > > > -     std::unordered_set<unsigned int> ipaBuffers_;
> > > > +     std::unordered_set<unsigned int> bufferIds_;
> > > >       /*
> > > >        * Stores a cascade of Video Mux or Bridge devices between the sensor and
> > > >        * Unicam together with media link across the entities.
> > > > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > > >
> > > >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > > >
> > > > -     ipa::RPi::IPAConfigResult result;
> > > > +     ipa::RPi::ConfigResult result;
> > > >       ret = data->configureIPA(config, &result);
> > > >       if (ret)
> > > >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> > > > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > > >               data->applyScalerCrop(*controls);
> > > >
> > > >       /* Start the IPA. */
> > > > -     ipa::RPi::StartConfig startConfig;
> > > > +     ipa::RPi::StartResult result;
> > > >       data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> > > > -                       &startConfig);
> > > > +                       &result);
> > > >
> > > >       /* Apply any gain/exposure settings that the IPA may have passed back. */
> > > > -     if (!startConfig.controls.empty())
> > > > -             data->setSensorControls(startConfig.controls);
> > > > +     if (!result.controls.empty())
> > > > +             data->setSensorControls(result.controls);
> > > >
> > > >       /* Configure the number of dropped frames required on startup. */
> > > >       data->dropFrameCount_ = data->config_.disableStartupFrameDrops
> > > > -                           ? 0 : startConfig.dropFrameCount;
> > > > +                             ? 0 : result.dropFrameCount;
> > > >
> > > >       for (auto const stream : data->streams_)
> > > >               stream->resetBuffers();
> > > > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > > >
> > > >       data->sensorFormats_ = populateSensorFormats(data->sensor_);
> > > >
> > > > -     ipa::RPi::IPAInitResult result;
> > > > +     ipa::RPi::InitResult result;
> > > >       if (data->loadIPA(&result)) {
> > > >               LOG(RPI, Error) << "Failed to load a suitable IPA library";
> > > >               return -EINVAL;
> > > > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > >  void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)
> > > >  {
> > > >       RPiCameraData *data = cameraData(camera);
> > > > -     std::vector<IPABuffer> ipaBuffers;
> > > > +     std::vector<IPABuffer> bufferIds;
> > > >       /*
> > > >        * Link the FrameBuffers with the id (key value) in the map stored in
> > > >        * the RPi stream object - along with an identifier mask.
> > > > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
> > > >        * handler and the IPA.
> > > >        */
> > > >       for (auto const &it : buffers) {
> > > > -             ipaBuffers.push_back(IPABuffer(mask | it.first,
> > > > +             bufferIds.push_back(IPABuffer(mask | it.first,
> > > >                                              it.second->planes()));
> > > > -             data->ipaBuffers_.insert(mask | it.first);
> > > > +             data->bufferIds_.insert(mask | it.first);
> > > >       }
> > > >
> > > > -     data->ipa_->mapBuffers(ipaBuffers);
> > > > +     data->ipa_->mapBuffers(bufferIds);
> > > >  }
> > > >
> > > >  void RPiCameraData::freeBuffers()
> > > > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()
> > > >                * Copy the buffer ids from the unordered_set to a vector to
> > > >                * pass to the IPA.
> > > >                */
> > > > -             std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(),
> > > > -                                                  ipaBuffers_.end());
> > > > -             ipa_->unmapBuffers(ipaBuffers);
> > > > -             ipaBuffers_.clear();
> > > > +             std::vector<unsigned int> bufferIds(bufferIds_.begin(),
> > > > +                                                 bufferIds_.end());
> > > > +             ipa_->unmapBuffers(bufferIds);
> > > > +             bufferIds_.clear();
> > > >       }
> > > >
> > > >       for (auto const stream : streams_)
> > > > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> > > >       delayedCtrls_->applyControls(sequence);
> > > >  }
> > > >
> > > > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > > > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)
> > > >  {
> > > >       ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
> > > >
> > > >       if (!ipa_)
> > > >               return -ENOENT;
> > > >
> > > > -     ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> > > > -     ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> > > > -     ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > > > +     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);
> > > > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > > >       }
> > > >
> > > >       IPASettings settings(configurationFile, sensor_->model());
> > > > +     ipa::RPi::InitParams params;
> > > >
> > > > -     return ipa_->init(settings, !!sensor_->focusLens(), result);
> > > > +     params.lensPresent = !!sensor_->focusLens();
> > > > +     return ipa_->init(settings, params, result);
> > > >  }
> > > >
> > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
> > > > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)
> > > >  {
> > > >       std::map<unsigned int, ControlInfoMap> entityControls;
> > > > -     ipa::RPi::IPAConfig ipaConfig;
> > > > +     ipa::RPi::ConfigParams params;
> > > >
> > > >       /* \todo Move passing of ispControls and lensControls to ipa::init() */
> > > > -     ipaConfig.sensorControls = sensor_->controls();
> > > > -     ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();
> > > > +     params.sensorControls = sensor_->controls();
> > > > +     params.ispControls = isp_[Isp::Input].dev()->controls();
> > > >       if (sensor_->focusLens())
> > > > -             ipaConfig.lensControls = sensor_->focusLens()->controls();
> > > > +             params.lensControls = sensor_->focusLens()->controls();
> > > >
> > > >       /* Always send the user transform to the IPA. */
> > > > -     ipaConfig.transform = static_cast<unsigned int>(config->transform);
> > > > +     params.transform = static_cast<unsigned int>(config->transform);
> > > >
> > > >       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> > > >       if (!lsTable_.isValid()) {
> > > > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > > >                * \todo Investigate if mapping the lens shading table buffer
> > > >                * could be handled with mapBuffers().
> > > >                */
> > > > -             ipaConfig.lsTableHandle = lsTable_;
> > > > +             params.lsTableHandle = lsTable_;
> > > >       }
> > > >
> > > >       /* We store the IPACameraSensorInfo for digital zoom calculations. */
> > > > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > > >       }
> > > >
> > > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > > -     ControlList controls;
> > > > -     ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);
> > > > +     ret = ipa_->configure(sensorInfo_, params, result);
> > > >       if (ret < 0) {
> > > >               LOG(RPI, Error) << "IPA configuration failed!";
> > > >               return -EPIPE;
> > > >       }
> > > >
> > > > -     if (!controls.empty())
> > > > -             setSensorControls(controls);
> > > > +     if (!result->controls.empty())
> > > > +             setSensorControls(result->controls);
> > > >
> > > >       return 0;
> > > >  }
> > > > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)
> > > >       }
> > > >  }
> > > >
> > > > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> > > > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
> > > >  {
> > > >       if (!isRunning())
> > > >               return;
> > > >
> > > > -     FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);
> > > > +     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. */
> > > >       Request *request = requestQueue_.front();
> > > > -     request->metadata().merge(controls);
> > > > +     request->metadata().merge(metadata);
> > > >
> > > >       /*
> > > >        * Inform the sensor of the latest colour gains if it has the
> > > >        * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
> > > >        */
> > > > -     const auto &colourGains = controls.get(libcamera::controls::ColourGains);
> > > > +     const auto &colourGains = metadata.get(libcamera::controls::ColourGains);
> > > >       if (notifyGainsUnity_ && colourGains) {
> > > >               /* The control wants linear gains in the order B, Gb, Gr, R. */
> > > >               ControlList ctrls(sensor_->controls());
> > > > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> > > >
> > > >               sensor_->setControls(&ctrls);
> > > >       }
> > > > -
> > > > -     state_ = State::IpaComplete;
> > > > -     handleState();
> > > >  }
> > > >
> > > > -void RPiCameraData::runIsp(uint32_t bufferId)
> > > > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
> > > >  {
> > > > +     unsigned int embeddedId = buffers.embedded & RPi::MaskID;
> > > > +     unsigned int bayer = buffers.bayer & RPi::MaskID;
> > > > +     FrameBuffer *buffer;
> > > > +
> > > >       if (!isRunning())
> > > >               return;
> > > >
> > > > -     FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);
> > > > -
> > > > -     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bufferId & RPi::MaskID)
> > > > +     buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);
> > > > +     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID)
> > > >                       << ", timestamp: " << buffer->metadata().timestamp;
> > > >
> > > >       isp_[Isp::Input].queueBuffer(buffer);
> > > >       ispOutputCount_ = 0;
> > > > -     handleState();
> > > > -}
> > > >
> > > > -void RPiCameraData::embeddedComplete(uint32_t bufferId)
> > > > -{
> > > > -     if (!isRunning())
> > > > -             return;
> > > > +     if (sensorMetadata_ && embeddedId) {
> > > > +             buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);
> > > > +             handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > > > +     }
> > > >
> > > > -     FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);
> > > > -     handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > > >       handleState();
> > > >  }
> > > >
> > > > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > > >        * application until after the IPA signals so.
> > > >        */
> > > >       if (stream == &isp_[Isp::Stats]) {
> > > > -             ipa_->signalStatReady(RPi::MaskStats | static_cast<unsigned int>(index),
> > > > -                                   requestQueue_.front()->sequence());
> > > > +             ipa::RPi::ProcessParams params;
> > > > +             params.buffers.stats = index | RPi::MaskStats;
> > > > +             params.ipaContext = requestQueue_.front()->sequence();
> > > > +             ipa_->processStats(params);
> > > >       } else {
> > > >               /* Any other ISP output can be handed back to the application now. */
> > > >               handleStreamBuffer(buffer, stream);
> > > > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()
> > > >       request->metadata().clear();
> > > >       fillRequestMetadata(bayerFrame.controls, request);
> > > >
> > > > -     /*
> > > > -      * Process all the user controls by the IPA. Once this is complete, we
> > > > -      * queue the ISP output buffer listed in the request to start the HW
> > > > -      * pipeline.
> > > > -      */
> > > > -     ipa_->signalQueueRequest(request->controls());
> > > > -
> > > >       /* Set our state to say the pipeline is active. */
> > > >       state_ = State::Busy;
> > > >
> > > > -     unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > > > +     unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > > >
> > > > -     LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > > > -                     << " Bayer buffer id: " << bayerId;
> > > > +     LOG(RPI, Debug) << "Signalling prepareIsp:"
> > > > +                     << " Bayer buffer id: " << bayer;
> > > >
> > > > -     ipa::RPi::ISPConfig ispPrepare;
> > > > -     ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;
> > > > -     ispPrepare.controls = std::move(bayerFrame.controls);
> > > > -     ispPrepare.ipaContext = request->sequence();
> > > > -     ispPrepare.delayContext = bayerFrame.delayContext;
> > > > +     ipa::RPi::PrepareParams params;
> > > > +     params.buffers.bayer = RPi::MaskBayerData | bayer;
> > > > +     params.sensorControls = std::move(bayerFrame.controls);
> > > > +     params.requestControls = request->controls();
> > > > +     params.ipaContext = request->sequence();
> > > > +     params.delayContext = bayerFrame.delayContext;
> > > >
> > > >       if (embeddedBuffer) {
> > > >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> > > >
> > > > -             ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;
> > > > -             ispPrepare.embeddedBufferPresent = true;
> > > > -
> > > > -             LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > > > +             params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;
> > > > +             LOG(RPI, Debug) << "Signalling prepareIsp:"
> > > >                               << " Embedded buffer id: " << embeddedId;
> > > >       }
> > > >
> > > > -     ipa_->signalIspPrepare(ispPrepare);
> > > > +     ipa_->prepareIsp(params);
> > > >  }
> > > >
> > > >  bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)
>
> --
> Regards,
>
> Laurent Pinchart

Hi Naush,

On Fri, Apr 28, 2023 at 10:43:23AM +0100, Naushir Patuck wrote:
> On Thu, 27 Apr 2023 at 18:15, Laurent Pinchart wrote:
> > On Thu, Apr 27, 2023 at 10:53:25AM +0100, Naushir Patuck via libcamera-devel wrote:
> > > On Thu, 27 Apr 2023 at 09:56, Jacopo Mondi wrote:
> > > > On Wed, Apr 26, 2023 at 02:10:50PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > > > Restructure the IPA mojom interface to be more consistent in the use
> > > > > of the API. Function parameters are now grouped into *Params structures
> > > > > and results are now returned in *Results structures.
> > > > >
> > > > > The following pipeline -> IPA interfaces have been removed:
> > > > >
> > > > > signalQueueRequest(libcamera.ControlList controls);
> > > > > signalIspPrepare(ISPConfig data);
> > > > > signalStatReady(uint32 bufferId, uint32 ipaContext);
> > > > >
> > > > > and replaced with:
> > > > >
> > > > > prepareIsp(PrepareParams params);
> > > > > processStats(ProcessParams params);
> > > > >
> > > > > signalQueueRequest() is now encompassed within signalPrepareIsp().
> >
> > I think you meant "within prepareIsp()" here.
> >
> > > > > The following IPA -> pipeline interfaces have been removed:
> > > > >
> > > > > runIsp(uint32 bufferId);
> > > > > embeddedComplete(uint32 bufferId);
> > > > > statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > > > >
> > > > > and replaced with the following async calls:
> > > > >
> > > > > prepareIspComplete(BufferIds buffers);
> > > > > processStatsComplete(BufferIds buffers);
> > > > > metadataReady(libcamera.ControlList metadata);
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > ---
> > > > >  include/libcamera/ipa/raspberrypi.mojom       | 127 +++++---------
> > > > >  src/ipa/rpi/vc4/raspberrypi.cpp               | 102 +++++-------
> > > > >  .../pipeline/rpi/vc4/raspberrypi.cpp          | 155 +++++++++---------
> > > > >  3 files changed, 165 insertions(+), 219 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > > index 80e0126618c8..7d56248f4ab3 100644
> > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > @@ -8,7 +8,7 @@ module ipa.RPi;
> > > > >
> > > > >  import "include/libcamera/ipa/core.mojom";
> > > > >
> > > > > -/* Size of the LS grid allocation. */
> > > > > +/* Size of the LS grid allocation on VC4. */
> > > > >  const uint32 MaxLsGridSize = 0x8000;
> > > > >
> > > > >  struct SensorConfig {
> > > > > @@ -19,117 +19,78 @@ struct SensorConfig {
> > > > >       uint32 sensorMetadata;
> > > > >  };
> > > > >
> > > > > -struct IPAInitResult {
> > > > > +struct InitParams {
> > > > > +     bool lensPresent;
> > > > > +};
> > > > > +
> > > > > +struct InitResult {
> > > > >       SensorConfig sensorConfig;
> > > > >       libcamera.ControlInfoMap controlInfo;
> > > > >  };
> > > > >
> > > > > -struct ISPConfig {
> > > > > -     uint32 embeddedBufferId;
> > > > > -     uint32 bayerBufferId;
> > > > > -     bool embeddedBufferPresent;
> > > > > -     libcamera.ControlList controls;
> > > > > -     uint32 ipaContext;
> > > > > -     uint32 delayContext;
> > > > > +struct BufferIds {
> > > > > +     uint32 bayer;
> > > > > +     uint32 embedded;
> > > > > +     uint32 stats;
> > > > >  };
> > > > >
> > > > > -struct IPAConfig {
> > > > > +struct ConfigParams {
> > > > >       uint32 transform;
> > > > > -     libcamera.SharedFD lsTableHandle;
> > > > >       libcamera.ControlInfoMap sensorControls;
> > > > >       libcamera.ControlInfoMap ispControls;
> > > > >       libcamera.ControlInfoMap lensControls;
> > > > > +        /* VC4 specifc */
> > > > > +     libcamera.SharedFD lsTableHandle;
> > > > >  };
> > > > >
> > > > > -struct IPAConfigResult {
> > > > > -       float modeSensitivity;
> > > > > -       libcamera.ControlInfoMap controlInfo;
> > > > > +struct ConfigResult {
> > > > > +     float modeSensitivity;
> > > > > +     libcamera.ControlInfoMap controlInfo;
> > > > > +     libcamera.ControlList controls;
> > > > >  };
> > > > >
> > > > > -struct StartConfig {
> > > > > +struct StartResult {
> > > > >       libcamera.ControlList controls;
> > > > >       int32 dropFrameCount;
> > > > >  };
> > > > >
> > > > > +struct PrepareParams {
> > > > > +     BufferIds buffers;
> >
> > The only part that bothers me a bit is usage of BufferIds here, as the
> > stats buffer isn't used.
> 
> Adding the stats buffer to BufferIds allows me to generalise the structure and
> use it throughout the interface.
> 
> It is also sort-of ' used right now, in that we see the absence of the stats
> buffer to "know" that the stats are not in-line with prepareISP.  However, this
> is not intended to stay that way, we will have a separate parameter to signal
> this if needed.

I suppose we can keep it as-is for now, even if it feels a bit like
shoe-horning a single IPA interface for different needs. The IPA
interface mechanism was designed to make it easy for pipeline handlers
and IPA modules to communicate with custom interfaces :-)

> > > > > +     libcamera.ControlList sensorControls;
> > > > > +     libcamera.ControlList requestControls;
> > > > > +     uint32 ipaContext;
> > > > > +     uint32 delayContext;
> > > > > +};
> > > > > +
> > > > > +struct ProcessParams {
> > > > > +     BufferIds buffers;
> > > > > +     uint32 ipaContext;
> > > > > +};
> > > > > +
> > > > >  interface IPARPiInterface {
> > > > > -     init(libcamera.IPASettings settings, bool lensPresent)
> > > > > -             => (int32 ret, IPAInitResult result);
> > > > > -     start(libcamera.ControlList controls) => (StartConfig startConfig);
> > > > > +     init(libcamera.IPASettings settings, InitParams params)
> > > > > +             => (int32 ret, InitResult result);
> > > > > +
> > > > > +     start(libcamera.ControlList controls) => (StartResult result);
> > > > >       stop();
> > > > >
> > > > > -     /**
> > > > > -      * \fn configure()
> > > >
> > > > Is it intentional to drop documentation ? Was it used at all ?
> > >
> > > I don't believe it is at all used.  I looked at ipu3 and rkisp1 and they have
> > > no doc tags either.
> >
> > It's not mandatory, but it's nice to have. Could you keep it if that's
> > not too much trouble ?
> 
> Ack
> 
> > > > > -      * \brief Configure the IPA stream and sensor settings
> > > > > -      * \param[in] sensorInfo Camera sensor information
> > > > > -      * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > > -      * \param[out] controls Controls to apply by the pipeline entity
> > > > > -      * \param[out] result Other results that the pipeline handler may require
> > > > > -      *
> > > > > -      * This function shall be called when the camera is configured to inform
> > > > > -      * the IPA of the camera's streams and the sensor settings.
> > > > > -      *
> > > > > -      * The \a sensorInfo conveys information about the camera sensor settings that
> > > > > -      * the pipeline handler has selected for the configuration.
> > > > > -      *
> > > > > -      * The \a ipaConfig and \a controls parameters carry data passed by the
> > > > > -      * pipeline handler to the IPA and back.
> > > > > -      */
> > > > > -     configure(libcamera.IPACameraSensorInfo sensorInfo,
> > > > > -               IPAConfig ipaConfig)
> > > > > -             => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > > > -
> > > > > -     /**
> > > > > -      * \fn mapBuffers()
> > > > > -      * \brief Map buffers shared between the pipeline handler and the IPA
> > > > > -      * \param[in] buffers List of buffers to map
> > > > > -      *
> > > > > -      * This function informs the IPA module of memory buffers set up by the
> > > > > -      * pipeline handler that the IPA needs to access. It provides dmabuf
> > > > > -      * file handles for each buffer, and associates the buffers with unique
> > > > > -      * numerical IDs.
> > > > > -      *
> > > > > -      * IPAs shall map the dmabuf file handles to their address space and
> > > > > -      * keep a cache of the mappings, indexed by the buffer numerical IDs.
> > > > > -      * The IDs are used in all other IPA interface functions to refer to
> > > > > -      * buffers, including the unmapBuffers() function.
> > > > > -      *
> > > > > -      * All buffers that the pipeline handler wishes to share with an IPA
> > > > > -      * shall be mapped with this function. Buffers may be mapped all at once
> > > > > -      * with a single call, or mapped and unmapped dynamically at runtime,
> > > > > -      * depending on the IPA protocol. Regardless of the protocol, all
> > > > > -      * buffers mapped at a given time shall have unique numerical IDs.
> > > > > -      *
> > > > > -      * The numerical IDs have no meaning defined by the IPA interface, and
> > > > > -      * should be treated as opaque handles by IPAs, with the only exception
> > > > > -      * that ID zero is invalid.
> > > > > -      *
> > > > > -      * \sa unmapBuffers()
> > > > > -      */
> > > > > -     mapBuffers(array<libcamera.IPABuffer> buffers);
> > > > > +     configure(libcamera.IPACameraSensorInfo sensorInfo, ConfigParams params)
> > > > > +             => (int32 ret, ConfigResult result);
> > > > >
> > > > > -     /**
> > > > > -      * \fn unmapBuffers()
> > > > > -      * \brief Unmap buffers shared by the pipeline to the IPA
> > > > > -      * \param[in] ids List of buffer IDs to unmap
> > > > > -      *
> > > > > -      * This function removes mappings set up with mapBuffers(). Numerical
> > > > > -      * IDs of unmapped buffers may be reused when mapping new buffers.
> > > > > -      *
> > > > > -      * \sa mapBuffers()
> > > > > -      */
> > > > > +     mapBuffers(array<libcamera.IPABuffer> buffers);
> > > > >       unmapBuffers(array<uint32> ids);
> > > > >
> > > > > -     [async] signalStatReady(uint32 bufferId, uint32 ipaContext);
> > > > > -     [async] signalQueueRequest(libcamera.ControlList controls);
> > > > > -     [async] signalIspPrepare(ISPConfig data);
> > > > > +     [async] prepareIsp(PrepareParams params);
> > > > > +     [async] processStats(ProcessParams params);
> > > > >  };
> > > > >
> > > > >  interface IPARPiEventInterface {
> > > > > -     statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> > > > > -     runIsp(uint32 bufferId);
> > > > > -     embeddedComplete(uint32 bufferId);
> > > > > +     prepareIspComplete(BufferIds buffers);
> > > > > +     processStatsComplete(BufferIds buffers);
> > > > > +     metadataReady(libcamera.ControlList metadata);
> > > > >       setIspControls(libcamera.ControlList controls);
> > > > >       setDelayedControls(libcamera.ControlList controls, uint32 delayContext);
> > > > >       setLensControls(libcamera.ControlList controls);
> > > > >       setCameraTimeout(uint32 maxFrameLengthMs);
> > > > >  };
> > > > > +
> > > >
> > > > Additional blank line
> > >
> > > Ack
> > >
> > > > The rest looks good
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >
> > > > > diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/vc4/raspberrypi.cpp
> > > > > index 841635ccde2b..d737f1d662a0 100644
> > > > > --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> > > > > +++ b/src/ipa/rpi/vc4/raspberrypi.cpp
> > > > > @@ -136,30 +136,28 @@ public:
> > > > >                       munmap(lsTable_, MaxLsGridSize);
> > > > >       }
> > > > >
> > > > > -     int init(const IPASettings &settings, bool lensPresent, IPAInitResult *result) override;
> > > > > -     void start(const ControlList &controls, StartConfig *startConfig) override;
> > > > > +     int init(const IPASettings &settings, const InitParams &params, InitResult *result) override;
> > > > > +     void start(const ControlList &controls, StartResult *result) override;
> > > > >       void stop() override {}
> > > > >
> > > > > -     int configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &data,
> > > > > -                   ControlList *controls, IPAConfigResult *result) override;
> > > > > +     int configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > > > > +                   ConfigResult *result) override;
> > > > >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > > -     void signalStatReady(const uint32_t bufferId, uint32_t ipaContext) override;
> > > > > -     void signalQueueRequest(const ControlList &controls) override;
> > > > > -     void signalIspPrepare(const ISPConfig &data) override;
> > > > > +     void prepareIsp(const PrepareParams &params) override;
> > > > > +     void processStats(const ProcessParams &params) override;
> > > > >
> > > > >  private:
> > > > >       void setMode(const IPACameraSensorInfo &sensorInfo);
> > > > >       bool validateSensorControls();
> > > > >       bool validateIspControls();
> > > > >       bool validateLensControls();
> > > > > -     void queueRequest(const ControlList &controls);
> > > > > -     void returnEmbeddedBuffer(unsigned int bufferId);
> > > > > -     void prepareISP(const ISPConfig &data);
> > > > > +     void applyControls(const ControlList &controls);
> > > > > +     void prepare(const PrepareParams &params);
> > > > >       void reportMetadata(unsigned int ipaContext);
> > > > >       void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> > > > >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> > > > > -     void processStats(unsigned int bufferId, unsigned int ipaContext);
> > > > > +     void process(unsigned int bufferId, unsigned int ipaContext);
> > > > >       void setCameraTimeoutValue();
> > > > >       void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
> > > > >       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> > > > > @@ -229,7 +227,7 @@ private:
> > > > >       Duration lastTimeout_;
> > > > >  };
> > > > >
> > > > > -int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> > > > > +int IPARPi::init(const IPASettings &settings, const InitParams &params, InitResult *result)
> > > > >  {
> > > > >       /*
> > > > >        * Load the "helper" for this sensor. This tells us all the device specific stuff
> > > > > @@ -274,7 +272,7 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> > > > >               return -EINVAL;
> > > > >       }
> > > > >
> > > > > -     lensPresent_ = lensPresent;
> > > > > +     lensPresent_ = params.lensPresent;
> > > > >
> > > > >       controller_.initialise();
> > > > >
> > > > > @@ -287,14 +285,13 @@ int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *r
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > -void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > > > +void IPARPi::start(const ControlList &controls, StartResult *result)
> > > > >  {
> > > > >       RPiController::Metadata metadata;
> > > > >
> > > > > -     ASSERT(startConfig);
> > > > >       if (!controls.empty()) {
> > > > >               /* We have been given some controls to action before start. */
> > > > > -             queueRequest(controls);
> > > > > +             applyControls(controls);
> > > > >       }
> > > > >
> > > > >       controller_.switchMode(mode_, &metadata);
> > > > > @@ -313,7 +310,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > > >       if (agcStatus.shutterTime && agcStatus.analogueGain) {
> > > > >               ControlList ctrls(sensorCtrls_);
> > > > >               applyAGC(&agcStatus, ctrls);
> > > > > -             startConfig->controls = std::move(ctrls);
> > > > > +             result->controls = std::move(ctrls);
> > > > >               setCameraTimeoutValue();
> > > > >       }
> > > > >
> > > > > @@ -360,7 +357,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > > >               mistrustCount_ = helper_->mistrustFramesModeSwitch();
> > > > >       }
> > > > >
> > > > > -     startConfig->dropFrameCount = dropFrameCount_;
> > > > > +     result->dropFrameCount = dropFrameCount_;
> > > > >
> > > > >       firstStart_ = false;
> > > > >       lastRunTimestamp_ = 0;
> > > > > @@ -435,11 +432,11 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)
> > > > >                            mode_.minFrameDuration, mode_.maxFrameDuration);
> > > > >  }
> > > > >
> > > > > -int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ipaConfig,
> > > > > -                   ControlList *controls, IPAConfigResult *result)
> > > > > +int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams &params,
> > > > > +                   ConfigResult *result)
> > > > >  {
> > > > > -     sensorCtrls_ = ipaConfig.sensorControls;
> > > > > -     ispCtrls_ = ipaConfig.ispControls;
> > > > > +     sensorCtrls_ = params.sensorControls;
> > > > > +     ispCtrls_ = params.ispControls;
> > > > >
> > > > >       if (!validateSensorControls()) {
> > > > >               LOG(IPARPI, Error) << "Sensor control validation failed.";
> > > > > @@ -452,7 +449,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > > >       }
> > > > >
> > > > >       if (lensPresent_) {
> > > > > -             lensCtrls_ = ipaConfig.lensControls;
> > > > > +             lensCtrls_ = params.lensControls;
> > > > >               if (!validateLensControls()) {
> > > > >                       LOG(IPARPI, Warning) << "Lens validation failed, "
> > > > >                                            << "no lens control will be available.";
> > > > > @@ -466,10 +463,10 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > > >       /* Re-assemble camera mode using the sensor info. */
> > > > >       setMode(sensorInfo);
> > > > >
> > > > > -     mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform);
> > > > > +     mode_.transform = static_cast<libcamera::Transform>(params.transform);
> > > > >
> > > > >       /* Store the lens shading table pointer and handle if available. */
> > > > > -     if (ipaConfig.lsTableHandle.isValid()) {
> > > > > +     if (params.lsTableHandle.isValid()) {
> > > > >               /* Remove any previous table, if there was one. */
> > > > >               if (lsTable_) {
> > > > >                       munmap(lsTable_, MaxLsGridSize);
> > > > > @@ -477,7 +474,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > > >               }
> > > > >
> > > > >               /* Map the LS table buffer into user space. */
> > > > > -             lsTableHandle_ = std::move(ipaConfig.lsTableHandle);
> > > > > +             lsTableHandle_ = std::move(params.lsTableHandle);
> > > > >               if (lsTableHandle_.isValid()) {
> > > > >                       lsTable_ = mmap(nullptr, MaxLsGridSize, PROT_READ | PROT_WRITE,
> > > > >                                       MAP_SHARED, lsTableHandle_.get(), 0);
> > > > > @@ -512,8 +509,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> > > > >               applyAGC(&agcStatus, ctrls);
> > > > >       }
> > > > >
> > > > > -     ASSERT(controls);
> > > > > -     *controls = std::move(ctrls);
> > > > > +     result->controls = std::move(ctrls);
> > > > >
> > > > >       /*
> > > > >        * Apply the correct limits to the exposure, gain and frame duration controls
> > > > > @@ -560,37 +556,34 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
> > > > >       }
> > > > >  }
> > > > >
> > > > > -void IPARPi::signalStatReady(uint32_t bufferId, uint32_t ipaContext)
> > > > > +void IPARPi::processStats(const ProcessParams &params)
> > > > >  {
> > > > > -     unsigned int context = ipaContext % rpiMetadata_.size();
> > > > > +     unsigned int context = params.ipaContext % rpiMetadata_.size();
> > > > >
> > > > >       if (++checkCount_ != frameCount_) /* assert here? */
> > > > >               LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> > > > >       if (processPending_ && frameCount_ > mistrustCount_)
> > > > > -             processStats(bufferId, context);
> > > > > +             process(params.buffers.stats, context);
> > > > >
> > > > >       reportMetadata(context);
> > > > > -
> > > > > -     statsMetadataComplete.emit(bufferId, libcameraMetadata_);
> > > > > +     processStatsComplete.emit(params.buffers);
> > > > >  }
> > > > >
> > > > > -void IPARPi::signalQueueRequest(const ControlList &controls)
> > > > > -{
> > > > > -     queueRequest(controls);
> > > > > -}
> > > > >
> > > > > -void IPARPi::signalIspPrepare(const ISPConfig &data)
> > > > > +void IPARPi::prepareIsp(const PrepareParams &params)
> > > > >  {
> > > > > +     applyControls(params.requestControls);
> > > > > +
> > > > >       /*
> > > > >        * At start-up, or after a mode-switch, we may want to
> > > > >        * avoid running the control algos for a few frames in case
> > > > >        * they are "unreliable".
> > > > >        */
> > > > > -     prepareISP(data);
> > > > > +     prepare(params);
> > > > >       frameCount_++;
> > > > >
> > > > >       /* Ready to push the input buffer into the ISP. */
> > > > > -     runIsp.emit(data.bayerBufferId);
> > > > > +     prepareIspComplete.emit(params.buffers);
> > > > >  }
> > > > >
> > > > >  void IPARPi::reportMetadata(unsigned int ipaContext)
> > > > > @@ -703,6 +696,8 @@ void IPARPi::reportMetadata(unsigned int ipaContext)
> > > > >               libcameraMetadata_.set(controls::AfState, s);
> > > > >               libcameraMetadata_.set(controls::AfPauseState, p);
> > > > >       }
> > > > > +
> > > > > +     metadataReady.emit(libcameraMetadata_);
> > > > >  }
> > > > >
> > > > >  bool IPARPi::validateSensorControls()
> > > > > @@ -826,7 +821,7 @@ static const std::map<int32_t, RPiController::AfAlgorithm::AfPause> AfPauseTable
> > > > >       { controls::AfPauseResume, RPiController::AfAlgorithm::AfPauseResume },
> > > > >  };
> > > > >
> > > > > -void IPARPi::queueRequest(const ControlList &controls)
> > > > > +void IPARPi::applyControls(const ControlList &controls)
> > > > >  {
> > > > >       using RPiController::AfAlgorithm;
> > > > >
> > > > > @@ -1256,27 +1251,22 @@ void IPARPi::queueRequest(const ControlList &controls)
> > > > >       }
> > > > >  }
> > > > >
> > > > > -void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > > > > +void IPARPi::prepare(const PrepareParams &params)
> > > > >  {
> > > > > -     embeddedComplete.emit(bufferId);
> > > > > -}
> > > > > -
> > > > > -void IPARPi::prepareISP(const ISPConfig &data)
> > > > > -{
> > > > > -     int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> > > > > -     unsigned int ipaContext = data.ipaContext % rpiMetadata_.size();
> > > > > +     int64_t frameTimestamp = params.sensorControls.get(controls::SensorTimestamp).value_or(0);
> > > > > +     unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
> > > > >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > > > >       Span<uint8_t> embeddedBuffer;
> > > > >
> > > > >       rpiMetadata.clear();
> > > > > -     fillDeviceStatus(data.controls, ipaContext);
> > > > > +     fillDeviceStatus(params.sensorControls, ipaContext);
> > > > >
> > > > > -     if (data.embeddedBufferPresent) {
> > > > > +     if (params.buffers.embedded) {
> > > > >               /*
> > > > >                * Pipeline handler has supplied us with an embedded data buffer,
> > > > >                * we must pass it to the CamHelper for parsing.
> > > > >                */
> > > > > -             auto it = buffers_.find(data.embeddedBufferId);
> > > > > +             auto it = buffers_.find(params.buffers.embedded);
> > > > >               ASSERT(it != buffers_.end());
> > > > >               embeddedBuffer = it->second.planes()[0];
> > > > >       }
> > > > > @@ -1288,7 +1278,7 @@ void IPARPi::prepareISP(const ISPConfig &data)
> > > > >        * metadata.
> > > > >        */
> > > > >       AgcStatus agcStatus;
> > > > > -     RPiController::Metadata &delayedMetadata = rpiMetadata_[data.delayContext];
> > > > > +     RPiController::Metadata &delayedMetadata = rpiMetadata_[params.delayContext];
> > > > >       if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> > > > >               rpiMetadata.set("agc.delayed_status", agcStatus);
> > > > >
> > > > > @@ -1298,10 +1288,6 @@ void IPARPi::prepareISP(const ISPConfig &data)
> > > > >        */
> > > > >       helper_->prepare(embeddedBuffer, rpiMetadata);
> > > > >
> > > > > -     /* Done with embedded data now, return to pipeline handler asap. */
> > > > > -     if (data.embeddedBufferPresent)
> > > > > -             returnEmbeddedBuffer(data.embeddedBufferId);
> > > > > -
> > > > >       /* Allow a 10% margin on the comparison below. */
> > > > >       Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> > > > >       if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > > > > @@ -1445,7 +1431,7 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co
> > > > >       return statistics;
> > > > >  }
> > > > >
> > > > > -void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> > > > > +void IPARPi::process(unsigned int bufferId, unsigned int ipaContext)
> > > > >  {
> > > > >       RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > > > index e54bf1ef2c17..30ce6feab0d1 100644
> > > > > --- a/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > > > @@ -200,15 +200,15 @@ public:
> > > > >       void freeBuffers();
> > > > >       void frameStarted(uint32_t sequence);
> > > > >
> > > > > -     int loadIPA(ipa::RPi::IPAInitResult *result);
> > > > > -     int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
> > > > > +     int loadIPA(ipa::RPi::InitResult *result);
> > > > > +     int configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result);
> > > > >       int loadPipelineConfiguration();
> > > > >
> > > > >       void enumerateVideoDevices(MediaLink *link);
> > > > >
> > > > > -     void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > > > -     void runIsp(uint32_t bufferId);
> > > > > -     void embeddedComplete(uint32_t bufferId);
> > > > > +     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);
> > > > >       void setLensControls(const ControlList &controls);
> > > > > @@ -238,7 +238,7 @@ public:
> > > > >       /* The vector below is just for convenience when iterating over all streams. */
> > > > >       std::vector<RPi::Stream *> streams_;
> > > > >       /* Stores the ids of the buffers mapped in the IPA. */
> > > > > -     std::unordered_set<unsigned int> ipaBuffers_;
> > > > > +     std::unordered_set<unsigned int> bufferIds_;
> > > > >       /*
> > > > >        * Stores a cascade of Video Mux or Bridge devices between the sensor and
> > > > >        * Unicam together with media link across the entities.
> > > > > @@ -1000,7 +1000,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > > > >
> > > > >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > > > >
> > > > > -     ipa::RPi::IPAConfigResult result;
> > > > > +     ipa::RPi::ConfigResult result;
> > > > >       ret = data->configureIPA(config, &result);
> > > > >       if (ret)
> > > > >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> > > > > @@ -1117,17 +1117,17 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > > > >               data->applyScalerCrop(*controls);
> > > > >
> > > > >       /* Start the IPA. */
> > > > > -     ipa::RPi::StartConfig startConfig;
> > > > > +     ipa::RPi::StartResult result;
> > > > >       data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
> > > > > -                       &startConfig);
> > > > > +                       &result);
> > > > >
> > > > >       /* Apply any gain/exposure settings that the IPA may have passed back. */
> > > > > -     if (!startConfig.controls.empty())
> > > > > -             data->setSensorControls(startConfig.controls);
> > > > > +     if (!result.controls.empty())
> > > > > +             data->setSensorControls(result.controls);
> > > > >
> > > > >       /* Configure the number of dropped frames required on startup. */
> > > > >       data->dropFrameCount_ = data->config_.disableStartupFrameDrops
> > > > > -                           ? 0 : startConfig.dropFrameCount;
> > > > > +                             ? 0 : result.dropFrameCount;
> > > > >
> > > > >       for (auto const stream : data->streams_)
> > > > >               stream->resetBuffers();
> > > > > @@ -1358,7 +1358,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > > > >
> > > > >       data->sensorFormats_ = populateSensorFormats(data->sensor_);
> > > > >
> > > > > -     ipa::RPi::IPAInitResult result;
> > > > > +     ipa::RPi::InitResult result;
> > > > >       if (data->loadIPA(&result)) {
> > > > >               LOG(RPI, Error) << "Failed to load a suitable IPA library";
> > > > >               return -EINVAL;
> > > > > @@ -1599,7 +1599,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > > >  void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask)
> > > > >  {
> > > > >       RPiCameraData *data = cameraData(camera);
> > > > > -     std::vector<IPABuffer> ipaBuffers;
> > > > > +     std::vector<IPABuffer> bufferIds;
> > > > >       /*
> > > > >        * Link the FrameBuffers with the id (key value) in the map stored in
> > > > >        * the RPi stream object - along with an identifier mask.
> > > > > @@ -1608,12 +1608,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
> > > > >        * handler and the IPA.
> > > > >        */
> > > > >       for (auto const &it : buffers) {
> > > > > -             ipaBuffers.push_back(IPABuffer(mask | it.first,
> > > > > +             bufferIds.push_back(IPABuffer(mask | it.first,
> > > > >                                              it.second->planes()));
> > > > > -             data->ipaBuffers_.insert(mask | it.first);
> > > > > +             data->bufferIds_.insert(mask | it.first);
> > > > >       }
> > > > >
> > > > > -     data->ipa_->mapBuffers(ipaBuffers);
> > > > > +     data->ipa_->mapBuffers(bufferIds);
> > > > >  }
> > > > >
> > > > >  void RPiCameraData::freeBuffers()
> > > > > @@ -1623,10 +1623,10 @@ void RPiCameraData::freeBuffers()
> > > > >                * Copy the buffer ids from the unordered_set to a vector to
> > > > >                * pass to the IPA.
> > > > >                */
> > > > > -             std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(),
> > > > > -                                                  ipaBuffers_.end());
> > > > > -             ipa_->unmapBuffers(ipaBuffers);
> > > > > -             ipaBuffers_.clear();
> > > > > +             std::vector<unsigned int> bufferIds(bufferIds_.begin(),
> > > > > +                                                 bufferIds_.end());
> > > > > +             ipa_->unmapBuffers(bufferIds);
> > > > > +             bufferIds_.clear();
> > > > >       }
> > > > >
> > > > >       for (auto const stream : streams_)
> > > > > @@ -1643,16 +1643,16 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> > > > >       delayedCtrls_->applyControls(sequence);
> > > > >  }
> > > > >
> > > > > -int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > > > > +int RPiCameraData::loadIPA(ipa::RPi::InitResult *result)
> > > > >  {
> > > > >       ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
> > > > >
> > > > >       if (!ipa_)
> > > > >               return -ENOENT;
> > > > >
> > > > > -     ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> > > > > -     ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> > > > > -     ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > > > > +     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);
> > > > > @@ -1674,23 +1674,25 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> > > > >       }
> > > > >
> > > > >       IPASettings settings(configurationFile, sensor_->model());
> > > > > +     ipa::RPi::InitParams params;
> > > > >
> > > > > -     return ipa_->init(settings, !!sensor_->focusLens(), result);
> > > > > +     params.lensPresent = !!sensor_->focusLens();
> > > > > +     return ipa_->init(settings, params, result);
> > > > >  }
> > > > >
> > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
> > > > > +int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::ConfigResult *result)
> > > > >  {
> > > > >       std::map<unsigned int, ControlInfoMap> entityControls;
> > > > > -     ipa::RPi::IPAConfig ipaConfig;
> > > > > +     ipa::RPi::ConfigParams params;
> > > > >
> > > > >       /* \todo Move passing of ispControls and lensControls to ipa::init() */
> > > > > -     ipaConfig.sensorControls = sensor_->controls();
> > > > > -     ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();
> > > > > +     params.sensorControls = sensor_->controls();
> > > > > +     params.ispControls = isp_[Isp::Input].dev()->controls();
> > > > >       if (sensor_->focusLens())
> > > > > -             ipaConfig.lensControls = sensor_->focusLens()->controls();
> > > > > +             params.lensControls = sensor_->focusLens()->controls();
> > > > >
> > > > >       /* Always send the user transform to the IPA. */
> > > > > -     ipaConfig.transform = static_cast<unsigned int>(config->transform);
> > > > > +     params.transform = static_cast<unsigned int>(config->transform);
> > > > >
> > > > >       /* Allocate the lens shading table via dmaHeap and pass to the IPA. */
> > > > >       if (!lsTable_.isValid()) {
> > > > > @@ -1703,7 +1705,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > > > >                * \todo Investigate if mapping the lens shading table buffer
> > > > >                * could be handled with mapBuffers().
> > > > >                */
> > > > > -             ipaConfig.lsTableHandle = lsTable_;
> > > > > +             params.lsTableHandle = lsTable_;
> > > > >       }
> > > > >
> > > > >       /* We store the IPACameraSensorInfo for digital zoom calculations. */
> > > > > @@ -1714,15 +1716,14 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA
> > > > >       }
> > > > >
> > > > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > > > -     ControlList controls;
> > > > > -     ret = ipa_->configure(sensorInfo_, ipaConfig, &controls, result);
> > > > > +     ret = ipa_->configure(sensorInfo_, params, result);
> > > > >       if (ret < 0) {
> > > > >               LOG(RPI, Error) << "IPA configuration failed!";
> > > > >               return -EPIPE;
> > > > >       }
> > > > >
> > > > > -     if (!controls.empty())
> > > > > -             setSensorControls(controls);
> > > > > +     if (!result->controls.empty())
> > > > > +             setSensorControls(result->controls);
> > > > >
> > > > >       return 0;
> > > > >  }
> > > > > @@ -1883,24 +1884,32 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)
> > > > >       }
> > > > >  }
> > > > >
> > > > > -void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
> > > > > +void RPiCameraData::processStatsComplete(const ipa::RPi::BufferIds &buffers)
> > > > >  {
> > > > >       if (!isRunning())
> > > > >               return;
> > > > >
> > > > > -     FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId & RPi::MaskID);
> > > > > +     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. */
> > > > >       Request *request = requestQueue_.front();
> > > > > -     request->metadata().merge(controls);
> > > > > +     request->metadata().merge(metadata);
> > > > >
> > > > >       /*
> > > > >        * Inform the sensor of the latest colour gains if it has the
> > > > >        * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set).
> > > > >        */
> > > > > -     const auto &colourGains = controls.get(libcamera::controls::ColourGains);
> > > > > +     const auto &colourGains = metadata.get(libcamera::controls::ColourGains);
> > > > >       if (notifyGainsUnity_ && colourGains) {
> > > > >               /* The control wants linear gains in the order B, Gb, Gr, R. */
> > > > >               ControlList ctrls(sensor_->controls());
> > > > > @@ -1914,33 +1923,29 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> > > > >
> > > > >               sensor_->setControls(&ctrls);
> > > > >       }
> > > > > -
> > > > > -     state_ = State::IpaComplete;
> > > > > -     handleState();
> > > > >  }
> > > > >
> > > > > -void RPiCameraData::runIsp(uint32_t bufferId)
> > > > > +void RPiCameraData::prepareIspComplete(const ipa::RPi::BufferIds &buffers)
> > > > >  {
> > > > > +     unsigned int embeddedId = buffers.embedded & RPi::MaskID;
> > > > > +     unsigned int bayer = buffers.bayer & RPi::MaskID;
> > > > > +     FrameBuffer *buffer;
> > > > > +
> > > > >       if (!isRunning())
> > > > >               return;
> > > > >
> > > > > -     FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId & RPi::MaskID);
> > > > > -
> > > > > -     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bufferId & RPi::MaskID)
> > > > > +     buffer = unicam_[Unicam::Image].getBuffers().at(bayer & RPi::MaskID);
> > > > > +     LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << (bayer & RPi::MaskID)
> > > > >                       << ", timestamp: " << buffer->metadata().timestamp;
> > > > >
> > > > >       isp_[Isp::Input].queueBuffer(buffer);
> > > > >       ispOutputCount_ = 0;
> > > > > -     handleState();
> > > > > -}
> > > > >
> > > > > -void RPiCameraData::embeddedComplete(uint32_t bufferId)
> > > > > -{
> > > > > -     if (!isRunning())
> > > > > -             return;
> > > > > +     if (sensorMetadata_ && embeddedId) {
> > > > > +             buffer = unicam_[Unicam::Embedded].getBuffers().at(embeddedId & RPi::MaskID);
> > > > > +             handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > > > > +     }
> > > > >
> > > > > -     FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId & RPi::MaskID);
> > > > > -     handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > > > >       handleState();
> > > > >  }
> > > > >
> > > > > @@ -2116,8 +2121,10 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > > > >        * application until after the IPA signals so.
> > > > >        */
> > > > >       if (stream == &isp_[Isp::Stats]) {
> > > > > -             ipa_->signalStatReady(RPi::MaskStats | static_cast<unsigned int>(index),
> > > > > -                                   requestQueue_.front()->sequence());
> > > > > +             ipa::RPi::ProcessParams params;
> > > > > +             params.buffers.stats = index | RPi::MaskStats;
> > > > > +             params.ipaContext = requestQueue_.front()->sequence();
> > > > > +             ipa_->processStats(params);
> > > > >       } else {
> > > > >               /* Any other ISP output can be handed back to the application now. */
> > > > >               handleStreamBuffer(buffer, stream);
> > > > > @@ -2344,38 +2351,30 @@ void RPiCameraData::tryRunPipeline()
> > > > >       request->metadata().clear();
> > > > >       fillRequestMetadata(bayerFrame.controls, request);
> > > > >
> > > > > -     /*
> > > > > -      * Process all the user controls by the IPA. Once this is complete, we
> > > > > -      * queue the ISP output buffer listed in the request to start the HW
> > > > > -      * pipeline.
> > > > > -      */
> > > > > -     ipa_->signalQueueRequest(request->controls());
> > > > > -
> > > > >       /* Set our state to say the pipeline is active. */
> > > > >       state_ = State::Busy;
> > > > >
> > > > > -     unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > > > > +     unsigned int bayer = unicam_[Unicam::Image].getBufferId(bayerFrame.buffer);
> > > > >
> > > > > -     LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > > > > -                     << " Bayer buffer id: " << bayerId;
> > > > > +     LOG(RPI, Debug) << "Signalling prepareIsp:"
> > > > > +                     << " Bayer buffer id: " << bayer;
> > > > >
> > > > > -     ipa::RPi::ISPConfig ispPrepare;
> > > > > -     ispPrepare.bayerBufferId = RPi::MaskBayerData | bayerId;
> > > > > -     ispPrepare.controls = std::move(bayerFrame.controls);
> > > > > -     ispPrepare.ipaContext = request->sequence();
> > > > > -     ispPrepare.delayContext = bayerFrame.delayContext;
> > > > > +     ipa::RPi::PrepareParams params;
> > > > > +     params.buffers.bayer = RPi::MaskBayerData | bayer;
> > > > > +     params.sensorControls = std::move(bayerFrame.controls);
> > > > > +     params.requestControls = request->controls();
> > > > > +     params.ipaContext = request->sequence();
> > > > > +     params.delayContext = bayerFrame.delayContext;
> > > > >
> > > > >       if (embeddedBuffer) {
> > > > >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
> > > > >
> > > > > -             ispPrepare.embeddedBufferId = RPi::MaskEmbeddedData | embeddedId;
> > > > > -             ispPrepare.embeddedBufferPresent = true;
> > > > > -
> > > > > -             LOG(RPI, Debug) << "Signalling signalIspPrepare:"
> > > > > +             params.buffers.embedded = RPi::MaskEmbeddedData | embeddedId;
> > > > > +             LOG(RPI, Debug) << "Signalling prepareIsp:"
> > > > >                               << " Embedded buffer id: " << embeddedId;
> > > > >       }
> > > > >
> > > > > -     ipa_->signalIspPrepare(ispPrepare);
> > > > > +     ipa_->prepareIsp(params);
> > > > >  }
> > > > >
> > > > >  bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)