[libcamera-devel,v1,3/9] libcamera: ipa_interface: Add support for custom IPA data to configure()
diff mbox series

Message ID 20200628231934.29025-4-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Support passing custom data to IPA configure()
Related show

Commit Message

Laurent Pinchart June 28, 2020, 11:19 p.m. UTC 
Add two new parameters, ipaConfig and result, to the
IPAInterface::configure() function to allow pipeline handlers to pass
custom data to their IPA, and receive data back. Wire this through the
code base. The C API interface will be addressed separately, likely
through automation of the C <-> C++ translation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
 include/libcamera/ipa/ipa_interface.h              |  4 +++-
 src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
 src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
 src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
 src/ipa/vimc/vimc.cpp                              |  4 +++-
 src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
 src/libcamera/ipa_interface.cpp                    |  7 +++++++
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
 src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
 src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
 src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
 test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
 13 files changed, 61 insertions(+), 18 deletions(-)

Comments

Niklas Söderlund June 29, 2020, 2:31 p.m. UTC | #1 
Hi Laurent,

Thanks for your work.

On 2020-06-29 02:19:28 +0300, Laurent Pinchart wrote:
> Add two new parameters, ipaConfig and result, to the
> IPAInterface::configure() function to allow pipeline handlers to pass
> custom data to their IPA, and receive data back. Wire this through the
> code base. The C API interface will be addressed separately, likely
> through automation of the C <-> C++ translation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
>  include/libcamera/ipa/ipa_interface.h              |  4 +++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
>  src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
>  src/ipa/vimc/vimc.cpp                              |  4 +++-
>  src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
>  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
>  src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
>  test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
>  13 files changed, 61 insertions(+), 18 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 4e6f791d18e6..8f767e844221 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -24,7 +24,9 @@ public:
>  	void stop() override;
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override;
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index dc9fc714d564..5016ec25ea9c 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -158,7 +158,9 @@ public:
>  
>  	virtual void configure(const CameraSensorInfo &sensorInfo,
>  			       const std::map<unsigned int, IPAStream> &streamConfig,
> -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			       const IPAOperationData &ipaConfig,
> +			       IPAOperationData *result) = 0;
>  
>  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
>  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index 2a2e43abc708..47ce5a704851 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>  		entityControls.emplace(id, infoMaps[id]);
>  	}
>  
> -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> +	/* \todo Translate the ipaConfig and reponse  */

I wonder if we should add a FATAL LOG statement here to make it clear 
that we break the wrapper and this issue should be solved before anyone 
attempts to use the feature, which we know won't work.

