[libcamera-devel,v6,09/11] libcamera: pipeline, ipa: raspberrypi: Use new data definition

10730 diff mbox series

Now that we can generate custom functions and data structures with mojo,
switch the raspberrypi pipeline handler and IPA to use the custom data
structures as defined in the mojom data definition file.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v6:
- rebase on "pipeline: ipa: raspberrypi: Handle failures during IPA
  configuration"
- move the enums and consts to the mojom file
  - use them with the namespace in the IPA and pipeline handler
- no longer need to initialize the ControlInfoMap
- fill in the LS table handle in the pipeline handler instead of passing
  it from the IPA (which was passed to the IPA from the pipeline handler
  in the first place)
- use custom start()
- no more postfix _ in generated struct fields

Changes in v5:
- minor fixes
- use new struct names

Changes in v4:
- rename Controls to controls
- use the renamed header (raspberrypi_ipa_interface.h)

Changes in v3:
- use ipa::rpi namespace
- rebase on the RPi namespace patch series newly merged into master

Changes in v2:
- rebased on "libcamera: pipeline: ipa: raspberrypi: Rework drop frame
  signalling"
- use newly customized RPi IPA interface
---
 include/libcamera/ipa/raspberrypi.h           |  11 -
 src/ipa/raspberrypi/raspberrypi.cpp           | 181 ++++++-------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 244 +++++++++---------
 .../pipeline/raspberrypi/rpi_stream.cpp       |   6 +-
 .../pipeline/raspberrypi/rpi_stream.h         |   5 +-
 5 files changed, 208 insertions(+), 239 deletions(-)

Hi Paul,

Thank you for the patch.

