[libcamera-devel,07/11] ipa: Pass IPA initialization settings to IPAInterface::init()
diff mbox series

Message ID 20200427031713.14013-8-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Add support for IPA configuration
Related show

Commit Message

Laurent Pinchart April 27, 2020, 3:17 a.m. UTC 
Add a new IPASettings class to pass IPA initialization settings through
the IPAInterface::init() method. The settings currently only contain the
name of a configuration file, and are expected to be extended later.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/ipa/ipa_interface.h                 | 13 +++++++--
 src/ipa/libipa/ipa_interface_wrapper.cpp    |  8 ++++--
 src/ipa/libipa/ipa_interface_wrapper.h      |  3 +-
 src/ipa/rkisp1/rkisp1.cpp                   |  2 +-
 src/ipa/vimc/vimc.cpp                       |  4 +--
 src/libcamera/include/ipa_context_wrapper.h |  2 +-
 src/libcamera/ipa_context_wrapper.cpp       |  9 ++++--
 src/libcamera/ipa_interface.cpp             | 32 +++++++++++++++++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  2 +-
 src/libcamera/pipeline/vimc/vimc.cpp        |  2 +-
 src/libcamera/proxy/ipa_proxy_linux.cpp     |  2 +-
 src/libcamera/proxy/ipa_proxy_thread.cpp    |  6 ++--
 test/ipa/ipa_interface_test.cpp             |  3 +-
 test/ipa/ipa_wrappers_test.cpp              | 13 +++++++--
 14 files changed, 80 insertions(+), 21 deletions(-)

Comments

Jacopo Mondi April 27, 2020, 9:13 a.m. UTC | #1 
Hi Laurent,

On Mon, Apr 27, 2020 at 06:17:09AM +0300, Laurent Pinchart wrote:
> Add a new IPASettings class to pass IPA initialization settings through
> the IPAInterface::init() method. The settings currently only contain the
> name of a configuration file, and are expected to be extended later.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/ipa/ipa_interface.h                 | 13 +++++++--
>  src/ipa/libipa/ipa_interface_wrapper.cpp    |  8 ++++--
>  src/ipa/libipa/ipa_interface_wrapper.h      |  3 +-
>  src/ipa/rkisp1/rkisp1.cpp                   |  2 +-
>  src/ipa/vimc/vimc.cpp                       |  4 +--
>  src/libcamera/include/ipa_context_wrapper.h |  2 +-
>  src/libcamera/ipa_context_wrapper.cpp       |  9 ++++--
>  src/libcamera/ipa_interface.cpp             | 32 +++++++++++++++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  2 +-
>  src/libcamera/pipeline/vimc/vimc.cpp        |  2 +-
>  src/libcamera/proxy/ipa_proxy_linux.cpp     |  2 +-
>  src/libcamera/proxy/ipa_proxy_thread.cpp    |  6 ++--
>  test/ipa/ipa_interface_test.cpp             |  3 +-
>  test/ipa/ipa_wrappers_test.cpp              | 13 +++++++--
>  14 files changed, 80 insertions(+), 21 deletions(-)
>
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index e65844bc7b34..ef3d6507b692 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -18,6 +18,10 @@ struct ipa_context {
>  	const struct ipa_context_ops *ops;
>  };
>
> +struct ipa_settings {
> +	const char *configuration_file;
> +};
> +

I've seen you suggesting multiple times to use a char * to transport
strings, here in the IPA operations and the kernel ioctls in example. I
wonder, isn't this dangeours ? One can easily point this to some
memory that could vanish, and even if I recognize reserving a buffer
with a fixed length is limiting, it feels more safer to me...