> +	IPAOperationData ipaConfig;
> +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> +			     nullptr);
>  }
>  
>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index bc89ab58d03a..860be22ddb5d 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -78,7 +78,9 @@ public:
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &data,
> +		       IPAOperationData *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;
> @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  
>  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result)
>  {
>  	if (entityControls.empty())
>  		return;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index fbdc908fc816..34d6f63a7ff4 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -39,7 +39,9 @@ public:
>  
>  	void configure(const CameraSensorInfo &info,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *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;
> @@ -76,7 +78,9 @@ private:
>   */
>  void IPARkISP1::configure(const CameraSensorInfo &info,
>  			  const std::map<unsigned int, IPAStream> &streamConfig,
> -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			  const IPAOperationData &ipaConfig,
> +			  IPAOperationData *result)
>  {
>  	if (entityControls.empty())
>  		return;
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index af278a482b8a..1593c92d80f3 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -39,7 +39,9 @@ public:
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override {}
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
>  	void processEvent(const IPAOperationData &event) override {}
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 471118f57cdc..231300ce0bec 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
>  
>  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>  				  const std::map<unsigned int, IPAStream> &streamConfig,
> -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +				  const IPAOperationData &ipaConfig,
> +				  IPAOperationData *result)
>  {
>  	if (intf_)
> -		return intf_->configure(sensorInfo, streamConfig, entityControls);
> +		return intf_->configure(sensorInfo, streamConfig,
> +					entityControls, ipaConfig, result);
>  
>  	if (!ctx_)
>  		return;
> @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>  		++i;
>  	}
>  
> +	/* \todo Translate the ipaConfig and reponse */
>  	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
>  			     c_info_maps, entityControls.size());
>  }
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index ebe47e1233a5..23fc56d7d48e 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -557,6 +557,8 @@ namespace libcamera {
>   * \param[in] sensorInfo Camera sensor information
>   * \param[in] streamConfig Configuration of all active streams
>   * \param[in] entityControls Controls provided by the pipeline entities
> + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> + * \param[out] result Pipeline-handler-specific configuration result
>   *
>   * This method shall be called when the camera is started to inform the IPA of
>   * the camera's streams and the sensor settings. The meaning of the numerical
> @@ -566,6 +568,11 @@ namespace libcamera {
>   * The \a sensorInfo conveys information about the camera sensor settings that
>   * the pipeline handler has selected for the configuration. The IPA may use
>   * that information to tune its algorithms.
> + *
> + * The \a ipaConfig 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 configure() function.
>   */
>  
>  /**
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index dcd737a1d1a0..3b5cdf1e1849 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
>  	}
>  
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	IPAOperationData ipaConfig;
> +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> +			      ipaConfig, nullptr);
>  
>  	return 0;
>  }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3c01821135f8..4fde5539e667 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>  
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	IPAOperationData ipaConfig;
> +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> +			      ipaConfig, nullptr);
>  
>  	return ret;
>  }
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index be34f20aa857..68eafb307b2a 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -31,7 +31,9 @@ public:
>  	void stop() override {}
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override {}
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
>  	void processEvent(const IPAOperationData &event) override {}
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index 6fbebed2ba72..aa403e22f297 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -31,7 +31,9 @@ public:
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void processEvent(const IPAOperationData &event) override;
> @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
>  
>  void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>  			       const std::map<unsigned int, IPAStream> &streamConfig,
> -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			       const IPAOperationData &ipaConfig,
> +			       IPAOperationData *result)
>  {
> -	ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> +			result);
>  }
>  
>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index aa7a9dcc6050..23c799da0663 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -69,7 +69,9 @@ public:
>  
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override
>  	{
>  		/* Verify sensorInfo. */
>  		if (sensorInfo.outputSize.width != 2560 ||
> @@ -317,7 +319,9 @@ protected:
>  		};
>  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
>  		controlInfo.emplace(42, subdev_->controls());
> -		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> +		IPAOperationData ipaConfig;
> +		ret = INVOKE(configure, sensorInfo, config, controlInfo,
> +			     ipaConfig, nullptr);
>  		if (ret == TestFail)
>  			return TestFail;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 29, 2020, 2:57 p.m. UTC | #2 
Hi Niklas,

On Mon, Jun 29, 2020 at 04:31:13PM +0200, Niklas Söderlund wrote:
> On 2020-06-29 02:19:28 +0300, Laurent Pinchart wrote:
> > Add two new parameters, ipaConfig and result, to the
> > IPAInterface::configure() function to allow pipeline handlers to pass
> > custom data to their IPA, and receive data back. Wire this through the
> > code base. The C API interface will be addressed separately, likely
> > through automation of the C <-> C++ translation.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
> >  include/libcamera/ipa/ipa_interface.h              |  4 +++-
> >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
> >  src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
> >  src/ipa/vimc/vimc.cpp                              |  4 +++-
> >  src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
> >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
> >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
> >  test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
> >  13 files changed, 61 insertions(+), 18 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> > index 4e6f791d18e6..8f767e844221 100644
> > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > @@ -24,7 +24,9 @@ public:
> >  	void stop() override;
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override;
> >  
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > index dc9fc714d564..5016ec25ea9c 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -158,7 +158,9 @@ public:
> >  
> >  	virtual void configure(const CameraSensorInfo &sensorInfo,
> >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +			       const IPAOperationData &ipaConfig,
> > +			       IPAOperationData *result) = 0;
> >  
> >  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> >  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index 2a2e43abc708..47ce5a704851 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> >  		entityControls.emplace(id, infoMaps[id]);
> >  	}
> >  
> > -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > +	/* \todo Translate the ipaConfig and reponse  */
> 
> I wonder if we should add a FATAL LOG statement here to make it clear 
> that we break the wrapper and this issue should be solved before anyone 
> attempts to use the feature, which we know won't work.

That would break existing tests though, as they use this function.

> > +	IPAOperationData ipaConfig;
> > +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> > +			     nullptr);
> >  }
> >  
> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index bc89ab58d03a..860be22ddb5d 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -78,7 +78,9 @@ public:
> >  
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &data,
> > +		       IPAOperationData *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;
> > @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> >  
> >  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result)
> >  {
> >  	if (entityControls.empty())
> >  		return;
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index fbdc908fc816..34d6f63a7ff4 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -39,7 +39,9 @@ public:
> >  
> >  	void configure(const CameraSensorInfo &info,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *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;
> > @@ -76,7 +78,9 @@ private:
> >   */
> >  void IPARkISP1::configure(const CameraSensorInfo &info,
> >  			  const std::map<unsigned int, IPAStream> &streamConfig,
> > -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +			  const IPAOperationData &ipaConfig,
> > +			  IPAOperationData *result)
> >  {
> >  	if (entityControls.empty())
> >  		return;
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index af278a482b8a..1593c92d80f3 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -39,7 +39,9 @@ public:
> >  
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override {}
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> >  	void processEvent(const IPAOperationData &event) override {}
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index 471118f57cdc..231300ce0bec 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
> >  
> >  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> >  				  const std::map<unsigned int, IPAStream> &streamConfig,
> > -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +				  const IPAOperationData &ipaConfig,
> > +				  IPAOperationData *result)
> >  {
> >  	if (intf_)
> > -		return intf_->configure(sensorInfo, streamConfig, entityControls);
> > +		return intf_->configure(sensorInfo, streamConfig,
> > +					entityControls, ipaConfig, result);
> >  
> >  	if (!ctx_)
> >  		return;
> > @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> >  		++i;
> >  	}
> >  
> > +	/* \todo Translate the ipaConfig and reponse */
> >  	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> >  			     c_info_maps, entityControls.size());
> >  }
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index ebe47e1233a5..23fc56d7d48e 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -557,6 +557,8 @@ namespace libcamera {
> >   * \param[in] sensorInfo Camera sensor information
> >   * \param[in] streamConfig Configuration of all active streams
> >   * \param[in] entityControls Controls provided by the pipeline entities
> > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > + * \param[out] result Pipeline-handler-specific configuration result
> >   *
> >   * This method shall be called when the camera is started to inform the IPA of
> >   * the camera's streams and the sensor settings. The meaning of the numerical
> > @@ -566,6 +568,11 @@ namespace libcamera {
> >   * The \a sensorInfo conveys information about the camera sensor settings that
> >   * the pipeline handler has selected for the configuration. The IPA may use
> >   * that information to tune its algorithms.
> > + *
> > + * The \a ipaConfig 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 configure() function.
> >   */
> >  
> >  /**
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index dcd737a1d1a0..3b5cdf1e1849 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
> >  	}
> >  
> >  	/* Ready the IPA - it must know about the sensor resolution. */
> > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > +	IPAOperationData ipaConfig;
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +			      ipaConfig, nullptr);
> >  
> >  	return 0;
> >  }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 3c01821135f8..4fde5539e667 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> >  	entityControls.emplace(0, data->sensor_->controls());
> >  
> > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > +	IPAOperationData ipaConfig;
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +			      ipaConfig, nullptr);
> >  
> >  	return ret;
> >  }
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index be34f20aa857..68eafb307b2a 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -31,7 +31,9 @@ public:
> >  	void stop() override {}
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override {}
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> >  	void processEvent(const IPAOperationData &event) override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index 6fbebed2ba72..aa403e22f297 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -31,7 +31,9 @@ public:
> >  
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override;
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >  	void processEvent(const IPAOperationData &event) override;
> > @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
> >  
> >  void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +			       const IPAOperationData &ipaConfig,
> > +			       IPAOperationData *result)
> >  {
> > -	ipa_->configure(sensorInfo, streamConfig, entityControls);
> > +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > +			result);
> >  }
> >  
> >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > index aa7a9dcc6050..23c799da0663 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -69,7 +69,9 @@ public:
> >  
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override
> >  	{
> >  		/* Verify sensorInfo. */
> >  		if (sensorInfo.outputSize.width != 2560 ||
> > @@ -317,7 +319,9 @@ protected:
> >  		};
> >  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
> >  		controlInfo.emplace(42, subdev_->controls());
> > -		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > +		IPAOperationData ipaConfig;
> > +		ret = INVOKE(configure, sensorInfo, config, controlInfo,
> > +			     ipaConfig, nullptr);
> >  		if (ret == TestFail)
> >  			return TestFail;
> >
Niklas Söderlund June 29, 2020, 4:25 p.m. UTC | #3 
Hi Laurent,

On 2020-06-29 17:57:43 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Jun 29, 2020 at 04:31:13PM +0200, Niklas Söderlund wrote:
> > On 2020-06-29 02:19:28 +0300, Laurent Pinchart wrote:
> > > Add two new parameters, ipaConfig and result, to the
> > > IPAInterface::configure() function to allow pipeline handlers to pass
> > > custom data to their IPA, and receive data back. Wire this through the
> > > code base. The C API interface will be addressed separately, likely
> > > through automation of the C <-> C++ translation.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
> > >  include/libcamera/ipa/ipa_interface.h              |  4 +++-
> > >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
> > >  src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
> > >  src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
> > >  src/ipa/vimc/vimc.cpp                              |  4 +++-
> > >  src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
> > >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
> > >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
> > >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
> > >  test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
> > >  13 files changed, 61 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> > > index 4e6f791d18e6..8f767e844221 100644
> > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > @@ -24,7 +24,9 @@ public:
> > >  	void stop() override;
> > >  	void configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *result) override;
> > >  
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > > index dc9fc714d564..5016ec25ea9c 100644
> > > --- a/include/libcamera/ipa/ipa_interface.h
> > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > @@ -158,7 +158,9 @@ public:
> > >  
> > >  	virtual void configure(const CameraSensorInfo &sensorInfo,
> > >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> > > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +			       const IPAOperationData &ipaConfig,
> > > +			       IPAOperationData *result) = 0;
> > >  
> > >  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > >  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > index 2a2e43abc708..47ce5a704851 100644
> > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > >  		entityControls.emplace(id, infoMaps[id]);
> > >  	}
> > >  
> > > -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > > +	/* \todo Translate the ipaConfig and reponse  */
> > 
> > I wonder if we should add a FATAL LOG statement here to make it clear 
> > that we break the wrapper and this issue should be solved before anyone 
> > attempts to use the feature, which we know won't work.
> 
> That would break existing tests though, as they use this function.