On Thu, Dec 24, 2020 at 05:16:11PM +0900, Paul Elder wrote:
> Now that we can generate custom functions and data structures with mojo,
> switch the raspberrypi pipeline handler and IPA to use the custom data
> structures as defined in the mojom data definition file.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v6:
> - rebase on "pipeline: ipa: raspberrypi: Handle failures during IPA
>   configuration"
> - move the enums and consts to the mojom file
>   - use them with the namespace in the IPA and pipeline handler
> - no longer need to initialize the ControlInfoMap
> - fill in the LS table handle in the pipeline handler instead of passing
>   it from the IPA (which was passed to the IPA from the pipeline handler
>   in the first place)
> - use custom start()
> - no more postfix _ in generated struct fields
> 
> Changes in v5:
> - minor fixes
> - use new struct names
> 
> Changes in v4:
> - rename Controls to controls
> - use the renamed header (raspberrypi_ipa_interface.h)
> 
> Changes in v3:
> - use ipa::rpi namespace
> - rebase on the RPi namespace patch series newly merged into master
> 
> Changes in v2:
> - rebased on "libcamera: pipeline: ipa: raspberrypi: Rework drop frame
>   signalling"
> - use newly customized RPi IPA interface
> ---
>  include/libcamera/ipa/raspberrypi.h           |  11 -
>  src/ipa/raspberrypi/raspberrypi.cpp           | 181 ++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 244 +++++++++---------
>  .../pipeline/raspberrypi/rpi_stream.cpp       |   6 +-
>  .../pipeline/raspberrypi/rpi_stream.h         |   5 +-
>  5 files changed, 208 insertions(+), 239 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index df30b4a2..70ecc5a8 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -16,17 +16,6 @@ namespace libcamera {
>  
>  namespace RPi {
>  
> -enum BufferMask {
> -	ID		= 0x00ffff,
> -	STATS		= 0x010000,
> -	EMBEDDED_DATA	= 0x020000,
> -	BAYER_DATA	= 0x040000,
> -	EXTERNAL_BUFFER	= 0x100000,
> -};
> -
> -/* Size of the LS grid allocation. */
> -static constexpr unsigned int MaxLsGridSize = 32 << 10;
> -
>  /* List of controls handled by the Raspberry Pi IPA */
>  static const ControlInfoMap Controls = {
>  	{ &controls::AeEnable, ControlInfo(false, true) },
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 67f47c11..5acc25a0 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -20,6 +20,7 @@
>  #include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/ipa/raspberrypi.h>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
>  #include <libcamera/request.h>
>  #include <libcamera/span.h>
>  
> @@ -59,7 +60,7 @@ constexpr unsigned int DefaultExposureTime = 20000;
>  
>  LOG_DEFINE_CATEGORY(IPARPI)
>  
> -class IPARPi : public IPAInterface
> +class IPARPi : public ipa::rpi::IPARPiInterface
>  {
>  public:
>  	IPARPi()
> @@ -72,21 +73,24 @@ public:
>  	~IPARPi()
>  	{
>  		if (lsTable_)
> -			munmap(lsTable_, RPi::MaxLsGridSize);
> +			munmap(lsTable_, ipa::rpi::MaxLsGridSize);
>  	}
>  
>  	int init(const IPASettings &settings) override;
> -	int start(const IPAOperationData &data, IPAOperationData *result) override;
> +	void start(const ipa::rpi::StartControls &data,
> +		   ipa::rpi::StartControls *result, int *ret) override;
>  	void stop() override {}
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &ipaConfig,
> -		       IPAOperationData *response) override;
> +		       const std::map<unsigned int, ControlInfoMap> &entityControls,
> +		       const ipa::rpi::ConfigInput &data,
> +		       ipa::rpi::ConfigOutput *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> -	void processEvent(const IPAOperationData &event) override;
> +	void signalStatReady(const uint32_t bufferId) override;
> +	void signalQueueRequest(const ControlList &controls) override;
> +	void signalIspPrepare(const ipa::rpi::ISPConfig &data) override;
>  
>  private:
>  	void setMode(const CameraSensorInfo &sensorInfo);
> @@ -156,15 +160,17 @@ int IPARPi::init(const IPASettings &settings)
>  	return 0;
>  }
>  
> -int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
> +void IPARPi::start(const ipa::rpi::StartControls &data,
> +		   ipa::rpi::StartControls *result, int *ret)
>  {
>  	RPiController::Metadata metadata;
>  
> +	ASSERT(ret);
>  	ASSERT(result);
> -	result->operation = 0;
> -	if (data.operation & RPi::IPA_CONFIG_STARTUP) {
> +	result->hasControls = false;
> +	if (data.hasControls) {
>  		/* We have been given some controls to action before start. */
> -		queueRequest(data.controls[0]);
> +		queueRequest(data.controls);
>  	}
>  
>  	controller_.SwitchMode(mode_, &metadata);
> @@ -179,8 +185,8 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
>  	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
>  		ControlList ctrls(sensorCtrls_);
>  		applyAGC(&agcStatus, ctrls);
> -		result->controls.emplace_back(ctrls);
> -		result->operation |= RPi::IPA_CONFIG_SENSOR;
> +		result->controls = std::move(ctrls);
> +		result->hasControls = true;
>  	}
>  
>  	/*
> @@ -227,12 +233,11 @@ int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
>  		mistrustCount_ = helper_->MistrustFramesModeSwitch();
>  	}
>  
> -	result->data.push_back(dropFrame);
> -	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> +	result->dropFrameCount = dropFrame;
>  
>  	firstStart_ = false;
>  
> -	return 0;
> +	*ret = 0;

As start() seems to never fail, you could drop the ret parameter.

>  }
>  
>  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> @@ -275,30 +280,30 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  
>  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &ipaConfig,
> -		       IPAOperationData *result)
> +		       const std::map<unsigned int, ControlInfoMap> &entityControls,
> +		       const ipa::rpi::ConfigInput &ipaConfig,
> +		       ipa::rpi::ConfigOutput *result)
>  {
>  	if (entityControls.size() != 2) {
>  		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> -		result->operation = RPi::IPA_CONFIG_FAILED;
> +		result->op = ipa::rpi::ConfigFailed;
>  		return;
>  	}
>  
> -	result->operation = 0;
> +	result->op = 0;
>  
>  	sensorCtrls_ = entityControls.at(0);
>  	ispCtrls_ = entityControls.at(1);
>  
>  	if (!validateSensorControls()) {
>  		LOG(IPARPI, Error) << "Sensor control validation failed.";
> -		result->operation = RPi::IPA_CONFIG_FAILED;
> +		result->op = ipa::rpi::ConfigFailed;
>  		return;
>  	}
>  
>  	if (!validateIspControls()) {
>  		LOG(IPARPI, Error) << "ISP control validation failed.";
> -		result->operation = RPi::IPA_CONFIG_FAILED;
> +		result->op = ipa::rpi::ConfigFailed;
>  		return;
>  	}
>  
> @@ -317,7 +322,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		if (!helper_) {
>  			LOG(IPARPI, Error) << "Could not create camera helper for "
>  					   << cameraName;
> -			result->operation = RPi::IPA_CONFIG_FAILED;
> +			result->op = ipa::rpi::ConfigFailed;
>  			return;
>  		}
>  
> @@ -329,34 +334,29 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		helper_->GetDelays(exposureDelay, gainDelay);
>  		sensorMetadata = helper_->SensorEmbeddedDataPresent();
>  
> -		result->data.push_back(gainDelay);
> -		result->data.push_back(exposureDelay);
> -		result->data.push_back(sensorMetadata);
> -
> -		result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> +		result->op |= ipa::rpi::ConfigStaggeredWrite;
> +		result->sensorConfig.gainDelay = gainDelay;
> +		result->sensorConfig.exposureDelay = exposureDelay;
> +		result->sensorConfig.sensorMetadata = sensorMetadata;
>  	}
>  
>  	/* Re-assemble camera mode using the sensor info. */
>  	setMode(sensorInfo);
>  
> -	/*
> -	 * The ipaConfig.data always gives us the user transform first. Note that
> -	 * this will always make the LS table pointer (if present) element 1.
> -	 */
> -	mode_.transform = static_cast<libcamera::Transform>(ipaConfig.data[0]);
> +	mode_.transform = static_cast<libcamera::Transform>(ipaConfig.transform);
>  
>  	/* Store the lens shading table pointer and handle if available. */
> -	if (ipaConfig.operation & RPi::IPA_CONFIG_LS_TABLE) {
> +	if (ipaConfig.op & ipa::rpi::ConfigLsTable) {
>  		/* Remove any previous table, if there was one. */
>  		if (lsTable_) {
> -			munmap(lsTable_, RPi::MaxLsGridSize);
> +			munmap(lsTable_, ipa::rpi::MaxLsGridSize);
>  			lsTable_ = nullptr;
>  		}
>  
> -		/* Map the LS table buffer into user space (now element 1). */
> -		lsTableHandle_ = FileDescriptor(ipaConfig.data[1]);
> +		/* Map the LS table buffer into user space. */
> +		lsTableHandle_ = FileDescriptor(ipaConfig.lsTableHandle);

Isn't ipaConfig.lsTableHandle already a FileDescriptor ?

>  		if (lsTableHandle_.isValid()) {
> -			lsTable_ = mmap(nullptr, RPi::MaxLsGridSize, PROT_READ | PROT_WRITE,
> +			lsTable_ = mmap(nullptr, ipa::rpi::MaxLsGridSize, PROT_READ | PROT_WRITE,
>  					MAP_SHARED, lsTableHandle_.fd(), 0);
>  
>  			if (lsTable_ == MAP_FAILED) {
> @@ -382,8 +382,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		agcStatus.analogue_gain = DefaultAnalogueGain;
>  		applyAGC(&agcStatus, ctrls);
>  
> -		result->controls.emplace_back(ctrls);
> -		result->operation |= RPi::IPA_CONFIG_SENSOR;
> +		result->op |= ipa::rpi::ConfigSensor;
> +		result->controls = std::move(ctrls);
>  	}
>  
>  	lastMode_ = mode_;
> @@ -408,56 +408,38 @@ void IPARPi::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> -void IPARPi::processEvent(const IPAOperationData &event)
> +void IPARPi::signalStatReady(const uint32_t bufferId)

For arithemtic arguments we could drop the const in the template. If
it's too difficult, it doesn't matter much.

>  {
> -	switch (event.operation) {
> -	case RPi::IPA_EVENT_SIGNAL_STAT_READY: {
> -		unsigned int bufferId = event.data[0];
> -
> -		if (++checkCount_ != frameCount_) /* assert here? */
> -			LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> -		if (frameCount_ > mistrustCount_)
> -			processStats(bufferId);
> -
> -		reportMetadata();
> -
> -		IPAOperationData op;
> -		op.operation = RPi::IPA_ACTION_STATS_METADATA_COMPLETE;
> -		op.data = { bufferId & RPi::BufferMask::ID };
> -		op.controls = { libcameraMetadata_ };
> -		queueFrameAction.emit(0, op);
> -		break;
> -	}
> +	if (++checkCount_ != frameCount_) /* assert here? */
> +		LOG(IPARPI, Error) << "WARNING: Prepare/Process mismatch!!!";
> +	if (frameCount_ > mistrustCount_)
> +		processStats(bufferId);
>  
> -	case RPi::IPA_EVENT_SIGNAL_ISP_PREPARE: {
> -		unsigned int embeddedbufferId = event.data[0];
> -		unsigned int bayerbufferId = event.data[1];
> +	reportMetadata();
>  
> -		/*
> -		 * 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(embeddedbufferId);
> -		frameCount_++;
> -
> -		/* Ready to push the input buffer into the ISP. */
> -		IPAOperationData op;
> -		op.operation = RPi::IPA_ACTION_RUN_ISP;
> -		op.data = { bayerbufferId & RPi::BufferMask::ID };
> -		queueFrameAction.emit(0, op);
> -		break;
> -	}
> +	statsMetadataComplete.emit(bufferId & ipa::rpi::MaskID, libcameraMetadata_);
> +}
>  
> -	case RPi::IPA_EVENT_QUEUE_REQUEST: {
> -		queueRequest(event.controls[0]);
> -		break;
> -	}
> +void IPARPi::signalQueueRequest(const ControlList &controls)
> +{
> +	queueRequest(controls);
> +}
>  
> -	default:
> -		LOG(IPARPI, Error) << "Unknown event " << event.operation;
> -		break;
> -	}
> +void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)
> +{
> +	unsigned int embeddedbufferId = data.embeddedbufferId;
> +	unsigned int bayerbufferId = data.bayerbufferId;
> +
> +	/*
> +	 * 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(embeddedbufferId);

You could write

	prepareISP(data.embeddedbufferId);

and drop the local variable.

> +	frameCount_++;
> +
> +	/* Ready to push the input buffer into the ISP. */
> +	runIsp.emit(bayerbufferId & ipa::rpi::MaskID);

Same here.

>  }
>  
>  void IPARPi::reportMetadata()
> @@ -815,10 +797,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>  
>  void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>  {
> -	IPAOperationData op;
> -	op.operation = RPi::IPA_ACTION_EMBEDDED_COMPLETE;
> -	op.data = { bufferId & RPi::BufferMask::ID };
> -	queueFrameAction.emit(0, op);
> +	embeddedComplete.emit(bufferId & ipa::rpi::MaskID);
>  }
>  
>  void IPARPi::prepareISP(unsigned int bufferId)
> @@ -879,12 +858,8 @@ void IPARPi::prepareISP(unsigned int bufferId)
>  		if (dpcStatus)
>  			applyDPC(dpcStatus, ctrls);
>  
> -		if (!ctrls.empty()) {
> -			IPAOperationData op;
> -			op.operation = RPi::IPA_ACTION_V4L2_SET_ISP;
> -			op.controls.push_back(ctrls);
> -			queueFrameAction.emit(0, op);
> -		}
> +		if (!ctrls.empty())
> +			setIsp.emit(ctrls);
>  	}
>  }
>  
> @@ -941,10 +916,7 @@ void IPARPi::processStats(unsigned int bufferId)
>  		ControlList ctrls(sensorCtrls_);
>  		applyAGC(&agcStatus, ctrls);
>  
> -		IPAOperationData op;
> -		op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;
> -		op.controls.emplace_back(ctrls);
> -		queueFrameAction.emit(0, op);
> +		setStaggered.emit(ctrls);
>  	}
>  }
>  
> @@ -1114,13 +1086,14 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
>  		.grid_width = w,
>  		.grid_stride = w,
>  		.grid_height = h,
> -		.dmabuf = lsTableHandle_.fd(),
> +		/* .dmabuf will be filled in by pipeline handler. */
> +		.dmabuf = 0,
>  		.ref_transform = 0,
>  		.corner_sampled = 1,
>  		.gain_format = GAIN_FORMAT_U4P10
>  	};
>  
> -	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > RPi::MaxLsGridSize) {
> +	if (!lsTable_ || w * h * 4 * sizeof(uint16_t) > ipa::rpi::MaxLsGridSize) {
>  		LOG(IPARPI, Error) << "Do not have a correctly allocate lens shading table!";
>  		return;
>  	}
> @@ -1191,9 +1164,9 @@ const struct IPAModuleInfo ipaModuleInfo = {
>  	"raspberrypi",
>  };
>  
> -struct ipa_context *ipaCreate()
> +IPAInterface *ipaCreate()
>  {
> -	return new IPAInterfaceWrapper(std::make_unique<IPARPi>());

Is there a corresponding header that could be dropped ?

> +	return new IPARPi();
>  }
>  
>  } /* extern "C" */
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 7a5f5881..4152ab69 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -17,11 +17,15 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/file_descriptor.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/ipa/core_ipa_interface.h>

