[libcamera-devel,v3,2/3] libcamera: ipa: Pass a set of controls and return results from ipa::start()

10562 diff mbox series

This change allows controls passed into PipelineHandler::start to be
forwarded onto IPAInterface::start(). We also add a return channel if the
pipeline handler must action any of these controls, e.g. setting the
analogue gain or shutter speed in the sensor device.

todo: The IPA interface wrapper needs a translation for passing
IPAOperationData into start() and configure()

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
 include/libcamera/ipa/ipa_interface.h              |  3 ++-
 src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
 src/ipa/raspberrypi/raspberrypi.cpp                |  5 +++--
 src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
 src/ipa/vimc/vimc.cpp                              |  6 ++++--
 src/libcamera/ipa_context_wrapper.cpp              |  5 +++--
 src/libcamera/ipa_interface.cpp                    |  7 +++++++
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  3 ++-
 src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
 src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
 src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
 test/ipa/ipa_interface_test.cpp                    |  3 ++-
 test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
 15 files changed, 50 insertions(+), 23 deletions(-)

Hi Naush,

On Fri, Dec 04, 2020 at 03:31:20PM +0000, Naushir Patuck wrote:
> This change allows controls passed into PipelineHandler::start to be
> forwarded onto IPAInterface::start(). We also add a return channel if the
> pipeline handler must action any of these controls, e.g. setting the
> analogue gain or shutter speed in the sensor device.
>
> todo: The IPA interface wrapper needs a translation for passing
> IPAOperationData into start() and configure()
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
>  include/libcamera/ipa/ipa_interface.h              |  3 ++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  5 +++--
>  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
>  src/ipa/vimc/vimc.cpp                              |  6 ++++--
>  src/libcamera/ipa_context_wrapper.cpp              |  5 +++--
>  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  3 ++-
>  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
>  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
>  test/ipa/ipa_interface_test.cpp                    |  3 ++-
>  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
>  15 files changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 8f767e84..ec27e48e 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -20,7 +20,8 @@ public:
>  	~IPAContextWrapper();
>
>  	int init(const IPASettings &settings) override;
> -	int start() override;
> +	int start(const IPAOperationData &data,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 322b7079..df12f9ce 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -153,7 +153,8 @@ public:
>  	virtual ~IPAInterface() = default;
>
>  	virtual int init(const IPASettings &settings) = 0;
> -	virtual int start() = 0;
> +	virtual int start(const IPAOperationData &data,
> +			  IPAOperationData *result) = 0;
>  	virtual void stop() = 0;
>
>  	virtual void configure(const CameraSensorInfo &sensorInfo,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index cee532e3..40628489 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
>  {
>  	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
>
> -	return ctx->ipa_->start();
> +	/* \todo Translate the data and result. */
> +	IPAOperationData data = {};
> +	return ctx->ipa_->start(data, nullptr);
>  }
>
>  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 29d48b1b..b8c0f955 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -78,13 +78,14 @@ public:
>  	}
>
>  	int init(const IPASettings &settings) override;
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	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 &data,
> +		       const IPAOperationData &ipaConfig,
>  		       IPAOperationData *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 07d7f1b2..39783abd 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -37,7 +37,8 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>
>  	void configure(const CameraSensorInfo &info,
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cf841135..074902ee 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -34,7 +34,8 @@ public:
>
>  	int init(const IPASettings &settings) override;
>
> -	int start() override;
> +	int start(const IPAOperationData &data,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>
>  	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
>  	return 0;
>  }
>
> -int IPAVimc::start()
> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &data,
> +		   [[maybe_unused]] IPAOperationData *result)
>  {
>  	trace(IPAOperationStart);
>
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 231300ce..19c44ad8 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -86,10 +86,11 @@ int IPAContextWrapper::init(const IPASettings &settings)
>  	return 0;
>  }
>
> -int IPAContextWrapper::start()
> +int IPAContextWrapper::start(const IPAOperationData &data,
> +			     IPAOperationData *result)
>  {
>  	if (intf_)
> -		return intf_->start();
> +		return intf_->start(data, result);
>
>  	if (!ctx_)
>  		return 0;
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 23fc56d7..5be6f787 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -536,10 +536,17 @@ namespace libcamera {
>  /**
>   * \fn IPAInterface::start()
>   * \brief Start the IPA
> + * \param[in] data Protocol-specific data for the start operation
> + * \param[out] result Result of the start operation
>   *
>   * This method informs the IPA module that the camera is about to be started.
>   * The IPA module shall prepare any resources it needs to operate.
>   *
> + * The \a data and \a result parameters carry custom data passed by the
> + * pipeline handler to the IPA and back. The pipeline handler may set the \a
> + * result parameter to null if the IPA protocol doesn't need to pass a result
> + * back through the start() function.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9937db73..bafd0b2d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -752,7 +752,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  	}
>
>  	/* Start the IPA. */
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaData = {}, result = {};
> +	if (controls)
> +		ipaData.controls.emplace_back(*controls);
> +	ret = data->ipa_->start(ipaData, &result);
>  	if (ret) {
>  		LOG(RPI, Error)
>  			<< "Failed to start IPA for " << camera->id();
> @@ -1189,7 +1192,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	}
>
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData result;
> +	IPAOperationData result = {};
>
>  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4e959fde..eaa10f9f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -842,7 +842,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>  	if (ret)
>  		return ret;
>
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaData = {};
> +	ret = data->ipa_->start(ipaData, nullptr);
>  	if (ret) {
>  		freeBuffers(camera);
>  		LOG(RkISP1, Error)
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index d81b8598..2a5054a8 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con
>  	if (ret < 0)
>  		return ret;
>
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaData = {};
> +	ret = data->ipa_->start(ipaData, nullptr);
>  	if (ret) {
>  		data->video_->releaseBuffers();
>  		return ret;
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index b78a0e45..ea6f3e5e 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -30,7 +30,8 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>  	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
>  		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index eead2883..a5fda2c8 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -26,7 +26,8 @@ public:
>  	IPAProxyThread(IPAModule *ipam);
>
>  	int init(const IPASettings &settings) override;
> -	int start() override;
> +	int start(const IPAOperationData &data,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>
>  	void configure(const CameraSensorInfo &sensorInfo,
> @@ -50,9 +51,9 @@ private:
>  			ipa_ = ipa;
>  		}
>
> -		int start()
> +		int start(const IPAOperationData &data, IPAOperationData *result)
>  		{
> -			return ipa_->start();
> +			return ipa_->start(data, result);
>  		}
>
>  		void stop()
> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)
>  	return 0;
>  }
>
> -int IPAProxyThread::start()
> +int IPAProxyThread::start(const IPAOperationData &data,
> +			  IPAOperationData *result)
>  {
>  	running_ = true;
>  	thread_.start();
>
> -	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> +	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,
> +				   data, result);
>  }
>
>  void IPAProxyThread::stop()
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 9f575f93..dbb6727f 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -120,7 +120,8 @@ protected:
>  		}
>
>  		/* Test start of IPA module. */
> -		ipa_->start();
> +		IPAOperationData data = {};
> +		ipa_->start(data, nullptr);
>  		timer.start(1000);
>  		while (timer.isRunning() && trace_ != IPAOperationStart)
>  			dispatcher->processEvents();
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cb..47533d10 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -56,7 +56,8 @@ public:
>  		return 0;
>  	}
>
> -	int start() override
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override
>  	{
>  		report(Op_start, TestPass);
>  		return 0;
> @@ -376,7 +377,7 @@ protected:
>  			return TestFail;
>  		}
>
> -		ret = INVOKE(start);
> +		ret = INVOKE(start, ipaConfig, nullptr);
>  		if (ret == TestFail) {
>  			cerr << "Failed to run start()";
>  			return TestFail;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Naush,

Thank you for the patch.

On Fri, Dec 04, 2020 at 03:31:20PM +0000, Naushir Patuck wrote:
> This change allows controls passed into PipelineHandler::start to be
> forwarded onto IPAInterface::start(). We also add a return channel if the
> pipeline handler must action any of these controls, e.g. setting the
> analogue gain or shutter speed in the sensor device.
> 
> todo: The IPA interface wrapper needs a translation for passing
> IPAOperationData into start() and configure()

I'd replace this with

"The IPA interface wrapper isn't addressed as it will soon be replaced
by a new mechanism to handle IPC."

as it's not really something we need to do.

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
>  include/libcamera/ipa/ipa_interface.h              |  3 ++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  5 +++--
>  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
>  src/ipa/vimc/vimc.cpp                              |  6 ++++--
>  src/libcamera/ipa_context_wrapper.cpp              |  5 +++--
>  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  7 +++++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  3 ++-
>  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
>  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
>  test/ipa/ipa_interface_test.cpp                    |  3 ++-
>  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
>  15 files changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 8f767e84..ec27e48e 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -20,7 +20,8 @@ public:
>  	~IPAContextWrapper();
>  
>  	int init(const IPASettings &settings) override;
> -	int start() override;
> +	int start(const IPAOperationData &data,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 322b7079..df12f9ce 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -153,7 +153,8 @@ public:
>  	virtual ~IPAInterface() = default;
>  
>  	virtual int init(const IPASettings &settings) = 0;
> -	virtual int start() = 0;
> +	virtual int start(const IPAOperationData &data,
> +			  IPAOperationData *result) = 0;
>  	virtual void stop() = 0;
>  
>  	virtual void configure(const CameraSensorInfo &sensorInfo,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index cee532e3..40628489 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
>  {
>  	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
>  
> -	return ctx->ipa_->start();
> +	/* \todo Translate the data and result. */
> +	IPAOperationData data = {};
> +	return ctx->ipa_->start(data, nullptr);
>  }
>  
>  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 29d48b1b..b8c0f955 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -78,13 +78,14 @@ public:
>  	}
>  
>  	int init(const IPASettings &settings) override;
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	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 &data,
> +		       const IPAOperationData &ipaConfig,
>  		       IPAOperationData *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 07d7f1b2..39783abd 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -37,7 +37,8 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>  
>  	void configure(const CameraSensorInfo &info,
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cf841135..074902ee 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -34,7 +34,8 @@ public:
>  
>  	int init(const IPASettings &settings) override;
>  
> -	int start() override;
> +	int start(const IPAOperationData &data,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>  
>  	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
>  	return 0;
>  }
>  
> -int IPAVimc::start()
> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &data,
> +		   [[maybe_unused]] IPAOperationData *result)
>  {
>  	trace(IPAOperationStart);
>  
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 231300ce..19c44ad8 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -86,10 +86,11 @@ int IPAContextWrapper::init(const IPASettings &settings)
>  	return 0;
>  }
>  
> -int IPAContextWrapper::start()
> +int IPAContextWrapper::start(const IPAOperationData &data,
> +			     IPAOperationData *result)
>  {
>  	if (intf_)
> -		return intf_->start();
> +		return intf_->start(data, result);
>  
>  	if (!ctx_)
>  		return 0;
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 23fc56d7..5be6f787 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -536,10 +536,17 @@ namespace libcamera {
>  /**
>   * \fn IPAInterface::start()
>   * \brief Start the IPA
> + * \param[in] data Protocol-specific data for the start operation
> + * \param[out] result Result of the start operation
>   *
>   * This method informs the IPA module that the camera is about to be started.
>   * The IPA module shall prepare any resources it needs to operate.
>   *
> + * The \a data and \a result parameters carry custom data passed by the
> + * pipeline handler to the IPA and back. The pipeline handler may set the \a
> + * result parameter to null if the IPA protocol doesn't need to pass a result
> + * back through the start() function.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9937db73..bafd0b2d 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -752,7 +752,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  	}
>  
>  	/* Start the IPA. */
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaData = {}, result = {};

We usually split variable declarations to one per line.

I'd propose adding a default constructor to IPAOperationData to set
operation to 0 instead of forcing explicit initialization everywhere,
but IPAOperationData will soon go away.

> +	if (controls)
> +		ipaData.controls.emplace_back(*controls);
> +	ret = data->ipa_->start(ipaData, &result);
>  	if (ret) {
>  		LOG(RPI, Error)
>  			<< "Failed to start IPA for " << camera->id();
> @@ -1189,7 +1192,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	}
>  
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData result;
> +	IPAOperationData result = {};

Is this needed, or just a drive-by change ?

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

>  
>  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4e959fde..eaa10f9f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -842,7 +842,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>  	if (ret)
>  		return ret;
>  
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaData = {};
> +	ret = data->ipa_->start(ipaData, nullptr);
>  	if (ret) {
>  		freeBuffers(camera);
>  		LOG(RkISP1, Error)
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index d81b8598..2a5054a8 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaData = {};
> +	ret = data->ipa_->start(ipaData, nullptr);
>  	if (ret) {
>  		data->video_->releaseBuffers();
>  		return ret;
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index b78a0e45..ea6f3e5e 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -30,7 +30,8 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>  	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
>  		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index eead2883..a5fda2c8 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -26,7 +26,8 @@ public:
>  	IPAProxyThread(IPAModule *ipam);
>  
>  	int init(const IPASettings &settings) override;
> -	int start() override;
> +	int start(const IPAOperationData &data,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
> @@ -50,9 +51,9 @@ private:
>  			ipa_ = ipa;
>  		}
>  
> -		int start()
> +		int start(const IPAOperationData &data, IPAOperationData *result)
>  		{
> -			return ipa_->start();
> +			return ipa_->start(data, result);
>  		}
>  
>  		void stop()
> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)
>  	return 0;
>  }
>  
> -int IPAProxyThread::start()
> +int IPAProxyThread::start(const IPAOperationData &data,
> +			  IPAOperationData *result)
>  {
>  	running_ = true;
>  	thread_.start();
>  
> -	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> +	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,
> +				   data, result);
>  }
>  
>  void IPAProxyThread::stop()
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 9f575f93..dbb6727f 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -120,7 +120,8 @@ protected:
>  		}
>  
>  		/* Test start of IPA module. */
> -		ipa_->start();
> +		IPAOperationData data = {};
> +		ipa_->start(data, nullptr);
>  		timer.start(1000);
>  		while (timer.isRunning() && trace_ != IPAOperationStart)
>  			dispatcher->processEvents();
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cb..47533d10 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -56,7 +56,8 @@ public:
>  		return 0;
>  	}
>  
> -	int start() override
> +	int start([[maybe_unused]] const IPAOperationData &data,
> +		  [[maybe_unused]] IPAOperationData *result) override
>  	{
>  		report(Op_start, TestPass);
>  		return 0;
> @@ -376,7 +377,7 @@ protected:
>  			return TestFail;
>  		}
>  
> -		ret = INVOKE(start);
> +		ret = INVOKE(start, ipaConfig, nullptr);
>  		if (ret == TestFail) {
>  			cerr << "Failed to run start()";
>  			return TestFail;