Since all this is going away I wont stress it :-)

> 
> > > +	IPAOperationData ipaConfig;
> > > +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> > > +			     nullptr);
> > >  }
> > >  
> > >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index bc89ab58d03a..860be22ddb5d 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -78,7 +78,9 @@ public:
> > >  
> > >  	void configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &data,
> > > +		       IPAOperationData *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;
> > > @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > >  
> > >  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *result)
> > >  {
> > >  	if (entityControls.empty())
> > >  		return;
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index fbdc908fc816..34d6f63a7ff4 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -39,7 +39,9 @@ public:
> > >  
> > >  	void configure(const CameraSensorInfo &info,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *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;
> > > @@ -76,7 +78,9 @@ private:
> > >   */
> > >  void IPARkISP1::configure(const CameraSensorInfo &info,
> > >  			  const std::map<unsigned int, IPAStream> &streamConfig,
> > > -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > +			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +			  const IPAOperationData &ipaConfig,
> > > +			  IPAOperationData *result)
> > >  {
> > >  	if (entityControls.empty())
> > >  		return;
> > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > index af278a482b8a..1593c92d80f3 100644
> > > --- a/src/ipa/vimc/vimc.cpp
> > > +++ b/src/ipa/vimc/vimc.cpp
> > > @@ -39,7 +39,9 @@ public:
> > >  
> > >  	void configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *result) override {}
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > >  	void processEvent(const IPAOperationData &event) override {}
> > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > > index 471118f57cdc..231300ce0bec 100644
> > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
> > >  
> > >  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > >  				  const std::map<unsigned int, IPAStream> &streamConfig,
> > > -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +				  const IPAOperationData &ipaConfig,
> > > +				  IPAOperationData *result)
> > >  {
> > >  	if (intf_)
> > > -		return intf_->configure(sensorInfo, streamConfig, entityControls);
> > > +		return intf_->configure(sensorInfo, streamConfig,
> > > +					entityControls, ipaConfig, result);
> > >  
> > >  	if (!ctx_)
> > >  		return;
> > > @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > >  		++i;
> > >  	}
> > >  
> > > +	/* \todo Translate the ipaConfig and reponse */
> > >  	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> > >  			     c_info_maps, entityControls.size());
> > >  }
> > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > > index ebe47e1233a5..23fc56d7d48e 100644
> > > --- a/src/libcamera/ipa_interface.cpp
> > > +++ b/src/libcamera/ipa_interface.cpp
> > > @@ -557,6 +557,8 @@ namespace libcamera {
> > >   * \param[in] sensorInfo Camera sensor information
> > >   * \param[in] streamConfig Configuration of all active streams
> > >   * \param[in] entityControls Controls provided by the pipeline entities
> > > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > + * \param[out] result Pipeline-handler-specific configuration result
> > >   *
> > >   * This method shall be called when the camera is started to inform the IPA of
> > >   * the camera's streams and the sensor settings. The meaning of the numerical
> > > @@ -566,6 +568,11 @@ namespace libcamera {
> > >   * The \a sensorInfo conveys information about the camera sensor settings that
> > >   * the pipeline handler has selected for the configuration. The IPA may use
> > >   * that information to tune its algorithms.
> > > + *
> > > + * The \a ipaConfig 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 configure() function.
> > >   */
> > >  
> > >  /**
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index dcd737a1d1a0..3b5cdf1e1849 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
> > >  	}
> > >  
> > >  	/* Ready the IPA - it must know about the sensor resolution. */
> > > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > +	IPAOperationData ipaConfig;
> > > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > +			      ipaConfig, nullptr);
> > >  
> > >  	return 0;
> > >  }
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 3c01821135f8..4fde5539e667 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > >  	entityControls.emplace(0, data->sensor_->controls());
> > >  
> > > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > +	IPAOperationData ipaConfig;
> > > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > +			      ipaConfig, nullptr);
> > >  
> > >  	return ret;
> > >  }
> > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > index be34f20aa857..68eafb307b2a 100644
> > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > @@ -31,7 +31,9 @@ public:
> > >  	void stop() override {}
> > >  	void configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *result) override {}
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > >  	void processEvent(const IPAOperationData &event) override {}
> > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > index 6fbebed2ba72..aa403e22f297 100644
> > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > @@ -31,7 +31,9 @@ public:
> > >  
> > >  	void configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *result) override;
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > >  	void processEvent(const IPAOperationData &event) override;
> > > @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
> > >  
> > >  void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +			       const IPAOperationData &ipaConfig,
> > > +			       IPAOperationData *result)
> > >  {
> > > -	ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > > +			result);
> > >  }
> > >  
> > >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > > index aa7a9dcc6050..23c799da0663 100644
> > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > @@ -69,7 +69,9 @@ public:
> > >  
> > >  	void configure(const CameraSensorInfo &sensorInfo,
> > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > +		       const IPAOperationData &ipaConfig,
> > > +		       IPAOperationData *result) override
> > >  	{
> > >  		/* Verify sensorInfo. */
> > >  		if (sensorInfo.outputSize.width != 2560 ||
> > > @@ -317,7 +319,9 @@ protected:
> > >  		};
> > >  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
> > >  		controlInfo.emplace(42, subdev_->controls());
> > > -		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > > +		IPAOperationData ipaConfig;
> > > +		ret = INVOKE(configure, sensorInfo, config, controlInfo,
> > > +			     ipaConfig, nullptr);
> > >  		if (ret == TestFail)
> > >  			return TestFail;
> > >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Niklas Söderlund June 29, 2020, 8:51 p.m. UTC | #4 
Hi Laurent,

On 2020-06-29 18:25:38 +0200, Niklas Söderlund wrote:
> Hi Laurent,
> 
> On 2020-06-29 17:57:43 +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> > 
> > On Mon, Jun 29, 2020 at 04:31:13PM +0200, Niklas Söderlund wrote:
> > > On 2020-06-29 02:19:28 +0300, Laurent Pinchart wrote:
> > > > Add two new parameters, ipaConfig and result, to the
> > > > IPAInterface::configure() function to allow pipeline handlers to pass
> > > > custom data to their IPA, and receive data back. Wire this through the
> > > > code base. The C API interface will be addressed separately, likely
> > > > through automation of the C <-> C++ translation.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
> > > >  include/libcamera/ipa/ipa_interface.h              |  4 +++-
> > > >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
> > > >  src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
> > > >  src/ipa/vimc/vimc.cpp                              |  4 +++-
> > > >  src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
> > > >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
> > > >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
> > > >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
> > > >  test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
> > > >  13 files changed, 61 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> > > > index 4e6f791d18e6..8f767e844221 100644
> > > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > > @@ -24,7 +24,9 @@ public:
> > > >  	void stop() override;
> > > >  	void configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *result) override;
> > > >  
> > > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > > > index dc9fc714d564..5016ec25ea9c 100644
> > > > --- a/include/libcamera/ipa/ipa_interface.h
> > > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > > @@ -158,7 +158,9 @@ public:
> > > >  
> > > >  	virtual void configure(const CameraSensorInfo &sensorInfo,
> > > >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> > > > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +			       const IPAOperationData &ipaConfig,
> > > > +			       IPAOperationData *result) = 0;
> > > >  
> > > >  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > > >  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > index 2a2e43abc708..47ce5a704851 100644
> > > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > > >  		entityControls.emplace(id, infoMaps[id]);
> > > >  	}
> > > >  
> > > > -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > > > +	/* \todo Translate the ipaConfig and reponse  */
> > > 
> > > I wonder if we should add a FATAL LOG statement here to make it clear 
> > > that we break the wrapper and this issue should be solved before anyone 
> > > attempts to use the feature, which we know won't work.
> > 
> > That would break existing tests though, as they use this function.
> 
> Since all this is going away I wont stress it :-)