This is included by libcamera/ipa/raspberrypi_ipa_interface.h, do we
need to include it manually ?

>  #include <libcamera/ipa/raspberrypi.h>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> +#include <libcamera/ipa/raspberrypi_ipa_proxy.h>
>  #include <libcamera/logging.h>
>  #include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  
> +#include <linux/bcm2835-isp.h>
>  #include <linux/videodev2.h>
>  
>  #include "libcamera/internal/bayer_format.h"
> @@ -146,7 +150,11 @@ public:
>  	int loadIPA();
>  	int configureIPA(const CameraConfiguration *config);
>  
> -	void queueFrameAction(unsigned int frame, const IPAOperationData &action);
> +	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> +	void runIsp(uint32_t bufferId);
> +	void embeddedComplete(uint32_t bufferId);
> +	void setIsp(const ControlList &controls);
> +	void setStaggered(const ControlList &controls);
>  
>  	/* bufferComplete signal handlers. */
>  	void unicamBufferDequeue(FrameBuffer *buffer);
> @@ -159,6 +167,8 @@ public:
>  	void handleState();
>  	void applyScalerCrop(const ControlList &controls);
>  
> +	std::unique_ptr<ipa::rpi::IPAProxyRPi> ipa_;
> +
>  	std::unique_ptr<CameraSensor> sensor_;
>  	/* Array of Unicam and ISP device streams and associated buffers/streams. */
>  	RPi::Device<Unicam, 2> unicam_;
> @@ -733,7 +743,7 @@ int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stre
>  	return ret;
>  }
>  
> -int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *controls)
> +int PipelineHandlerRPi::start(Camera *camera, ControlList *controls)
>  {
>  	RPiCameraData *data = cameraData(camera);
>  	int ret;
> @@ -751,13 +761,13 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  		data->applyScalerCrop(*controls);
>  
>  	/* Start the IPA. */
> -	IPAOperationData ipaData = {};
> -	IPAOperationData result = {};
> +	ipa::rpi::StartControls ipaData = {};
> +	ipa::rpi::StartControls result = {};

No need for = {}, there's a default constructor.

>  	if (controls) {
> -		ipaData.operation = RPi::IPA_CONFIG_STARTUP;
> -		ipaData.controls.emplace_back(*controls);
> +		ipaData.hasControls = true;
> +		ipaData.controls = std::move(*controls);

Maybe we could drop the hasControls field and rely on
ipaData.controls.empty() ?

>  	}
> -	ret = data->ipa_->start(ipaData, &result);
> +	data->ipa_->start(ipaData, &result, &ret);
>  	if (ret) {
>  		LOG(RPI, Error)
>  			<< "Failed to start IPA for " << camera->id();
> @@ -767,15 +777,15 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  
>  	/* Apply any gain/exposure settings that the IPA may have passed back. */
>  	ASSERT(data->staggeredCtrl_);
> -	if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> -		const ControlList &ctrls = result.controls[0];
> +	if (result.hasControls) {
> +		const ControlList &ctrls = result.controls;
>  		if (!data->staggeredCtrl_.set(ctrls))
>  			LOG(RPI, Error) << "V4L2 staggered set failed";
>  	}
>  
> -	if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {
> +	if (result.dropFrameCount > 0) {

I think you can drop the condition, RPi::IPA_CONFIG_DROP_FRAMES was set
unconditionally.

>  		/* Configure the number of dropped frames required on startup. */
> -		data->dropFrameCount_ = result.data[0];
> +		data->dropFrameCount_ = result.dropFrameCount;
>  	}
>  
>  	/* We need to set the dropFrameCount_ before queueing buffers. */
> @@ -1102,8 +1112,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * Pass the stats and embedded data buffers to the IPA. No other
>  	 * buffers need to be passed.
>  	 */
> -	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPi::BufferMask::STATS);
> -	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPi::BufferMask::EMBEDDED_DATA);
> +	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), ipa::rpi::MaskStats);
> +	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), ipa::rpi::MaskEmbeddedData);
>  
>  	return 0;
>  }
> @@ -1120,8 +1130,8 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer
>  	 * handler and the IPA.
>  	 */
>  	for (auto const &it : buffers) {
> -		ipaBuffers.push_back({ .id = mask | it.first,
> -				       .planes = it.second->planes() });
> +		ipaBuffers.push_back(IPABuffer(mask | it.first,
> +					       it.second->planes()));
>  		data->ipaBuffers_.insert(mask | it.first);
>  	}
>  
> @@ -1152,15 +1162,18 @@ void RPiCameraData::frameStarted(uint32_t sequence)
>  
>  int RPiCameraData::loadIPA()
>  {
> -	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> +	ipa_ = IPAManager::createIPA<ipa::rpi::IPAProxyRPi>(pipe_, 1, 1);
> +
>  	if (!ipa_)
>  		return -ENOENT;
>  
> -	ipa_->queueFrameAction.connect(this, &RPiCameraData::queueFrameAction);
> +	ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
> +	ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> +	ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> +	ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> +	ipa_->setStaggered.connect(this, &RPiCameraData::setStaggered);
>  
> -	IPASettings settings{
> -		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> -	};
> +	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
>  
>  	return ipa_->init(settings);
>  }
> @@ -1172,8 +1185,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		static_cast<const RPiCameraConfiguration *>(config);
>  
>  	std::map<unsigned int, IPAStream> streamConfig;
> -	std::map<unsigned int, const ControlInfoMap &> entityControls;
> -	IPAOperationData ipaConfig = {};
> +	std::map<unsigned int, ControlInfoMap> entityControls;