>  struct ipa_stream {
>  	unsigned int id;
>  	unsigned int pixel_format;
> @@ -63,7 +67,8 @@ struct ipa_callback_ops {
>  struct ipa_context_ops {
>  	void (*destroy)(struct ipa_context *ctx);
>  	void *(*get_interface)(struct ipa_context *ctx);
> -	void (*init)(struct ipa_context *ctx);
> +	void (*init)(struct ipa_context *ctx,
> +		     const struct ipa_settings *settings);
>  	int (*start)(struct ipa_context *ctx);
>  	void (*stop)(struct ipa_context *ctx);
>  	void (*register_callbacks)(struct ipa_context *ctx,
> @@ -100,6 +105,10 @@ struct ipa_context *ipaCreate();
>
>  namespace libcamera {
>
> +struct IPASettings {
> +	std::string configurationFile;
> +};
> +
>  struct IPAStream {
>  	unsigned int pixelFormat;
>  	Size size;
> @@ -121,7 +130,7 @@ class IPAInterface
>  public:
>  	virtual ~IPAInterface() {}
>
> -	virtual int init() = 0;
> +	virtual int init(const IPASettings &settings) = 0;
>  	virtual int start() = 0;
>  	virtual void stop() = 0;
>
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index f50f93a0185a..7776d9881575 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -79,11 +79,15 @@ void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)
>  	return ctx->ipa_.get();
>  }
>
> -void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
> +void IPAInterfaceWrapper::init(struct ipa_context *_ctx,
> +			       const struct ipa_settings *settings)
>  {
>  	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
>
> -	ctx->ipa_->init();
> +	IPASettings ipaSettings{
> +		.configurationFile = std::string{ settings->configuration_file }

Can't you just assign a char * to an std::string ?

> +	};
> +	ctx->ipa_->init(ipaSettings);
>  }
>
>  int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> index e4bc6dd4535d..78ccf0f59d42 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -23,7 +23,8 @@ public:
>  private:
>  	static void destroy(struct ipa_context *ctx);
>  	static void *get_interface(struct ipa_context *ctx);
> -	static void init(struct ipa_context *ctx);
> +	static void init(struct ipa_context *ctx,
> +			 const struct ipa_settings *settings);
>  	static int start(struct ipa_context *ctx);
>  	static void stop(struct ipa_context *ctx);
>  	static void register_callbacks(struct ipa_context *ctx,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 7f0ffb0a791f..9a347e527cd2 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -32,7 +32,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)
>  class IPARkISP1 : public IPAInterface
>  {
>  public:
> -	int init() override { return 0; }
> +	int init(const IPASettings &settings) override { return 0; }
>  	int start() override { return 0; }
>  	void stop() override {}
>
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cfdbd6f99155..e6bda8ec58b0 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -31,7 +31,7 @@ public:
>  	IPAVimc();
>  	~IPAVimc();
>
> -	int init() override;
> +	int init(const IPASettings &settings) override;
>
>  	int start() override;
>  	void stop() override;
> @@ -61,7 +61,7 @@ IPAVimc::~IPAVimc()
>  		::close(fd_);
>  }
>
> -int IPAVimc::init()
> +int IPAVimc::init(const IPASettings &settings)
>  {
>  	trace(IPAOperationInit);
>
> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> index 0a48bfe732c8..64395b4a450b 100644
> --- a/src/libcamera/include/ipa_context_wrapper.h
> +++ b/src/libcamera/include/ipa_context_wrapper.h
> @@ -19,7 +19,7 @@ public:
>  	IPAContextWrapper(struct ipa_context *context);
>  	~IPAContextWrapper();
>
> -	int init() override;
> +	int init(const IPASettings &settings) override;
>  	int start() override;
>  	void stop() override;
>  	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index ab6ce396b88a..5bd5d916ccc0 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -69,15 +69,18 @@ IPAContextWrapper::~IPAContextWrapper()
>  	ctx_->ops->destroy(ctx_);
>  }
>
> -int IPAContextWrapper::init()
> +int IPAContextWrapper::init(const IPASettings &settings)
>  {
>  	if (intf_)
> -		return intf_->init();
> +		return intf_->init(settings);
>
>  	if (!ctx_)
>  		return 0;
>
> -	ctx_->ops->init(ctx_);
> +	struct ipa_settings c_settings;
> +	c_settings.configuration_file = settings.configurationFile.c_str();
> +
> +	ctx_->ops->init(ctx_, &c_settings);
>
>  	return 0;
>  }
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 890d4340e4f3..3949b7d6ea5d 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -92,6 +92,16 @@
>   * \brief The IPA context operations
>   */
>
> +/**
> + * \struct ipa_settings
> + * \brief IPA initialization settings for the IPA context operations
> + * \sa IPASettings
> + *
> + * \var ipa_settings::configuration_file
> + * \brief The name of the IPA configuration file (may be null or point to an
> + * empty string)
> + */
> +
>  /**
>   * \struct ipa_stream
>   * \brief Stream information for the IPA context operations
> @@ -231,6 +241,7 @@
>   * \var ipa_context_ops::init
>   * \brief Initialise the IPA context
>   * \param[in] ctx The IPA context
> + * \param[in] settings The IPA initialization settings
>   *
>   * \sa libcamera::IPAInterface::init()
>   */
> @@ -310,6 +321,22 @@
>
>  namespace libcamera {
>
> +/**
> + * \struct IPASettings
> + * \brief IPA interface initialization settings
> + *
> + * The IPASettings structure stores data passed to the IPAInterface::init()
> + * function.

Is it worth mentioning these data are meant not to depend upon run
time operations ?

> + */
> +
> +/**
> + * \var IPASettings::configurationFile
> + * \brief The name of the IPA configuration file
> + *
> + * This field may be an empty string if the IPA doesn't require a configuration
> + * file.
> + */
> +
>  /**
>   * \struct IPAStream
>   * \brief Stream configuration for the IPA interface
> @@ -424,6 +451,11 @@ namespace libcamera {
>  /**
>   * \fn IPAInterface::init()
>   * \brief Initialise the IPAInterface
> + * \param[in] settings The IPA initialization settings
> + *
> + * This function initializes the IPA interface. It shall be called before any
> + * other function of the IPAInterface. The \a settings carry initialization
> + * parameters that will remain valid for the whole life time of the interface.

It's here, but maybe it's worth up there too...

>   */
>
>  /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index f42264361785..fde445b99a46 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -401,7 +401,7 @@ int RkISP1CameraData::loadIPA()
>  	ipa_->queueFrameAction.connect(this,
>  				       &RkISP1CameraData::queueFrameAction);
>
> -	ipa_->init();
> +	ipa_->init(IPASettings{});
>
>  	return 0;
>  }
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index c5eea3a01b0e..699f788aa1b8 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -374,7 +374,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  	if (data->ipa_ == nullptr)
>  		LOG(VIMC, Warning) << "no matching IPA found";
>  	else
> -		data->ipa_->init();
> +		data->ipa_->init(IPASettings{});
>
>  	/* Locate and open the capture video node. */
>  	if (data->init(media))
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index 89f5cb54687b..cd8ac824679d 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -26,7 +26,7 @@ public:
>  	IPAProxyLinux(IPAModule *ipam);
>  	~IPAProxyLinux();
>
> -	int init() override { return 0; }
> +	int init(const IPASettings &settings) override { return 0; }
>  	int start() override { return 0; }
>  	void stop() override {}
>  	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index 1ebb9b6a6c9d..be82fde34bd9 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -25,7 +25,7 @@ class IPAProxyThread : public IPAProxy, public Object
>  public:
>  	IPAProxyThread(IPAModule *ipam);
>
> -	int init() override;
> +	int init(const IPASettings &settings) override;
>  	int start() override;
>  	void stop() override;
>
> @@ -97,9 +97,9 @@ IPAProxyThread::IPAProxyThread(IPAModule *ipam)
>  	valid_ = true;
>  }
>
> -int IPAProxyThread::init()
> +int IPAProxyThread::init(const IPASettings &settings)
>  {
> -	int ret = ipa_->init();
> +	int ret = ipa_->init(settings);
>  	if (ret)
>  		return ret;
>
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 22f9ca41ef37..2e2dfb8d1ebd 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -98,7 +98,8 @@ protected:
>  		}
>
>  		/* Test initialization of IPA module. */
> -		ipa_->init();
> +		IPASettings settings;
> +		ipa_->init(settings);
>  		timer.start(1000);
>  		while (timer.isRunning() && trace_ != IPAOperationInit)
>  			dispatcher->processEvents();
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index fae1d56b67c7..21bf51a2e046 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -43,8 +43,14 @@ public:
>  	{
>  	}
>
> -	int init() override
> +	int init(const IPASettings &settings) override
>  	{
> +		if (settings.configurationFile != "/ipa/configuration/file") {
> +			cerr << "init(): Invalid configuration file" << endl;
> +			report(Op_init, TestFail);
> +			return 0;

Shouldn't you 'return report(Op_init.. ' ?

> +		}
> +
>  		report(Op_init, TestPass);
>  		return 0;
>  	}
> @@ -339,7 +345,10 @@ protected:
>  		 * Test init(), start() and stop() last to ensure nothing in the
>  		 * wrappers or serializer depends on them being called first.
>  		 */
> -		ret = INVOKE(init);
> +		IPASettings settings{
> +			.configurationFile = "/ipa/configuration/file"
> +		};
> +		ret = INVOKE(init, settings);
>  		if (ret == TestFail) {
>  			cerr << "Failed to run init()";
>  			return TestFail;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 27, 2020, 11:11 a.m. UTC | #2 
Hi Jacopo,

On Mon, Apr 27, 2020 at 11:13:03AM +0200, Jacopo Mondi wrote:
> On Mon, Apr 27, 2020 at 06:17:09AM +0300, Laurent Pinchart wrote:
> > Add a new IPASettings class to pass IPA initialization settings through
> > the IPAInterface::init() method. The settings currently only contain the
> > name of a configuration file, and are expected to be extended later.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/ipa/ipa_interface.h                 | 13 +++++++--
> >  src/ipa/libipa/ipa_interface_wrapper.cpp    |  8 ++++--
> >  src/ipa/libipa/ipa_interface_wrapper.h      |  3 +-
> >  src/ipa/rkisp1/rkisp1.cpp                   |  2 +-
> >  src/ipa/vimc/vimc.cpp                       |  4 +--
> >  src/libcamera/include/ipa_context_wrapper.h |  2 +-
> >  src/libcamera/ipa_context_wrapper.cpp       |  9 ++++--
> >  src/libcamera/ipa_interface.cpp             | 32 +++++++++++++++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  2 +-
> >  src/libcamera/pipeline/vimc/vimc.cpp        |  2 +-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp     |  2 +-
> >  src/libcamera/proxy/ipa_proxy_thread.cpp    |  6 ++--
> >  test/ipa/ipa_interface_test.cpp             |  3 +-
> >  test/ipa/ipa_wrappers_test.cpp              | 13 +++++++--
> >  14 files changed, 80 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index e65844bc7b34..ef3d6507b692 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -18,6 +18,10 @@ struct ipa_context {
> >  	const struct ipa_context_ops *ops;
> >  };
> >
> > +struct ipa_settings {
> > +	const char *configuration_file;
> > +};
> > +
> 
> I've seen you suggesting multiple times to use a char * to transport
> strings, here in the IPA operations and the kernel ioctls in example. I
> wonder, isn't this dangeours ? One can easily point this to some
> memory that could vanish, and even if I recognize reserving a buffer
> with a fixed length is limiting, it feels more safer to me...

One could indeed, but given that the C API structures are filled and
consumed by the wrappers only, there's a single place where we have to
be careful, so I think it's fine.

> >  struct ipa_stream {
> >  	unsigned int id;
> >  	unsigned int pixel_format;
> > @@ -63,7 +67,8 @@ struct ipa_callback_ops {
> >  struct ipa_context_ops {
> >  	void (*destroy)(struct ipa_context *ctx);
> >  	void *(*get_interface)(struct ipa_context *ctx);
> > -	void (*init)(struct ipa_context *ctx);
> > +	void (*init)(struct ipa_context *ctx,
> > +		     const struct ipa_settings *settings);
> >  	int (*start)(struct ipa_context *ctx);
> >  	void (*stop)(struct ipa_context *ctx);
> >  	void (*register_callbacks)(struct ipa_context *ctx,
> > @@ -100,6 +105,10 @@ struct ipa_context *ipaCreate();
> >
> >  namespace libcamera {
> >
> > +struct IPASettings {
> > +	std::string configurationFile;
> > +};
> > +
> >  struct IPAStream {
> >  	unsigned int pixelFormat;
> >  	Size size;
> > @@ -121,7 +130,7 @@ class IPAInterface
> >  public:
> >  	virtual ~IPAInterface() {}
> >
> > -	virtual int init() = 0;
> > +	virtual int init(const IPASettings &settings) = 0;
> >  	virtual int start() = 0;
> >  	virtual void stop() = 0;
> >
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index f50f93a0185a..7776d9881575 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -79,11 +79,15 @@ void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)
> >  	return ctx->ipa_.get();
> >  }
> >
> > -void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
> > +void IPAInterfaceWrapper::init(struct ipa_context *_ctx,
> > +			       const struct ipa_settings *settings)
> >  {
> >  	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> >
> > -	ctx->ipa_->init();
> > +	IPASettings ipaSettings{
> > +		.configurationFile = std::string{ settings->configuration_file }
> 
> Can't you just assign a char * to an std::string ?

We actually can, it's not just that the constructor isn't marked as
explicit, there's an overload of the assignment operator that takes a
const char *. I'll fix this.

> > +	};
> > +	ctx->ipa_->init(ipaSettings);
> >  }
> >
> >  int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> > index e4bc6dd4535d..78ccf0f59d42 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -23,7 +23,8 @@ public:
> >  private:
> >  	static void destroy(struct ipa_context *ctx);
> >  	static void *get_interface(struct ipa_context *ctx);
> > -	static void init(struct ipa_context *ctx);
> > +	static void init(struct ipa_context *ctx,
> > +			 const struct ipa_settings *settings);
> >  	static int start(struct ipa_context *ctx);
> >  	static void stop(struct ipa_context *ctx);
> >  	static void register_callbacks(struct ipa_context *ctx,
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 7f0ffb0a791f..9a347e527cd2 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -32,7 +32,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1)
> >  class IPARkISP1 : public IPAInterface
> >  {
> >  public:
> > -	int init() override { return 0; }
> > +	int init(const IPASettings &settings) override { return 0; }
> >  	int start() override { return 0; }
> >  	void stop() override {}
> >
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index cfdbd6f99155..e6bda8ec58b0 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -31,7 +31,7 @@ public:
> >  	IPAVimc();
> >  	~IPAVimc();
> >
> > -	int init() override;
> > +	int init(const IPASettings &settings) override;
> >
> >  	int start() override;
> >  	void stop() override;
> > @@ -61,7 +61,7 @@ IPAVimc::~IPAVimc()
> >  		::close(fd_);
> >  }
> >
> > -int IPAVimc::init()
> > +int IPAVimc::init(const IPASettings &settings)
> >  {
> >  	trace(IPAOperationInit);
> >
> > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> > index 0a48bfe732c8..64395b4a450b 100644
> > --- a/src/libcamera/include/ipa_context_wrapper.h
> > +++ b/src/libcamera/include/ipa_context_wrapper.h
> > @@ -19,7 +19,7 @@ public:
> >  	IPAContextWrapper(struct ipa_context *context);
> >  	~IPAContextWrapper();
> >
> > -	int init() override;
> > +	int init(const IPASettings &settings) override;
> >  	int start() override;
> >  	void stop() override;
> >  	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index ab6ce396b88a..5bd5d916ccc0 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -69,15 +69,18 @@ IPAContextWrapper::~IPAContextWrapper()
> >  	ctx_->ops->destroy(ctx_);
> >  }
> >
> > -int IPAContextWrapper::init()
> > +int IPAContextWrapper::init(const IPASettings &settings)
> >  {
> >  	if (intf_)
> > -		return intf_->init();
> > +		return intf_->init(settings);
> >
> >  	if (!ctx_)
> >  		return 0;
> >
> > -	ctx_->ops->init(ctx_);
> > +	struct ipa_settings c_settings;
> > +	c_settings.configuration_file = settings.configurationFile.c_str();
> > +
> > +	ctx_->ops->init(ctx_, &c_settings);
> >
> >  	return 0;
> >  }
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index 890d4340e4f3..3949b7d6ea5d 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -92,6 +92,16 @@
> >   * \brief The IPA context operations
> >   */
> >
> > +/**
> > + * \struct ipa_settings
> > + * \brief IPA initialization settings for the IPA context operations
> > + * \sa IPASettings
> > + *
> > + * \var ipa_settings::configuration_file
> > + * \brief The name of the IPA configuration file (may be null or point to an
> > + * empty string)
> > + */
> > +
> >  /**
> >   * \struct ipa_stream
> >   * \brief Stream information for the IPA context operations
> > @@ -231,6 +241,7 @@
> >   * \var ipa_context_ops::init
> >   * \brief Initialise the IPA context
> >   * \param[in] ctx The IPA context
> > + * \param[in] settings The IPA initialization settings
> >   *
> >   * \sa libcamera::IPAInterface::init()
> >   */
> > @@ -310,6 +321,22 @@
> >
> >  namespace libcamera {
> >
> > +/**
> > + * \struct IPASettings
> > + * \brief IPA interface initialization settings
> > + *
> > + * The IPASettings structure stores data passed to the IPAInterface::init()
> > + * function.
> 
> Is it worth mentioning these data are meant not to depend upon run
> time operations ?

I'll write

 * The IPASettings structure stores data passed to the IPAInterface::init()
 * function. The data contain settings that don't depend on a particular camera
 * or pipeline configuration and are valid for the whole life time of the IPA
 * interface.

> > + */
> > +
> > +/**
> > + * \var IPASettings::configurationFile
> > + * \brief The name of the IPA configuration file
> > + *
> > + * This field may be an empty string if the IPA doesn't require a configuration
> > + * file.
> > + */
> > +
> >  /**
> >   * \struct IPAStream
> >   * \brief Stream configuration for the IPA interface
> > @@ -424,6 +451,11 @@ namespace libcamera {
> >  /**
> >   * \fn IPAInterface::init()
> >   * \brief Initialise the IPAInterface
> > + * \param[in] settings The IPA initialization settings
> > + *
> > + * This function initializes the IPA interface. It shall be called before any
> > + * other function of the IPAInterface. The \a settings carry initialization
> > + * parameters that will remain valid for the whole life time of the interface.
> 
> It's here, but maybe it's worth up there too...
> 
> >   */
> >
> >  /**
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f42264361785..fde445b99a46 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -401,7 +401,7 @@ int RkISP1CameraData::loadIPA()
> >  	ipa_->queueFrameAction.connect(this,
> >  				       &RkISP1CameraData::queueFrameAction);
> >
> > -	ipa_->init();
> > +	ipa_->init(IPASettings{});
> >
> >  	return 0;
> >  }
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index c5eea3a01b0e..699f788aa1b8 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -374,7 +374,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> >  	if (data->ipa_ == nullptr)
> >  		LOG(VIMC, Warning) << "no matching IPA found";
> >  	else
> > -		data->ipa_->init();
> > +		data->ipa_->init(IPASettings{});
> >
> >  	/* Locate and open the capture video node. */
> >  	if (data->init(media))
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 89f5cb54687b..cd8ac824679d 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -26,7 +26,7 @@ public:
> >  	IPAProxyLinux(IPAModule *ipam);
> >  	~IPAProxyLinux();
> >
> > -	int init() override { return 0; }
> > +	int init(const IPASettings &settings) override { return 0; }
> >  	int start() override { return 0; }
> >  	void stop() override {}
> >  	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index 1ebb9b6a6c9d..be82fde34bd9 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -25,7 +25,7 @@ class IPAProxyThread : public IPAProxy, public Object
> >  public:
> >  	IPAProxyThread(IPAModule *ipam);
> >
> > -	int init() override;
> > +	int init(const IPASettings &settings) override;
> >  	int start() override;
> >  	void stop() override;
> >
> > @@ -97,9 +97,9 @@ IPAProxyThread::IPAProxyThread(IPAModule *ipam)
> >  	valid_ = true;
> >  }
> >
> > -int IPAProxyThread::init()
> > +int IPAProxyThread::init(const IPASettings &settings)
> >  {
> > -	int ret = ipa_->init();
> > +	int ret = ipa_->init(settings);
> >  	if (ret)
> >  		return ret;
> >
> > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> > index 22f9ca41ef37..2e2dfb8d1ebd 100644
> > --- a/test/ipa/ipa_interface_test.cpp
> > +++ b/test/ipa/ipa_interface_test.cpp
> > @@ -98,7 +98,8 @@ protected:
> >  		}
> >
> >  		/* Test initialization of IPA module. */
> > -		ipa_->init();
> > +		IPASettings settings;
> > +		ipa_->init(settings);
> >  		timer.start(1000);
> >  		while (timer.isRunning() && trace_ != IPAOperationInit)
> >  			dispatcher->processEvents();
> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > index fae1d56b67c7..21bf51a2e046 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -43,8 +43,14 @@ public:
> >  	{
> >  	}
> >
> > -	int init() override
> > +	int init(const IPASettings &settings) override
> >  	{
> > +		if (settings.configurationFile != "/ipa/configuration/file") {
> > +			cerr << "init(): Invalid configuration file" << endl;
> > +			report(Op_init, TestFail);
> > +			return 0;
> 
> Shouldn't you 'return report(Op_init.. ' ?

report returns void, while this function returns an int. The INVOKE()
macro used below ignores the return value of the function. Testing the
return code of the init() is already done by another test, so I'm not
too concerned.

> > +		}
> > +
> >  		report(Op_init, TestPass);
> >  		return 0;
> >  	}
> > @@ -339,7 +345,10 @@ protected:
> >  		 * Test init(), start() and stop() last to ensure nothing in the
> >  		 * wrappers or serializer depends on them being called first.
> >  		 */
> > -		ret = INVOKE(init);
> > +		IPASettings settings{
> > +			.configurationFile = "/ipa/configuration/file"
> > +		};
> > +		ret = INVOKE(init, settings);
> >  		if (ret == TestFail) {
> >  			cerr << "Failed to run init()";
> >  			return TestFail;

Patch
diff mbox series 

diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
index e65844bc7b34..ef3d6507b692 100644
--- a/include/ipa/ipa_interface.h
+++ b/include/ipa/ipa_interface.h
@@ -18,6 +18,10 @@  struct ipa_context {
 	const struct ipa_context_ops *ops;
 };
 
+struct ipa_settings {
+	const char *configuration_file;
+};
+
 struct ipa_stream {
 	unsigned int id;
 	unsigned int pixel_format;
@@ -63,7 +67,8 @@  struct ipa_callback_ops {
 struct ipa_context_ops {
 	void (*destroy)(struct ipa_context *ctx);
 	void *(*get_interface)(struct ipa_context *ctx);
-	void (*init)(struct ipa_context *ctx);
+	void (*init)(struct ipa_context *ctx,
+		     const struct ipa_settings *settings);
 	int (*start)(struct ipa_context *ctx);
 	void (*stop)(struct ipa_context *ctx);
 	void (*register_callbacks)(struct ipa_context *ctx,
@@ -100,6 +105,10 @@  struct ipa_context *ipaCreate();
 
 namespace libcamera {
 
+struct IPASettings {
+	std::string configurationFile;
+};
+
 struct IPAStream {
 	unsigned int pixelFormat;
 	Size size;
@@ -121,7 +130,7 @@  class IPAInterface
 public:
 	virtual ~IPAInterface() {}
 
-	virtual int init() = 0;
+	virtual int init(const IPASettings &settings) = 0;
 	virtual int start() = 0;
 	virtual void stop() = 0;
 
diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
index f50f93a0185a..7776d9881575 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.cpp
+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
@@ -79,11 +79,15 @@  void *IPAInterfaceWrapper::get_interface(struct ipa_context *_ctx)
 	return ctx->ipa_.get();
 }
 
-void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
+void IPAInterfaceWrapper::init(struct ipa_context *_ctx,
+			       const struct ipa_settings *settings)
 {
 	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
 
-	ctx->ipa_->init();
+	IPASettings ipaSettings{
+		.configurationFile = std::string{ settings->configuration_file }
+	};
+	ctx->ipa_->init(ipaSettings);
 }
 
 int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
index e4bc6dd4535d..78ccf0f59d42 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.h
+++ b/src/ipa/libipa/ipa_interface_wrapper.h
@@ -23,7 +23,8 @@  public:
 private:
 	static void destroy(struct ipa_context *ctx);
 	static void *get_interface(struct ipa_context *ctx);
-	static void init(struct ipa_context *ctx);
+	static void init(struct ipa_context *ctx,
+			 const struct ipa_settings *settings);
 	static int start(struct ipa_context *ctx);
 	static void stop(struct ipa_context *ctx);
 	static void register_callbacks(struct ipa_context *ctx,
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 7f0ffb0a791f..9a347e527cd2 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -32,7 +32,7 @@  LOG_DEFINE_CATEGORY(IPARkISP1)
 class IPARkISP1 : public IPAInterface
 {
 public:
-	int init() override { return 0; }
+	int init(const IPASettings &settings) override { return 0; }
 	int start() override { return 0; }
 	void stop() override {}
 
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index cfdbd6f99155..e6bda8ec58b0 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -31,7 +31,7 @@  public:
 	IPAVimc();
 	~IPAVimc();
 
-	int init() override;
+	int init(const IPASettings &settings) override;
 
 	int start() override;
 	void stop() override;
@@ -61,7 +61,7 @@  IPAVimc::~IPAVimc()
 		::close(fd_);
 }
 
-int IPAVimc::init()
+int IPAVimc::init(const IPASettings &settings)
 {
 	trace(IPAOperationInit);
 
diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
index 0a48bfe732c8..64395b4a450b 100644
--- a/src/libcamera/include/ipa_context_wrapper.h
+++ b/src/libcamera/include/ipa_context_wrapper.h
@@ -19,7 +19,7 @@  public:
 	IPAContextWrapper(struct ipa_context *context);
 	~IPAContextWrapper();
 
-	int init() override;
+	int init(const IPASettings &settings) override;
 	int start() override;
 	void stop() override;
 	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
index ab6ce396b88a..5bd5d916ccc0 100644
--- a/src/libcamera/ipa_context_wrapper.cpp
+++ b/src/libcamera/ipa_context_wrapper.cpp
@@ -69,15 +69,18 @@  IPAContextWrapper::~IPAContextWrapper()
 	ctx_->ops->destroy(ctx_);
 }
 
-int IPAContextWrapper::init()
+int IPAContextWrapper::init(const IPASettings &settings)
 {
 	if (intf_)
-		return intf_->init();
+		return intf_->init(settings);
 
 	if (!ctx_)
 		return 0;
 
-	ctx_->ops->init(ctx_);
+	struct ipa_settings c_settings;
+	c_settings.configuration_file = settings.configurationFile.c_str();
+
+	ctx_->ops->init(ctx_, &c_settings);
 
 	return 0;
 }
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index 890d4340e4f3..3949b7d6ea5d 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -92,6 +92,16 @@ 
  * \brief The IPA context operations
  */
 
+/**
+ * \struct ipa_settings
+ * \brief IPA initialization settings for the IPA context operations
+ * \sa IPASettings
+ *
+ * \var ipa_settings::configuration_file
+ * \brief The name of the IPA configuration file (may be null or point to an
+ * empty string)
+ */
+
 /**
  * \struct ipa_stream
  * \brief Stream information for the IPA context operations
@@ -231,6 +241,7 @@ 
  * \var ipa_context_ops::init
  * \brief Initialise the IPA context
  * \param[in] ctx The IPA context
+ * \param[in] settings The IPA initialization settings
  *
  * \sa libcamera::IPAInterface::init()
  */
@@ -310,6 +321,22 @@ 
 
 namespace libcamera {
 
+/**
+ * \struct IPASettings
+ * \brief IPA interface initialization settings
+ *
+ * The IPASettings structure stores data passed to the IPAInterface::init()
+ * function.
+ */
+
+/**
+ * \var IPASettings::configurationFile
+ * \brief The name of the IPA configuration file
+ *
+ * This field may be an empty string if the IPA doesn't require a configuration
+ * file.
+ */
+
 /**
  * \struct IPAStream
  * \brief Stream configuration for the IPA interface
@@ -424,6 +451,11 @@  namespace libcamera {
 /**
  * \fn IPAInterface::init()
  * \brief Initialise the IPAInterface
+ * \param[in] settings The IPA initialization settings
+ *
+ * This function initializes the IPA interface. It shall be called before any
+ * other function of the IPAInterface. The \a settings carry initialization
+ * parameters that will remain valid for the whole life time of the interface.
  */
 
 /**
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index f42264361785..fde445b99a46 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -401,7 +401,7 @@  int RkISP1CameraData::loadIPA()
 	ipa_->queueFrameAction.connect(this,
 				       &RkISP1CameraData::queueFrameAction);
 
-	ipa_->init();
+	ipa_->init(IPASettings{});
 
 	return 0;
 }
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index c5eea3a01b0e..699f788aa1b8 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -374,7 +374,7 @@  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
 	if (data->ipa_ == nullptr)
 		LOG(VIMC, Warning) << "no matching IPA found";
 	else
-		data->ipa_->init();
+		data->ipa_->init(IPASettings{});
 
 	/* Locate and open the capture video node. */
 	if (data->init(media))
diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
index 89f5cb54687b..cd8ac824679d 100644
--- a/src/libcamera/proxy/ipa_proxy_linux.cpp
+++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
@@ -26,7 +26,7 @@  public:
 	IPAProxyLinux(IPAModule *ipam);
 	~IPAProxyLinux();
 
-	int init() override { return 0; }
+	int init(const IPASettings &settings) override { return 0; }
 	int start() override { return 0; }
 	void stop() override {}
 	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
index 1ebb9b6a6c9d..be82fde34bd9 100644
--- a/src/libcamera/proxy/ipa_proxy_thread.cpp
+++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
@@ -25,7 +25,7 @@  class IPAProxyThread : public IPAProxy, public Object
 public:
 	IPAProxyThread(IPAModule *ipam);
 
-	int init() override;
+	int init(const IPASettings &settings) override;
 	int start() override;
 	void stop() override;
 
@@ -97,9 +97,9 @@  IPAProxyThread::IPAProxyThread(IPAModule *ipam)
 	valid_ = true;
 }
 
-int IPAProxyThread::init()
+int IPAProxyThread::init(const IPASettings &settings)
 {
-	int ret = ipa_->init();
+	int ret = ipa_->init(settings);
 	if (ret)
 		return ret;
 
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index 22f9ca41ef37..2e2dfb8d1ebd 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -98,7 +98,8 @@  protected:
 		}
 
 		/* Test initialization of IPA module. */
-		ipa_->init();
+		IPASettings settings;
+		ipa_->init(settings);
 		timer.start(1000);
 		while (timer.isRunning() && trace_ != IPAOperationInit)
 			dispatcher->processEvents();
diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
index fae1d56b67c7..21bf51a2e046 100644
--- a/test/ipa/ipa_wrappers_test.cpp
+++ b/test/ipa/ipa_wrappers_test.cpp
@@ -43,8 +43,14 @@  public:
 	{
 	}
 
-	int init() override
+	int init(const IPASettings &settings) override
 	{
+		if (settings.configurationFile != "/ipa/configuration/file") {
+			cerr << "init(): Invalid configuration file" << endl;
+			report(Op_init, TestFail);
+			return 0;
+		}
+
 		report(Op_init, TestPass);
 		return 0;
 	}
@@ -339,7 +345,10 @@  protected:
 		 * Test init(), start() and stop() last to ensure nothing in the
 		 * wrappers or serializer depends on them being called first.
 		 */
-		ret = INVOKE(init);
+		IPASettings settings{
+			.configurationFile = "/ipa/configuration/file"
+		};
+		ret = INVOKE(init, settings);
 		if (ret == TestFail) {
 			cerr << "Failed to run init()";
 			return TestFail;