I forgot to add,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> > 
> > > > +	IPAOperationData ipaConfig;
> > > > +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> > > > +			     nullptr);
> > > >  }
> > > >  
> > > >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index bc89ab58d03a..860be22ddb5d 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -78,7 +78,9 @@ public:
> > > >  
> > > >  	void configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &data,
> > > > +		       IPAOperationData *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;
> > > > @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > > >  
> > > >  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *result)
> > > >  {
> > > >  	if (entityControls.empty())
> > > >  		return;
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index fbdc908fc816..34d6f63a7ff4 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -39,7 +39,9 @@ public:
> > > >  
> > > >  	void configure(const CameraSensorInfo &info,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *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;
> > > > @@ -76,7 +78,9 @@ private:
> > > >   */
> > > >  void IPARkISP1::configure(const CameraSensorInfo &info,
> > > >  			  const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > +			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +			  const IPAOperationData &ipaConfig,
> > > > +			  IPAOperationData *result)
> > > >  {
> > > >  	if (entityControls.empty())
> > > >  		return;
> > > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > > index af278a482b8a..1593c92d80f3 100644
> > > > --- a/src/ipa/vimc/vimc.cpp
> > > > +++ b/src/ipa/vimc/vimc.cpp
> > > > @@ -39,7 +39,9 @@ public:
> > > >  
> > > >  	void configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *result) override {}
> > > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > > >  	void processEvent(const IPAOperationData &event) override {}
> > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > > > index 471118f57cdc..231300ce0bec 100644
> > > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > > @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
> > > >  
> > > >  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > > >  				  const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +				  const IPAOperationData &ipaConfig,
> > > > +				  IPAOperationData *result)
> > > >  {
> > > >  	if (intf_)
> > > > -		return intf_->configure(sensorInfo, streamConfig, entityControls);
> > > > +		return intf_->configure(sensorInfo, streamConfig,
> > > > +					entityControls, ipaConfig, result);
> > > >  
> > > >  	if (!ctx_)
> > > >  		return;
> > > > @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > > >  		++i;
> > > >  	}
> > > >  
> > > > +	/* \todo Translate the ipaConfig and reponse */
> > > >  	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> > > >  			     c_info_maps, entityControls.size());
> > > >  }
> > > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > > > index ebe47e1233a5..23fc56d7d48e 100644
> > > > --- a/src/libcamera/ipa_interface.cpp
> > > > +++ b/src/libcamera/ipa_interface.cpp
> > > > @@ -557,6 +557,8 @@ namespace libcamera {
> > > >   * \param[in] sensorInfo Camera sensor information
> > > >   * \param[in] streamConfig Configuration of all active streams
> > > >   * \param[in] entityControls Controls provided by the pipeline entities
> > > > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > + * \param[out] result Pipeline-handler-specific configuration result
> > > >   *
> > > >   * This method shall be called when the camera is started to inform the IPA of
> > > >   * the camera's streams and the sensor settings. The meaning of the numerical
> > > > @@ -566,6 +568,11 @@ namespace libcamera {
> > > >   * The \a sensorInfo conveys information about the camera sensor settings that
> > > >   * the pipeline handler has selected for the configuration. The IPA may use
> > > >   * that information to tune its algorithms.
> > > > + *
> > > > + * The \a ipaConfig 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 configure() function.
> > > >   */
> > > >  
> > > >  /**
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index dcd737a1d1a0..3b5cdf1e1849 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
> > > >  	}
> > > >  
> > > >  	/* Ready the IPA - it must know about the sensor resolution. */
> > > > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > > +	IPAOperationData ipaConfig;
> > > > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > > +			      ipaConfig, nullptr);
> > > >  
> > > >  	return 0;
> > > >  }
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 3c01821135f8..4fde5539e667 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > > >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > > >  	entityControls.emplace(0, data->sensor_->controls());
> > > >  
> > > > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > > +	IPAOperationData ipaConfig;
> > > > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > > +			      ipaConfig, nullptr);
> > > >  
> > > >  	return ret;
> > > >  }
> > > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > index be34f20aa857..68eafb307b2a 100644
> > > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > @@ -31,7 +31,9 @@ public:
> > > >  	void stop() override {}
> > > >  	void configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *result) override {}
> > > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > > >  	void processEvent(const IPAOperationData &event) override {}
> > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > index 6fbebed2ba72..aa403e22f297 100644
> > > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > @@ -31,7 +31,9 @@ public:
> > > >  
> > > >  	void configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *result) override;
> > > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > >  	void processEvent(const IPAOperationData &event) override;
> > > > @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
> > > >  
> > > >  void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > > >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +			       const IPAOperationData &ipaConfig,
> > > > +			       IPAOperationData *result)
> > > >  {
> > > > -	ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > > +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > > > +			result);
> > > >  }
> > > >  
> > > >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > > > index aa7a9dcc6050..23c799da0663 100644
> > > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > > @@ -69,7 +69,9 @@ public:
> > > >  
> > > >  	void configure(const CameraSensorInfo &sensorInfo,
> > > >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > > > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > > > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > +		       const IPAOperationData &ipaConfig,
> > > > +		       IPAOperationData *result) override
> > > >  	{
> > > >  		/* Verify sensorInfo. */
> > > >  		if (sensorInfo.outputSize.width != 2560 ||
> > > > @@ -317,7 +319,9 @@ protected:
> > > >  		};
> > > >  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
> > > >  		controlInfo.emplace(42, subdev_->controls());
> > > > -		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > > > +		IPAOperationData ipaConfig;
> > > > +		ret = INVOKE(configure, sensorInfo, config, controlInfo,
> > > > +			     ipaConfig, nullptr);
> > > >  		if (ret == TestFail)
> > > >  			return TestFail;
> > > >  
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> -- 
> Regards,
> Niklas Söderlund
Jacopo Mondi June 30, 2020, 10:34 a.m. UTC | #5 
Hi Laurent,

On Mon, Jun 29, 2020 at 02:19:28AM +0300, Laurent Pinchart wrote:
> Add two new parameters, ipaConfig and result, to the
> IPAInterface::configure() function to allow pipeline handlers to pass
> custom data to their IPA, and receive data back. Wire this through the
> code base. The C API interface will be addressed separately, likely
> through automation of the C <-> C++ translation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
>  include/libcamera/ipa/ipa_interface.h              |  4 +++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
>  src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
>  src/ipa/vimc/vimc.cpp                              |  4 +++-
>  src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
>  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
>  src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
>  test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
>  13 files changed, 61 insertions(+), 18 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 4e6f791d18e6..8f767e844221 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -24,7 +24,9 @@ public:
>  	void stop() override;
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override;

This, and all the other places where this is changed in the same way,
fit in one line if I'm not mistaken.

>
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index dc9fc714d564..5016ec25ea9c 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -158,7 +158,9 @@ public:
>
>  	virtual void configure(const CameraSensorInfo &sensorInfo,
>  			       const std::map<unsigned int, IPAStream> &streamConfig,
> -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			       const IPAOperationData &ipaConfig,
> +			       IPAOperationData *result) = 0;
>
>  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
>  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index 2a2e43abc708..47ce5a704851 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>  		entityControls.emplace(id, infoMaps[id]);
>  	}
>
> -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> +	/* \todo Translate the ipaConfig and reponse  */