It's not nice to have to make a copy of the ControlInfoMap just to pass
this to the serializer :-5 Could you add a \todo in an appropriate place
(I suppose int he patch to the IPC infrastructure that introduces this
need, not in the RPi pipeline handler) ?

> +	ipa::rpi::ConfigInput ipaConfig;
>  
>  	/* Get the device format to pass to the IPA. */
>  	V4L2DeviceFormat sensorFormat;
> @@ -1182,10 +1195,9 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	unsigned int i = 0;
>  	for (auto const &stream : isp_) {
>  		if (stream.isExternal()) {
> -			streamConfig[i++] = {
> -				.pixelFormat = stream.configuration().pixelFormat,
> -				.size = stream.configuration().size
> -			};
> +			streamConfig[i++] = IPAStream(
> +				stream.configuration().pixelFormat,
> +				stream.configuration().size);
>  		}
>  	}
>  
> @@ -1193,17 +1205,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
>  
>  	/* Always send the user transform to the IPA. */
> -	ipaConfig.data = { static_cast<unsigned int>(config->transform) };
> +	ipaConfig.transform = static_cast<unsigned int>(config->transform);
>  
>  	/* Allocate the lens shading table via dmaHeap and pass to the IPA. */
>  	if (!lsTable_.isValid()) {
> -		lsTable_ = dmaHeap_.alloc("ls_grid", RPi::MaxLsGridSize);
> +		lsTable_ = dmaHeap_.alloc("ls_grid", ipa::rpi::MaxLsGridSize);
>  		if (!lsTable_.isValid())
>  			return -ENOMEM;
>  
>  		/* Allow the IPA to mmap the LS table via the file descriptor. */

It just occurred to me that this seems to duplicate the mapBuffers()
infrastructure. Could you add a

		/*
		 * \todo Investigate if mapping the lens shading table buffer
		 * could be handled with mapBuffers().
		 */

> -		ipaConfig.operation = RPi::IPA_CONFIG_LS_TABLE;
> -		ipaConfig.data.push_back(static_cast<unsigned int>(lsTable_.fd()));
> +		ipaConfig.op |= ipa::rpi::ConfigLsTable;
> +		ipaConfig.lsTableHandle = lsTable_;
>  	}
>  
>  	/* We store the CameraSensorInfo for digital zoom calculations. */
> @@ -1214,32 +1226,33 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	}
>  
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData result = {};
> +	ipa::rpi::ConfigOutput result = {};

No need for = {} here either. There could be other places below.

>  
>  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
>  
> -	if (result.operation & RPi::IPA_CONFIG_FAILED) {
> +	if (result.op & ipa::rpi::ConfigFailed) {
>  		LOG(RPI, Error) << "IPA configuration failed!";
>  		return -EPIPE;
>  	}
>  
> -	unsigned int resultIdx = 0;
> -	if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> +	if (result.op & ipa::rpi::ConfigStaggeredWrite) {
>  		/*
>  		 * Setup our staggered control writer with the sensor default
>  		 * gain and exposure delays.
>  		 */
>  		if (!staggeredCtrl_) {
>  			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> -					    { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> -					      { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> -			sensorMetadata_ = result.data[resultIdx++];
> +					    { { V4L2_CID_ANALOGUE_GAIN,
> +						result.sensorConfig.gainDelay },
> +					      { V4L2_CID_EXPOSURE,
> +						result.sensorConfig.exposureDelay } });
> +			sensorMetadata_ = result.sensorConfig.sensorMetadata;
>  		}
>  	}
>  
> -	if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> -		const ControlList &ctrls = result.controls[0];
> +	if (result.op & ipa::rpi::ConfigSensor) {

Maybe ipa::rpi::ConfigSensor could be dropped, replaced with a
!result.controls.empty() check ?

> +		const ControlList &ctrls = result.controls;
>  		if (!staggeredCtrl_.set(ctrls))
>  			LOG(RPI, Error) << "V4L2 staggered set failed";
>  	}
> @@ -1260,90 +1273,87 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	return 0;
>  }
>  
> -void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,
> -				     const IPAOperationData &action)
> +void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
>  {
> +	if (state_ == State::Stopped)
> +		handleState();
> +
> +	FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> +
> +	handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> +
> +	/* Fill the Request metadata buffer with what the IPA has provided */
> +	Request *request = requestQueue_.front();
> +	request->metadata() = std::move(controls);

That's interesting, controls being a const reference, I wouldn't have
expected a move to be possible.

> +
>  	/*
> -	 * The following actions can be handled when the pipeline handler is in
> -	 * a stopped state.
> +	 * Also update the ScalerCrop in the metadata with what we actually
> +	 * used. But we must first rescale that from ISP (camera mode) pixels
> +	 * back into sensor native pixels.
> +	 *
> +	 * Sending this information on every frame may be helpful.
>  	 */
> -	switch (action.operation) {
> -	case RPi::IPA_ACTION_V4L2_SET_STAGGERED: {
> -		const ControlList &controls = action.controls[0];
> -		if (!staggeredCtrl_.set(controls))
> -			LOG(RPI, Error) << "V4L2 staggered set failed";
> -		goto done;
> +	if (updateScalerCrop_) {
> +		updateScalerCrop_ = false;
> +		scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> +						sensorInfo_.outputSize);
> +		scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
>  	}
> +	request->metadata().set(controls::ScalerCrop, scalerCrop_);
>  
> -	case RPi::IPA_ACTION_V4L2_SET_ISP: {
> -		ControlList controls = action.controls[0];
> -		isp_[Isp::Input].dev()->setControls(&controls);
> -		goto done;
> -	}
> -	}
> +	state_ = State::IpaComplete;
>  
> -	if (state_ == State::Stopped)
> -		goto done;
> +	handleState();
> +}
>  
> -	/*
> -	 * The following actions must not be handled when the pipeline handler
> -	 * is in a stopped state.
> -	 */
> -	switch (action.operation) {
> -	case RPi::IPA_ACTION_STATS_METADATA_COMPLETE: {
> -		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> +void RPiCameraData::runIsp(uint32_t bufferId)
> +{
> +	if (state_ == State::Stopped)
> +		handleState();
>  
> -		handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> +	FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>  
> -		/* Fill the Request metadata buffer with what the IPA has provided */
> -		Request *request = requestQueue_.front();
> -		request->metadata() = std::move(action.controls[0]);
> +	LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> +			<< ", timestamp: " << buffer->metadata().timestamp;
>  
> -		/*
> -		 * Also update the ScalerCrop in the metadata with what we actually
> -		 * used. But we must first rescale that from ISP (camera mode) pixels
> -		 * back into sensor native pixels.
> -		 *
> -		 * Sending this information on every frame may be helpful.
> -		 */
> -		if (updateScalerCrop_) {
> -			updateScalerCrop_ = false;
> -			scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> -							sensorInfo_.outputSize);
> -			scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
> -		}
> -		request->metadata().set(controls::ScalerCrop, scalerCrop_);
> +	isp_[Isp::Input].queueBuffer(buffer);
> +	ispOutputCount_ = 0;
> +	handleState();
> +}
>  
> -		state_ = State::IpaComplete;
> -		break;
> -	}
> +void RPiCameraData::embeddedComplete(uint32_t bufferId)
> +{
> +	if (state_ == State::Stopped)
> +		handleState();
>  
> -	case RPi::IPA_ACTION_EMBEDDED_COMPLETE: {
> -		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> -		handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> -		break;
> -	}
> +	FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> +	handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> +	handleState();
> +}
>  
> -	case RPi::IPA_ACTION_RUN_ISP: {
> -		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> +void RPiCameraData::setIsp(const ControlList &controls)
> +{
> +	ControlList ctrls = controls;
>  
> -		LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << bufferId
> -				<< ", timestamp: " << buffer->metadata().timestamp;
> +	Span<const uint8_t> s =
> +		ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> +	const bcm2835_isp_lens_shading *lsp =
> +		reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
> +	bcm2835_isp_lens_shading ls = *lsp;

	bcm2835_isp_lens_shading ls =
		*reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());

The double-copy is a bit of a shame, maybe we should extend the
ControlList and/or ControlValue API to make it possible to modify
controls in place. That's out of scope for this series though.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	ls.dmabuf = lsTable_.fd();
>  
> -		isp_[Isp::Input].queueBuffer(buffer);
> -		ispOutputCount_ = 0;
> -		break;
> -	}
> +	ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
> +					    sizeof(ls) });
> +	ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
>  
> -	default:
> -		LOG(RPI, Error) << "Unknown action " << action.operation;
> -		break;
> -	}
> +	isp_[Isp::Input].dev()->setControls(&ctrls);
> +	handleState();
> +}
>  
> -done:
> +void RPiCameraData::setStaggered(const ControlList &controls)
> +{
> +	if (!staggeredCtrl_.set(controls))
> +		LOG(RPI, Error) << "V4L2 staggered set failed";
>  	handleState();
>  }
>  
> @@ -1445,10 +1455,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  	 * application until after the IPA signals so.
>  	 */
>  	if (stream == &isp_[Isp::Stats]) {
> -		IPAOperationData op;
> -		op.operation = RPi::IPA_EVENT_SIGNAL_STAT_READY;
> -		op.data = { RPi::BufferMask::STATS | static_cast<unsigned int>(index) };
> -		ipa_->processEvent(op);
> +		ipa_->signalStatReady(ipa::rpi::MaskStats | static_cast<unsigned int>(index));
>  	} else {
>  		/* Any other ISP output can be handed back to the application now. */
>  		handleStreamBuffer(buffer, stream);
> @@ -1552,7 +1559,7 @@ void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *strea
>  {
>  	unsigned int id = stream->getBufferId(buffer);
>  
> -	if (!(id & RPi::BufferMask::EXTERNAL_BUFFER))
> +	if (!(id & ipa::rpi::MaskExternalBuffer))
>  		return;
>  
>  	/* Stop the Stream object from tracking the buffer. */
> @@ -1652,7 +1659,6 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)
>  void RPiCameraData::tryRunPipeline()
>  {
>  	FrameBuffer *bayerBuffer, *embeddedBuffer;
> -	IPAOperationData op;
>  
>  	/* If any of our request or buffer queues are empty, we cannot proceed. */
>  	if (state_ != State::Idle || requestQueue_.empty() ||
> @@ -1673,9 +1679,7 @@ void RPiCameraData::tryRunPipeline()
>  	 * queue the ISP output buffer listed in the request to start the HW
>  	 * pipeline.
>  	 */
> -	op.operation = RPi::IPA_EVENT_QUEUE_REQUEST;
> -	op.controls = { request->controls() };
> -	ipa_->processEvent(op);
> +	ipa_->signalQueueRequest(request->controls());
>  
>  	/* Set our state to say the pipeline is active. */
>  	state_ = State::Busy;
> @@ -1683,14 +1687,14 @@ void RPiCameraData::tryRunPipeline()
>  	unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);
>  	unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>  
> -	LOG(RPI, Debug) << "Signalling RPi::IPA_EVENT_SIGNAL_ISP_PREPARE:"
> +	LOG(RPI, Debug) << "Signalling signalIspPrepare:"
>  			<< " Bayer buffer id: " << bayerId
>  			<< " Embedded buffer id: " << embeddedId;
>  
> -	op.operation = RPi::IPA_EVENT_SIGNAL_ISP_PREPARE;
> -	op.data = { RPi::BufferMask::EMBEDDED_DATA | embeddedId,
> -		    RPi::BufferMask::BAYER_DATA | bayerId };
> -	ipa_->processEvent(op);
> +	ipa::rpi::ISPConfig ispPrepare;
> +	ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> +	ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> +	ipa_->signalIspPrepare(ispPrepare);
>  }
>  
>  bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 3a5dadab..496dd36f 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -6,6 +6,8 @@
>   */
>  #include "rpi_stream.h"
>  
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> +
>  #include "libcamera/internal/log.h"
>  
>  namespace libcamera {
> @@ -70,7 +72,7 @@ int Stream::getBufferId(FrameBuffer *buffer) const
>  
>  void Stream::setExternalBuffer(FrameBuffer *buffer)
>  {
> -	bufferMap_.emplace(RPi::BufferMask::EXTERNAL_BUFFER | id_.get(), buffer);
> +	bufferMap_.emplace(ipa::rpi::MaskExternalBuffer | id_.get(), buffer);
>  }
>  
>  void Stream::removeExternalBuffer(FrameBuffer *buffer)
> @@ -78,7 +80,7 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
>  	int id = getBufferId(buffer);
>  
>  	/* Ensure we have this buffer in the stream, and it is marked external. */
> -	ASSERT(id != -1 && (id & RPi::BufferMask::EXTERNAL_BUFFER));
> +	ASSERT(id != -1 && (id & ipa::rpi::MaskExternalBuffer));
>  	bufferMap_.erase(id);
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index 0b502f64..701110d0 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -13,6 +13,7 @@
>  #include <vector>
>  
>  #include <libcamera/ipa/raspberrypi.h>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
>  #include <libcamera/stream.h>
>  
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -31,13 +32,13 @@ class Stream : public libcamera::Stream
>  {
>  public:
>  	Stream()
> -		: id_(RPi::BufferMask::ID)
> +		: id_(ipa::rpi::MaskID)
>  	{
>  	}
>  
>  	Stream(const char *name, MediaEntity *dev, bool importOnly = false)
>  		: external_(false), importOnly_(importOnly), name_(name),
> -		  dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(RPi::BufferMask::ID)
> +		  dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(ipa::rpi::MaskID)
>  	{
>  	}
>