if the usage of response in place of result is not intentional, please
fix. Also missing . at the end, not sure if should be used in todo
entries though.

> +	IPAOperationData ipaConfig;
> +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> +			     nullptr);
>  }
>
>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index bc89ab58d03a..860be22ddb5d 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -78,7 +78,9 @@ public:
>
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &data,
> +		       IPAOperationData *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;
> @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>
>  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result)
>  {
>  	if (entityControls.empty())
>  		return;
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index fbdc908fc816..34d6f63a7ff4 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -39,7 +39,9 @@ public:
>
>  	void configure(const CameraSensorInfo &info,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *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;
> @@ -76,7 +78,9 @@ private:
>   */
>  void IPARkISP1::configure(const CameraSensorInfo &info,
>  			  const std::map<unsigned int, IPAStream> &streamConfig,
> -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			  const IPAOperationData &ipaConfig,
> +			  IPAOperationData *result)
>  {
>  	if (entityControls.empty())
>  		return;
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index af278a482b8a..1593c92d80f3 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -39,7 +39,9 @@ public:
>
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override {}
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
>  	void processEvent(const IPAOperationData &event) override {}
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 471118f57cdc..231300ce0bec 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
>
>  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>  				  const std::map<unsigned int, IPAStream> &streamConfig,
> -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +				  const IPAOperationData &ipaConfig,
> +				  IPAOperationData *result)
>  {
>  	if (intf_)
> -		return intf_->configure(sensorInfo, streamConfig, entityControls);
> +		return intf_->configure(sensorInfo, streamConfig,
> +					entityControls, ipaConfig, result);
>
>  	if (!ctx_)
>  		return;
> @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>  		++i;
>  	}
>
> +	/* \todo Translate the ipaConfig and reponse */
>  	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
>  			     c_info_maps, entityControls.size());
>  }
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index ebe47e1233a5..23fc56d7d48e 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -557,6 +557,8 @@ namespace libcamera {
>   * \param[in] sensorInfo Camera sensor information
>   * \param[in] streamConfig Configuration of all active streams
>   * \param[in] entityControls Controls provided by the pipeline entities
> + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> + * \param[out] result Pipeline-handler-specific configuration result
>   *
>   * This method shall be called when the camera is started to inform the IPA of
>   * the camera's streams and the sensor settings. The meaning of the numerical
> @@ -566,6 +568,11 @@ namespace libcamera {
>   * The \a sensorInfo conveys information about the camera sensor settings that
>   * the pipeline handler has selected for the configuration. The IPA may use
>   * that information to tune its algorithms.
> + *
> + * The \a ipaConfig 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 configure() function.

I would like to have a bit more details here. Like in example, how
does this differ from entityControls ? The way controls are passed
there and associated to an arbitrary entity shouldn't (in the long
run, provided all required controls are defined) serve the same
purpose ? Equally, if 'result' aims to carry sensor configurations,
shouldn't it be a control list ?

>   */
>
>  /**
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index dcd737a1d1a0..3b5cdf1e1849 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
>  	}
>
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	IPAOperationData ipaConfig;
> +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> +			      ipaConfig, nullptr);
>
>  	return 0;
>  }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3c01821135f8..4fde5539e667 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>
> -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	IPAOperationData ipaConfig;
> +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> +			      ipaConfig, nullptr);
>
>  	return ret;
>  }
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index be34f20aa857..68eafb307b2a 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -31,7 +31,9 @@ public:
>  	void stop() override {}
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override {}
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
>  	void processEvent(const IPAOperationData &event) override {}
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index 6fbebed2ba72..aa403e22f297 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -31,7 +31,9 @@ public:
>
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void processEvent(const IPAOperationData &event) override;
> @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
>
>  void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>  			       const std::map<unsigned int, IPAStream> &streamConfig,
> -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			       const IPAOperationData &ipaConfig,
> +			       IPAOperationData *result)
>  {
> -	ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> +			result);
>  }
>
>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index aa7a9dcc6050..23c799da0663 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -69,7 +69,9 @@ public:
>
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		       const IPAOperationData &ipaConfig,
> +		       IPAOperationData *result) override
>  	{
>  		/* Verify sensorInfo. */
>  		if (sensorInfo.outputSize.width != 2560 ||
> @@ -317,7 +319,9 @@ protected:
>  		};
>  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
>  		controlInfo.emplace(42, subdev_->controls());
> -		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> +		IPAOperationData ipaConfig;
> +		ret = INVOKE(configure, sensorInfo, config, controlInfo,
> +			     ipaConfig, nullptr);
>  		if (ret == TestFail)
>  			return TestFail;
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 2, 2020, 9:23 p.m. UTC | #6 
Hi Jacopo,

On Tue, Jun 30, 2020 at 12:34:59PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 29, 2020 at 02:19:28AM +0300, Laurent Pinchart wrote:
> > Add two new parameters, ipaConfig and result, to the
> > IPAInterface::configure() function to allow pipeline handlers to pass
> > custom data to their IPA, and receive data back. Wire this through the
> > code base. The C API interface will be addressed separately, likely
> > through automation of the C <-> C++ translation.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/ipa_context_wrapper.h   |  4 +++-
> >  include/libcamera/ipa/ipa_interface.h              |  4 +++-
> >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  5 ++++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                |  8 ++++++--
> >  src/ipa/rkisp1/rkisp1.cpp                          |  8 ++++++--
> >  src/ipa/vimc/vimc.cpp                              |  4 +++-
> >  src/libcamera/ipa_context_wrapper.cpp              |  8 ++++++--
> >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 +++-
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 +++-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  4 +++-
> >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 11 ++++++++---
> >  test/ipa/ipa_wrappers_test.cpp                     |  8 ++++++--
> >  13 files changed, 61 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> > index 4e6f791d18e6..8f767e844221 100644
> > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > @@ -24,7 +24,9 @@ public:
> >  	void stop() override;
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override;
> 
> This, and all the other places where this is changed in the same way,
> fit in one line if I'm not mistaken.

		       const IPAOperationData &ipaConfig, IPAOperationData *result) override;

is 93 characters long, and we try to keep lines at most 80 characters
long when it doesn't hinder readability.

> >
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > index dc9fc714d564..5016ec25ea9c 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -158,7 +158,9 @@ public:
> >
> >  	virtual void configure(const CameraSensorInfo &sensorInfo,
> >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +			       const IPAOperationData &ipaConfig,
> > +			       IPAOperationData *result) = 0;
> >
> >  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> >  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index 2a2e43abc708..47ce5a704851 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> >  		entityControls.emplace(id, infoMaps[id]);
> >  	}
> >
> > -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > +	/* \todo Translate the ipaConfig and reponse  */
> 
> if the usage of response in place of result is not intentional, please
> fix.

Good catch. I'll fix that.

> Also missing . at the end, not sure if should be used in todo
> entries though.

I would have sworn we don't add periods at the end of todo entries, but
among the 33 entries that fit on a single line, 17 have a period. I'll
thus follow the majority for once :-)

> > +	IPAOperationData ipaConfig;
> > +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> > +			     nullptr);
> >  }
> >
> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index bc89ab58d03a..860be22ddb5d 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -78,7 +78,9 @@ public:
> >
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &data,
> > +		       IPAOperationData *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;
> > @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> >
> >  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result)
> >  {
> >  	if (entityControls.empty())
> >  		return;
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index fbdc908fc816..34d6f63a7ff4 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -39,7 +39,9 @@ public:
> >
> >  	void configure(const CameraSensorInfo &info,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *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;
> > @@ -76,7 +78,9 @@ private:
> >   */
> >  void IPARkISP1::configure(const CameraSensorInfo &info,
> >  			  const std::map<unsigned int, IPAStream> &streamConfig,
> > -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +			  const IPAOperationData &ipaConfig,
> > +			  IPAOperationData *result)
> >  {
> >  	if (entityControls.empty())
> >  		return;
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index af278a482b8a..1593c92d80f3 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -39,7 +39,9 @@ public:
> >
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override {}
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> >  	void processEvent(const IPAOperationData &event) override {}
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index 471118f57cdc..231300ce0bec 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
> >
> >  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> >  				  const std::map<unsigned int, IPAStream> &streamConfig,
> > -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +				  const IPAOperationData &ipaConfig,
> > +				  IPAOperationData *result)
> >  {
> >  	if (intf_)
> > -		return intf_->configure(sensorInfo, streamConfig, entityControls);
> > +		return intf_->configure(sensorInfo, streamConfig,
> > +					entityControls, ipaConfig, result);
> >
> >  	if (!ctx_)
> >  		return;
> > @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> >  		++i;
> >  	}
> >
> > +	/* \todo Translate the ipaConfig and reponse */
> >  	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> >  			     c_info_maps, entityControls.size());
> >  }
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index ebe47e1233a5..23fc56d7d48e 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -557,6 +557,8 @@ namespace libcamera {
> >   * \param[in] sensorInfo Camera sensor information
> >   * \param[in] streamConfig Configuration of all active streams
> >   * \param[in] entityControls Controls provided by the pipeline entities
> > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > + * \param[out] result Pipeline-handler-specific configuration result
> >   *
> >   * This method shall be called when the camera is started to inform the IPA of
> >   * the camera's streams and the sensor settings. The meaning of the numerical
> > @@ -566,6 +568,11 @@ namespace libcamera {
> >   * The \a sensorInfo conveys information about the camera sensor settings that
> >   * the pipeline handler has selected for the configuration. The IPA may use
> >   * that information to tune its algorithms.
> > + *
> > + * The \a ipaConfig 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 configure() function.
> 
> I would like to have a bit more details here. Like in example, how
> does this differ from entityControls ? The way controls are passed
> there and associated to an arbitrary entity shouldn't (in the long
> run, provided all required controls are defined) serve the same
> purpose ? Equally, if 'result' aims to carry sensor configurations,
> shouldn't it be a control list ?

As we've discussed offline, entityControls is a map of entity ID to
ControlMapInfo, not ControlList. The two are thus different. Both
ipaConfig and result (which I'll rename to ipaResult for symmetry) carry
a vector of ControlList.

> >   */
> >
> >  /**
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index dcd737a1d1a0..3b5cdf1e1849 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
> >  	}
> >
> >  	/* Ready the IPA - it must know about the sensor resolution. */
> > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > +	IPAOperationData ipaConfig;
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +			      ipaConfig, nullptr);
> >
> >  	return 0;
> >  }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 3c01821135f8..4fde5539e667 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> >  	entityControls.emplace(0, data->sensor_->controls());
> >
> > -	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > +	IPAOperationData ipaConfig;
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +			      ipaConfig, nullptr);
> >
> >  	return ret;
> >  }
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index be34f20aa857..68eafb307b2a 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -31,7 +31,9 @@ public:
> >  	void stop() override {}
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override {}
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> >  	void processEvent(const IPAOperationData &event) override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index 6fbebed2ba72..aa403e22f297 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -31,7 +31,9 @@ public:
> >
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override;
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >  	void processEvent(const IPAOperationData &event) override;
> > @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
> >
> >  void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> >  			       const std::map<unsigned int, IPAStream> &streamConfig,
> > -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > +			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +			       const IPAOperationData &ipaConfig,
> > +			       IPAOperationData *result)
> >  {
> > -	ipa_->configure(sensorInfo, streamConfig, entityControls);
> > +	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > +			result);
> >  }
> >
> >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > index aa7a9dcc6050..23c799da0663 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -69,7 +69,9 @@ public:
> >
> >  	void configure(const CameraSensorInfo &sensorInfo,
> >  		       const std::map<unsigned int, IPAStream> &streamConfig,
> > -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > +		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > +		       const IPAOperationData &ipaConfig,
> > +		       IPAOperationData *result) override
> >  	{
> >  		/* Verify sensorInfo. */
> >  		if (sensorInfo.outputSize.width != 2560 ||
> > @@ -317,7 +319,9 @@ protected:
> >  		};
> >  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
> >  		controlInfo.emplace(42, subdev_->controls());
> > -		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > +		IPAOperationData ipaConfig;
> > +		ret = INVOKE(configure, sensorInfo, config, controlInfo,
> > +			     ipaConfig, nullptr);
> >  		if (ret == TestFail)
> >  			return TestFail;
> >

Patch
diff mbox series 

diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
index 4e6f791d18e6..8f767e844221 100644
--- a/include/libcamera/internal/ipa_context_wrapper.h
+++ b/include/libcamera/internal/ipa_context_wrapper.h
@@ -24,7 +24,9 @@  public:
 	void stop() override;
 	void configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *result) override;
 
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
index dc9fc714d564..5016ec25ea9c 100644
--- a/include/libcamera/ipa/ipa_interface.h
+++ b/include/libcamera/ipa/ipa_interface.h
@@ -158,7 +158,9 @@  public:
 
 	virtual void configure(const CameraSensorInfo &sensorInfo,
 			       const std::map<unsigned int, IPAStream> &streamConfig,
-			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
+			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+			       const IPAOperationData &ipaConfig,
+			       IPAOperationData *result) = 0;
 
 	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
 	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
index 2a2e43abc708..47ce5a704851 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.cpp
+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
@@ -166,7 +166,10 @@  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
 		entityControls.emplace(id, infoMaps[id]);
 	}
 
-	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
+	/* \todo Translate the ipaConfig and reponse  */
+	IPAOperationData ipaConfig;
+	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
+			     nullptr);
 }
 
 void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index bc89ab58d03a..860be22ddb5d 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -78,7 +78,9 @@  public:
 
 	void configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &data,
+		       IPAOperationData *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;
@@ -186,7 +188,9 @@  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
 
 void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *result)
 {
 	if (entityControls.empty())
 		return;
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index fbdc908fc816..34d6f63a7ff4 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -39,7 +39,9 @@  public:
 
 	void configure(const CameraSensorInfo &info,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *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;
@@ -76,7 +78,9 @@  private:
  */
 void IPARkISP1::configure(const CameraSensorInfo &info,
 			  const std::map<unsigned int, IPAStream> &streamConfig,
-			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
+			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+			  const IPAOperationData &ipaConfig,
+			  IPAOperationData *result)
 {
 	if (entityControls.empty())
 		return;
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index af278a482b8a..1593c92d80f3 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -39,7 +39,9 @@  public:
 
 	void configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *result) override {}
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
 	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
 	void processEvent(const IPAOperationData &event) override {}
diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
index 471118f57cdc..231300ce0bec 100644
--- a/src/libcamera/ipa_context_wrapper.cpp
+++ b/src/libcamera/ipa_context_wrapper.cpp
@@ -110,10 +110,13 @@  void IPAContextWrapper::stop()
 
 void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
 				  const std::map<unsigned int, IPAStream> &streamConfig,
-				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
+				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+				  const IPAOperationData &ipaConfig,
+				  IPAOperationData *result)
 {
 	if (intf_)
-		return intf_->configure(sensorInfo, streamConfig, entityControls);
+		return intf_->configure(sensorInfo, streamConfig,
+					entityControls, ipaConfig, result);
 
 	if (!ctx_)
 		return;
@@ -174,6 +177,7 @@  void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
 		++i;
 	}
 
+	/* \todo Translate the ipaConfig and reponse */
 	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
 			     c_info_maps, entityControls.size());
 }
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index ebe47e1233a5..23fc56d7d48e 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -557,6 +557,8 @@  namespace libcamera {
  * \param[in] sensorInfo Camera sensor information
  * \param[in] streamConfig Configuration of all active streams
  * \param[in] entityControls Controls provided by the pipeline entities
+ * \param[in] ipaConfig Pipeline-handler-specific configuration data
+ * \param[out] result Pipeline-handler-specific configuration result
  *
  * This method shall be called when the camera is started to inform the IPA of
  * the camera's streams and the sensor settings. The meaning of the numerical
@@ -566,6 +568,11 @@  namespace libcamera {
  * The \a sensorInfo conveys information about the camera sensor settings that
  * the pipeline handler has selected for the configuration. The IPA may use
  * that information to tune its algorithms.
+ *
+ * The \a ipaConfig 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 configure() function.
  */
 
 /**
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index dcd737a1d1a0..3b5cdf1e1849 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1017,7 +1017,9 @@  int PipelineHandlerRPi::configureIPA(Camera *camera)
 	}
 
 	/* Ready the IPA - it must know about the sensor resolution. */
-	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
+	IPAOperationData ipaConfig;
+	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
+			      ipaConfig, nullptr);
 
 	return 0;
 }
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 3c01821135f8..4fde5539e667 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -843,7 +843,9 @@  int PipelineHandlerRkISP1::start(Camera *camera)
 	std::map<unsigned int, const ControlInfoMap &> entityControls;
 	entityControls.emplace(0, data->sensor_->controls());
 
-	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
+	IPAOperationData ipaConfig;
+	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
+			      ipaConfig, nullptr);
 
 	return ret;
 }
diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
index be34f20aa857..68eafb307b2a 100644
--- a/src/libcamera/proxy/ipa_proxy_linux.cpp
+++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
@@ -31,7 +31,9 @@  public:
 	void stop() override {}
 	void configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *result) override {}
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
 	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
 	void processEvent(const IPAOperationData &event) override {}
diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
index 6fbebed2ba72..aa403e22f297 100644
--- a/src/libcamera/proxy/ipa_proxy_thread.cpp
+++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
@@ -31,7 +31,9 @@  public:
 
 	void configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *result) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 	void processEvent(const IPAOperationData &event) override;
@@ -129,9 +131,12 @@  void IPAProxyThread::stop()
 
 void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
 			       const std::map<unsigned int, IPAStream> &streamConfig,
-			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
+			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+			       const IPAOperationData &ipaConfig,
+			       IPAOperationData *result)
 {
-	ipa_->configure(sensorInfo, streamConfig, entityControls);
+	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
+			result);
 }
 
 void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
index aa7a9dcc6050..23c799da0663 100644
--- a/test/ipa/ipa_wrappers_test.cpp
+++ b/test/ipa/ipa_wrappers_test.cpp
@@ -69,7 +69,9 @@  public:
 
 	void configure(const CameraSensorInfo &sensorInfo,
 		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
+		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		       const IPAOperationData &ipaConfig,
+		       IPAOperationData *result) override
 	{
 		/* Verify sensorInfo. */
 		if (sensorInfo.outputSize.width != 2560 ||
@@ -317,7 +319,9 @@  protected:
 		};
 		std::map<unsigned int, const ControlInfoMap &> controlInfo;
 		controlInfo.emplace(42, subdev_->controls());
-		ret = INVOKE(configure, sensorInfo, config, controlInfo);
+		IPAOperationData ipaConfig;
+		ret = INVOKE(configure, sensorInfo, config, controlInfo,
+			     ipaConfig, nullptr);
 		if (ret == TestFail)
 			return TestFail;