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

10499 diff mbox series

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

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

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

Hi Naush,

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

This is 'problematic' as it makes isolated IPAs deviate from other
ones. We have the same issue with configure() which has a very similar
\todo item :/

Now, this is not an issue right now, and I think with Paul's IPC
we'll get it for free.

Other comments below:

>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
>  include/libcamera/ipa/ipa_interface.h              |  3 ++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
>  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
>  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
>  src/ipa/vimc/vimc.cpp                              |  6 ++++--
>  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
>  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
>  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
>  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
>  test/ipa/ipa_interface_test.cpp                    |  3 ++-
>  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
>  15 files changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 8f767e84..1878a615 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -20,7 +20,8 @@ public:
>  	~IPAContextWrapper();
>
>  	int init(const IPASettings &settings) override;
> -	int start() override;
> +	int start(const IPAOperationData &ipaConfig,

'ipaConfig' is only used in configure.
It's generally just called 'data' and I would use it here too.

> +		  IPAOperationData *result) override;
>  	void stop() override;
>  	void configure(const CameraSensorInfo &sensorInfo,
>  		       const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 322b7079..b44e2538 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -153,7 +153,8 @@ public:
>  	virtual ~IPAInterface() = default;
>
>  	virtual int init(const IPASettings &settings) = 0;
> -	virtual int start() = 0;
> +	virtual int start(const IPAOperationData &ipaConfig,
> +			  IPAOperationData *result) = 0;
>  	virtual void stop() = 0;
>
>  	virtual void configure(const CameraSensorInfo &sensorInfo,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index cee532e3..2b305b56 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context *_ctx)
>  {
>  	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
>
> -	return ctx->ipa_->start();
> +	/* \todo Translate the ipaConfig and result. */
> +	IPAOperationData ipaConfig = {};
> +	return ctx->ipa_->start(ipaConfig, nullptr);
>  }
>
>  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index f338ff8b..7a07b477 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -77,7 +77,8 @@ public:
>  	}
>
>  	int init(const IPASettings &settings) override;
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>
>  	void configure(const CameraSensorInfo &sensorInfo,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 07d7f1b2..0b8a9096 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -37,7 +37,8 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>
>  	void configure(const CameraSensorInfo &info,
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cf841135..ed26331d 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -34,7 +34,8 @@ public:
>
>  	int init(const IPASettings &settings) override;
>
> -	int start() override;
> +	int start(const IPAOperationData &ipaConfig,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>
>  	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
>  	return 0;
>  }
>
> -int IPAVimc::start()
> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +		   [[maybe_unused]] IPAOperationData *result)

                   ^ is this mis-aligned or is it my email client ?
                   There are more in this patch if that's the case.
>  {
>  	trace(IPAOperationStart);
>
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 231300ce..8c4ec40a 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings &settings)
>  	return 0;
>  }
>
> -int IPAContextWrapper::start()
> +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> +			     IPAOperationData *result)
>  {
>  	if (intf_)
> -		return intf_->start();
> +		return intf_->start(ipaConfig, result);
>
>  	if (!ctx_)
>  		return 0;
>
> +	/* \todo Translate the ipaConfig and response */
>  	return ctx_->ops->start(ctx_);
>  }
>
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 23fc56d7..282c3c0f 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -536,10 +536,17 @@ namespace libcamera {
>  /**
>   * \fn IPAInterface::start()
>   * \brief Start the IPA
> + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> + * \param[out] result Pipeline-handler-specific configuration result

These come from the 'configuration' operation
I would:

\param[in] data Protocol-specific data for the start operation
\param[out] result Result of the start operation

>   *
>   * This method informs the IPA module that the camera is about to be started.
>   * The IPA module shall prepare any resources it needs to operate.
>   *
> + * The \a 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.
> + *

Copied from configure too, replace 'configure()' at least.

>   * \return 0 on success or a negative error code otherwise
>   */
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index ddb30e49..29bcef07 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>  	}
>
>  	/* Start the IPA. */
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaConfig = {}, result = {};
> +	ipaConfig.controls.emplace_back(*controls);

What if controls is nullptr ?

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

Unrelated but I think it's ok

>
>  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2e8d2930..a6c82a48 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>  	if (ret)
>  		return ret;
>
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaConfig = {};
> +	ret = data->ipa_->start(ipaConfig, nullptr);

If you want to use a single IPAOperationData for start and configure,
please rename it (here and in other pipeline handlers).

Thanks
  j


>  	if (ret) {
>  		freeBuffers(camera);
>  		LOG(RkISP1, Error)
> @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>
> -	IPAOperationData ipaConfig;
> +	ipaConfig = {};
>  	data->ipa_->configure(sensorInfo, streamConfig, entityControls,
>  			      ipaConfig, nullptr);
>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index d81b8598..b4773cf5 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con
>  	if (ret < 0)
>  		return ret;
>
> -	ret = data->ipa_->start();
> +	IPAOperationData ipaConfig = {};
> +	ret = data->ipa_->start(ipaConfig, nullptr);
>  	if (ret) {
>  		data->video_->releaseBuffers();
>  		return ret;
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index b78a0e45..619976e5 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -30,7 +30,8 @@ public:
>  	{
>  		return 0;
>  	}
> -	int start() override { return 0; }
> +	int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void stop() override {}
>  	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
>  		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index eead2883..9a81b6e7 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -26,7 +26,8 @@ public:
>  	IPAProxyThread(IPAModule *ipam);
>
>  	int init(const IPASettings &settings) override;
> -	int start() override;
> +	int start(const IPAOperationData &ipaConfig,
> +		  IPAOperationData *result) override;
>  	void stop() override;
>
>  	void configure(const CameraSensorInfo &sensorInfo,
> @@ -50,9 +51,9 @@ private:
>  			ipa_ = ipa;
>  		}
>
> -		int start()
> +		int start(const IPAOperationData &ipaConfig, IPAOperationData *result)
>  		{
> -			return ipa_->start();
> +			return ipa_->start(ipaConfig, result);
>  		}
>
>  		void stop()
> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)
>  	return 0;
>  }
>
> -int IPAProxyThread::start()
> +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> +			  IPAOperationData *result)
>  {
>  	running_ = true;
>  	thread_.start();
>
> -	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> +	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,
> +				   ipaConfig, result);
>  }
>
>  void IPAProxyThread::stop()
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 67488409..03b7f0ad 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -120,7 +120,8 @@ protected:
>  		}
>
>  		/* Test start of IPA module. */
> -		ipa_->start();
> +		IPAOperationData ipaConfig = {};
> +		ipa_->start(ipaConfig, nullptr);
>  		timer.start(1000);
>  		while (timer.isRunning() && trace_ != IPAOperationStart)
>  			dispatcher->processEvents();
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cb..7b8ae77b 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -56,7 +56,8 @@ public:
>  		return 0;
>  	}
>
> -	int start() override
> +	int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> +		  [[maybe_unused]] IPAOperationData *result) override
>  	{
>  		report(Op_start, TestPass);
>  		return 0;
> @@ -376,7 +377,7 @@ protected:
>  			return TestFail;
>  		}
>
> -		ret = INVOKE(start);
> +		ret = INVOKE(start, ipaConfig, nullptr);
>  		if (ret == TestFail) {
>  			cerr << "Failed to run start()";
>  			return TestFail;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for your review feedback.

On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Naush,
>
> On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
> > This change allows controls passed into PipelineHandler::start to be
> > forwarded onto IPAInterface::start(). We also add a return channel if the
> > pipeline handler must action any of these controls, e.g. setting the
> > analogue gain or shutter speed in the sensor device.
> >
> > todo: The IPA interface wrapper needs a translation for passing
> > IPAOperationData into start() and configure()
>
> This is 'problematic' as it makes isolated IPAs deviate from other
> ones. We have the same issue with configure() which has a very similar
> \todo item :/
>
> Now, this is not an issue right now, and I think with Paul's IPC
> we'll get it for free.
>
> Other comments below:
>
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
> >  include/libcamera/ipa/ipa_interface.h              |  3 ++-
> >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
> >  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
> >  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
> >  src/ipa/vimc/vimc.cpp                              |  6 ++++--
> >  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
> >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
> >  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
> >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
> >  test/ipa/ipa_interface_test.cpp                    |  3 ++-
> >  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
> >  15 files changed, 50 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> b/include/libcamera/internal/ipa_context_wrapper.h
> > index 8f767e84..1878a615 100644
> > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > @@ -20,7 +20,8 @@ public:
> >       ~IPAContextWrapper();
> >
> >       int init(const IPASettings &settings) override;
> > -     int start() override;
> > +     int start(const IPAOperationData &ipaConfig,
>
> 'ipaConfig' is only used in configure.
> It's generally just called 'data' and I would use it here too.
>

Sure, will fix.  I will also s/ ipaConfig/data/ in all instances of start()
that have been changed in this patch.


> > +               IPAOperationData *result) override;
> >       void stop() override;
> >       void configure(const CameraSensorInfo &sensorInfo,
> >                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> > diff --git a/include/libcamera/ipa/ipa_interface.h
> b/include/libcamera/ipa/ipa_interface.h
> > index 322b7079..b44e2538 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -153,7 +153,8 @@ public:
> >       virtual ~IPAInterface() = default;
> >
> >       virtual int init(const IPASettings &settings) = 0;
> > -     virtual int start() = 0;
> > +     virtual int start(const IPAOperationData &ipaConfig,
> > +                       IPAOperationData *result) = 0;
> >       virtual void stop() = 0;
> >
> >       virtual void configure(const CameraSensorInfo &sensorInfo,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index cee532e3..2b305b56 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context
> *_ctx)
> >  {
> >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> *>(_ctx);
> >
> > -     return ctx->ipa_->start();
> > +     /* \todo Translate the ipaConfig and result. */
> > +     IPAOperationData ipaConfig = {};
> > +     return ctx->ipa_->start(ipaConfig, nullptr);
> >  }
> >
> >  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f338ff8b..7a07b477 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -77,7 +77,8 @@ public:
> >       }
> >
> >       int init(const IPASettings &settings) override;
> > -     int start() override { return 0; }
> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > +               [[maybe_unused]] IPAOperationData *result) override {
> return 0; }
> >       void stop() override {}
> >
> >       void configure(const CameraSensorInfo &sensorInfo,
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 07d7f1b2..0b8a9096 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -37,7 +37,8 @@ public:
> >       {
> >               return 0;
> >       }
> > -     int start() override { return 0; }
> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > +               [[maybe_unused]] IPAOperationData *result) override {
> return 0; }
> >       void stop() override {}
> >
> >       void configure(const CameraSensorInfo &info,
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index cf841135..ed26331d 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -34,7 +34,8 @@ public:
> >
> >       int init(const IPASettings &settings) override;
> >
> > -     int start() override;
> > +     int start(const IPAOperationData &ipaConfig,
> > +               IPAOperationData *result) override;
> >       void stop() override;
> >
> >       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
> >       return 0;
> >  }
> >
> > -int IPAVimc::start()
> > +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                [[maybe_unused]] IPAOperationData *result)
>
>                    ^ is this mis-aligned or is it my email client ?
>                    There are more in this patch if that's the case.
>

I think this may be an email client rendering thing.  They do look correct
to me, and I would expect the style checker to complain about
misalignment.  However, I will check though once again.


> >  {
> >       trace(IPAOperationStart);
> >
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> b/src/libcamera/ipa_context_wrapper.cpp
> > index 231300ce..8c4ec40a 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings
> &settings)
> >       return 0;
> >  }
> >
> > -int IPAContextWrapper::start()
> > +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> > +                          IPAOperationData *result)
> >  {
> >       if (intf_)
> > -             return intf_->start();
> > +             return intf_->start(ipaConfig, result);
> >
> >       if (!ctx_)
> >               return 0;
> >
> > +     /* \todo Translate the ipaConfig and response */
> >       return ctx_->ops->start(ctx_);
> >  }
> >
> > diff --git a/src/libcamera/ipa_interface.cpp
> b/src/libcamera/ipa_interface.cpp
> > index 23fc56d7..282c3c0f 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -536,10 +536,17 @@ namespace libcamera {
> >  /**
> >   * \fn IPAInterface::start()
> >   * \brief Start the IPA
> > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > + * \param[out] result Pipeline-handler-specific configuration result
>
> These come from the 'configuration' operation
> I would:
>
> \param[in] data Protocol-specific data for the start operation
> \param[out] result Result of the start operation
>

Fixed.


>
> >   *
> >   * This method informs the IPA module that the camera is about to be
> started.
> >   * The IPA module shall prepare any resources it needs to operate.
> >   *
> > + * The \a 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.
> > + *
>
> Copied from configure too, replace 'configure()' at least.
>

Oops, replaced with start()!


>
> >   * \return 0 on success or a negative error code otherwise
> >   */
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index ddb30e49..29bcef07 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera,
> [[maybe_unused]] ControlList *cont
> >       }
> >
> >       /* Start the IPA. */
> > -     ret = data->ipa_->start();
> > +     IPAOperationData ipaConfig = {}, result = {};
> > +     ipaConfig.controls.emplace_back(*controls);
>
> What if controls is nullptr ?
>

Hmmm.... I am a bit confused.  My version of the code has:

 if (controls) {
       ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
       ipaConfig.controls.emplace_back(*controls);
}

But clearly my patch in the email does not :(  Once I complete all your
suggested fixups, I will send an updated patchset!


> > +     ret = data->ipa_->start(ipaConfig, &result);
> >       if (ret) {
> >               LOG(RPI, Error)
> >                       << "Failed to start IPA for " << camera->id();
> > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >       }
> >
> >       /* Ready the IPA - it must know about the sensor resolution. */
> > -     IPAOperationData result;
> > +     IPAOperationData result = {};
>
> Unrelated but I think it's ok
>

This was an update requested by David in his review.  Just to be safe :)


>
> >
> >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> >                       &result);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 2e8d2930..a6c82a48 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> [[maybe_unused]] ControlList *c
> >       if (ret)
> >               return ret;
> >
> > -     ret = data->ipa_->start();
> > +     IPAOperationData ipaConfig = {};
> > +     ret = data->ipa_->start(ipaConfig, nullptr);
>
> If you want to use a single IPAOperationData for start and configure,
> please rename it (here and in other pipeline handlers).
>

I would use data (as with the other IPAOperationData parameters passed into
start), but we already have a declaration of RkISP1CameraData *data.  Any
objections to renaming it ipaData?

Regards,
Naush



>
> Thanks
>   j
>
>
> >       if (ret) {
> >               freeBuffers(camera);
> >               LOG(RkISP1, Error)
> > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> [[maybe_unused]] ControlList *c
> >       std::map<unsigned int, const ControlInfoMap &> entityControls;
> >       entityControls.emplace(0, data->sensor_->controls());
> >
> > -     IPAOperationData ipaConfig;
> > +     ipaConfig = {};
> >       data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> >                             ipaConfig, nullptr);
> >
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
> b/src/libcamera/pipeline/vimc/vimc.cpp
> > index d81b8598..b4773cf5 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera,
> [[maybe_unused]] ControlList *con
> >       if (ret < 0)
> >               return ret;
> >
> > -     ret = data->ipa_->start();
> > +     IPAOperationData ipaConfig = {};
> > +     ret = data->ipa_->start(ipaConfig, nullptr);
> >       if (ret) {
> >               data->video_->releaseBuffers();
> >               return ret;
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index b78a0e45..619976e5 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -30,7 +30,8 @@ public:
> >       {
> >               return 0;
> >       }
> > -     int start() override { return 0; }
> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > +               [[maybe_unused]] IPAOperationData *result) override {
> return 0; }
> >       void stop() override {}
> >       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> >                      [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index eead2883..9a81b6e7 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -26,7 +26,8 @@ public:
> >       IPAProxyThread(IPAModule *ipam);
> >
> >       int init(const IPASettings &settings) override;
> > -     int start() override;
> > +     int start(const IPAOperationData &ipaConfig,
> > +               IPAOperationData *result) override;
> >       void stop() override;
> >
> >       void configure(const CameraSensorInfo &sensorInfo,
> > @@ -50,9 +51,9 @@ private:
> >                       ipa_ = ipa;
> >               }
> >
> > -             int start()
> > +             int start(const IPAOperationData &ipaConfig,
> IPAOperationData *result)
> >               {
> > -                     return ipa_->start();
> > +                     return ipa_->start(ipaConfig, result);
> >               }
> >
> >               void stop()
> > @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings
> &settings)
> >       return 0;
> >  }
> >
> > -int IPAProxyThread::start()
> > +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> > +                       IPAOperationData *result)
> >  {
> >       running_ = true;
> >       thread_.start();
> >
> > -     return proxy_.invokeMethod(&ThreadProxy::start,
> ConnectionTypeBlocking);
> > +     return proxy_.invokeMethod(&ThreadProxy::start,
> ConnectionTypeBlocking,
> > +                                ipaConfig, result);
> >  }
> >
> >  void IPAProxyThread::stop()
> > diff --git a/test/ipa/ipa_interface_test.cpp
> b/test/ipa/ipa_interface_test.cpp
> > index 67488409..03b7f0ad 100644
> > --- a/test/ipa/ipa_interface_test.cpp
> > +++ b/test/ipa/ipa_interface_test.cpp
> > @@ -120,7 +120,8 @@ protected:
> >               }
> >
> >               /* Test start of IPA module. */
> > -             ipa_->start();
> > +             IPAOperationData ipaConfig = {};
> > +             ipa_->start(ipaConfig, nullptr);
> >               timer.start(1000);
> >               while (timer.isRunning() && trace_ != IPAOperationStart)
> >                       dispatcher->processEvents();
> > diff --git a/test/ipa/ipa_wrappers_test.cpp
> b/test/ipa/ipa_wrappers_test.cpp
> > index 59d991cb..7b8ae77b 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -56,7 +56,8 @@ public:
> >               return 0;
> >       }
> >
> > -     int start() override
> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > +               [[maybe_unused]] IPAOperationData *result) override
> >       {
> >               report(Op_start, TestPass);
> >               return 0;
> > @@ -376,7 +377,7 @@ protected:
> >                       return TestFail;
> >               }
> >
> > -             ret = INVOKE(start);
> > +             ret = INVOKE(start, ipaConfig, nullptr);
> >               if (ret == TestFail) {
> >                       cerr << "Failed to run start()";
> >                       return TestFail;
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi Jacopo,


On Fri, 4 Dec 2020 at 13:06, Naushir Patuck <naush@raspberrypi.com> wrote:

> Hi Jacopo,
>
> Thank you for your review feedback.
>
> On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
>> Hi Naush,
>>
>> On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
>> > This change allows controls passed into PipelineHandler::start to be
>> > forwarded onto IPAInterface::start(). We also add a return channel if
>> the
>> > pipeline handler must action any of these controls, e.g. setting the
>> > analogue gain or shutter speed in the sensor device.
>> >
>> > todo: The IPA interface wrapper needs a translation for passing
>> > IPAOperationData into start() and configure()
>>
>> This is 'problematic' as it makes isolated IPAs deviate from other
>> ones. We have the same issue with configure() which has a very similar
>> \todo item :/
>>
>> Now, this is not an issue right now, and I think with Paul's IPC
>> we'll get it for free.
>>
>> Other comments below:
>>
>> >
>> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>> > Tested-by: David Plowman <david.plowman@raspberrypi.com>
>> > ---
>> >  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
>> >  include/libcamera/ipa/ipa_interface.h              |  3 ++-
>> >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
>> >  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
>> >  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
>> >  src/ipa/vimc/vimc.cpp                              |  6 ++++--
>> >  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
>> >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
>> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
>> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
>> >  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
>> >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
>> >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
>> >  test/ipa/ipa_interface_test.cpp                    |  3 ++-
>> >  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
>> >  15 files changed, 50 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
>> b/include/libcamera/internal/ipa_context_wrapper.h
>> > index 8f767e84..1878a615 100644
>> > --- a/include/libcamera/internal/ipa_context_wrapper.h
>> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
>> > @@ -20,7 +20,8 @@ public:
>> >       ~IPAContextWrapper();
>> >
>> >       int init(const IPASettings &settings) override;
>> > -     int start() override;
>> > +     int start(const IPAOperationData &ipaConfig,
>>
>> 'ipaConfig' is only used in configure.
>> It's generally just called 'data' and I would use it here too.
>>
>
> Sure, will fix.  I will also s/ ipaConfig/data/ in all instances of
> start() that have been changed in this patch.
>
>
>> > +               IPAOperationData *result) override;
>> >       void stop() override;
>> >       void configure(const CameraSensorInfo &sensorInfo,
>> >                      const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> > diff --git a/include/libcamera/ipa/ipa_interface.h
>> b/include/libcamera/ipa/ipa_interface.h
>> > index 322b7079..b44e2538 100644
>> > --- a/include/libcamera/ipa/ipa_interface.h
>> > +++ b/include/libcamera/ipa/ipa_interface.h
>> > @@ -153,7 +153,8 @@ public:
>> >       virtual ~IPAInterface() = default;
>> >
>> >       virtual int init(const IPASettings &settings) = 0;
>> > -     virtual int start() = 0;
>> > +     virtual int start(const IPAOperationData &ipaConfig,
>> > +                       IPAOperationData *result) = 0;
>> >       virtual void stop() = 0;
>> >
>> >       virtual void configure(const CameraSensorInfo &sensorInfo,
>> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
>> b/src/ipa/libipa/ipa_interface_wrapper.cpp
>> > index cee532e3..2b305b56 100644
>> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
>> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
>> > @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context
>> *_ctx)
>> >  {
>> >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
>> *>(_ctx);
>> >
>> > -     return ctx->ipa_->start();
>> > +     /* \todo Translate the ipaConfig and result. */
>> > +     IPAOperationData ipaConfig = {};
>> > +     return ctx->ipa_->start(ipaConfig, nullptr);
>> >  }
>> >
>> >  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> b/src/ipa/raspberrypi/raspberrypi.cpp
>> > index f338ff8b..7a07b477 100644
>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> > @@ -77,7 +77,8 @@ public:
>> >       }
>> >
>> >       int init(const IPASettings &settings) override;
>> > -     int start() override { return 0; }
>> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
>> > +               [[maybe_unused]] IPAOperationData *result) override {
>> return 0; }
>> >       void stop() override {}
>> >
>> >       void configure(const CameraSensorInfo &sensorInfo,
>> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> > index 07d7f1b2..0b8a9096 100644
>> > --- a/src/ipa/rkisp1/rkisp1.cpp
>> > +++ b/src/ipa/rkisp1/rkisp1.cpp
>> > @@ -37,7 +37,8 @@ public:
>> >       {
>> >               return 0;
>> >       }
>> > -     int start() override { return 0; }
>> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
>> > +               [[maybe_unused]] IPAOperationData *result) override {
>> return 0; }
>> >       void stop() override {}
>> >
>> >       void configure(const CameraSensorInfo &info,
>> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
>> > index cf841135..ed26331d 100644
>> > --- a/src/ipa/vimc/vimc.cpp
>> > +++ b/src/ipa/vimc/vimc.cpp
>> > @@ -34,7 +34,8 @@ public:
>> >
>> >       int init(const IPASettings &settings) override;
>> >
>> > -     int start() override;
>> > +     int start(const IPAOperationData &ipaConfig,
>> > +               IPAOperationData *result) override;
>> >       void stop() override;
>> >
>> >       void configure([[maybe_unused]] const CameraSensorInfo
>> &sensorInfo,
>> > @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
>> >       return 0;
>> >  }
>> >
>> > -int IPAVimc::start()
>> > +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,
>> > +                [[maybe_unused]] IPAOperationData *result)
>>
>>                    ^ is this mis-aligned or is it my email client ?
>>                    There are more in this patch if that's the case.
>>
>
> I think this may be an email client rendering thing.  They do look correct
> to me, and I would expect the style checker to complain about
> misalignment.  However, I will check though once again.
>
>
>> >  {
>> >       trace(IPAOperationStart);
>> >
>> > diff --git a/src/libcamera/ipa_context_wrapper.cpp
>> b/src/libcamera/ipa_context_wrapper.cpp
>> > index 231300ce..8c4ec40a 100644
>> > --- a/src/libcamera/ipa_context_wrapper.cpp
>> > +++ b/src/libcamera/ipa_context_wrapper.cpp
>> > @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings
>> &settings)
>> >       return 0;
>> >  }
>> >
>> > -int IPAContextWrapper::start()
>> > +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
>> > +                          IPAOperationData *result)
>> >  {
>> >       if (intf_)
>> > -             return intf_->start();
>> > +             return intf_->start(ipaConfig, result);
>> >
>> >       if (!ctx_)
>> >               return 0;
>> >
>> > +     /* \todo Translate the ipaConfig and response */
>> >       return ctx_->ops->start(ctx_);
>> >  }
>> >
>> > diff --git a/src/libcamera/ipa_interface.cpp
>> b/src/libcamera/ipa_interface.cpp
>> > index 23fc56d7..282c3c0f 100644
>> > --- a/src/libcamera/ipa_interface.cpp
>> > +++ b/src/libcamera/ipa_interface.cpp
>> > @@ -536,10 +536,17 @@ namespace libcamera {
>> >  /**
>> >   * \fn IPAInterface::start()
>> >   * \brief Start the IPA
>> > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
>> > + * \param[out] result Pipeline-handler-specific configuration result
>>
>> These come from the 'configuration' operation
>> I would:
>>
>> \param[in] data Protocol-specific data for the start operation
>> \param[out] result Result of the start operation
>>
>
> Fixed.
>
>
>>
>> >   *
>> >   * This method informs the IPA module that the camera is about to be
>> started.
>> >   * The IPA module shall prepare any resources it needs to operate.
>> >   *
>> > + * The \a 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.
>> > + *
>>
>> Copied from configure too, replace 'configure()' at least.
>>
>
> Oops, replaced with start()!
>
>
>>
>> >   * \return 0 on success or a negative error code otherwise
>> >   */
>> >
>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > index ddb30e49..29bcef07 100644
>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera,
>> [[maybe_unused]] ControlList *cont
>> >       }
>> >
>> >       /* Start the IPA. */
>> > -     ret = data->ipa_->start();
>> > +     IPAOperationData ipaConfig = {}, result = {};
>> > +     ipaConfig.controls.emplace_back(*controls);
>>
>> What if controls is nullptr ?
>>
>
> Hmmm.... I am a bit confused.  My version of the code has:
>
>  if (controls) {
>        ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
>        ipaConfig.controls.emplace_back(*controls);
> }
>
> But clearly my patch in the email does not :(  Once I complete all your
> suggested fixups, I will send an updated patchset!
>

Sorry, my bad, I was confusing myself.  The above code (with the check on
controls) is only introduced in patch 3/3.  I will backport the test to
this patch 2/3.

Regards,
Naush


>
>
>> > +     ret = data->ipa_->start(ipaConfig, &result);
>> >       if (ret) {
>> >               LOG(RPI, Error)
>> >                       << "Failed to start IPA for " << camera->id();
>> > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
>> CameraConfiguration *config)
>> >       }
>> >
>> >       /* Ready the IPA - it must know about the sensor resolution. */
>> > -     IPAOperationData result;
>> > +     IPAOperationData result = {};
>>
>> Unrelated but I think it's ok
>>
>
> This was an update requested by David in his review.  Just to be safe :)
>
>
>>
>> >
>> >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
>> ipaConfig,
>> >                       &result);
>> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> > index 2e8d2930..a6c82a48 100644
>> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> > @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera,
>> [[maybe_unused]] ControlList *c
>> >       if (ret)
>> >               return ret;
>> >
>> > -     ret = data->ipa_->start();
>> > +     IPAOperationData ipaConfig = {};
>> > +     ret = data->ipa_->start(ipaConfig, nullptr);
>>
>> If you want to use a single IPAOperationData for start and configure,
>> please rename it (here and in other pipeline handlers).
>>
>
> I would use data (as with the other IPAOperationData parameters passed
> into start), but we already have a declaration of RkISP1CameraData *data.
> Any objections to renaming it ipaData?
>
> Regards,
> Naush
>
>
>
>>
>> Thanks
>>   j
>>
>>
>> >       if (ret) {
>> >               freeBuffers(camera);
>> >               LOG(RkISP1, Error)
>> > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,
>> [[maybe_unused]] ControlList *c
>> >       std::map<unsigned int, const ControlInfoMap &> entityControls;
>> >       entityControls.emplace(0, data->sensor_->controls());
>> >
>> > -     IPAOperationData ipaConfig;
>> > +     ipaConfig = {};
>> >       data->ipa_->configure(sensorInfo, streamConfig, entityControls,
>> >                             ipaConfig, nullptr);
>> >
>> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
>> b/src/libcamera/pipeline/vimc/vimc.cpp
>> > index d81b8598..b4773cf5 100644
>> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> > @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera,
>> [[maybe_unused]] ControlList *con
>> >       if (ret < 0)
>> >               return ret;
>> >
>> > -     ret = data->ipa_->start();
>> > +     IPAOperationData ipaConfig = {};
>> > +     ret = data->ipa_->start(ipaConfig, nullptr);
>> >       if (ret) {
>> >               data->video_->releaseBuffers();
>> >               return ret;
>> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
>> b/src/libcamera/proxy/ipa_proxy_linux.cpp
>> > index b78a0e45..619976e5 100644
>> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
>> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
>> > @@ -30,7 +30,8 @@ public:
>> >       {
>> >               return 0;
>> >       }
>> > -     int start() override { return 0; }
>> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
>> > +               [[maybe_unused]] IPAOperationData *result) override {
>> return 0; }
>> >       void stop() override {}
>> >       void configure([[maybe_unused]] const CameraSensorInfo
>> &sensorInfo,
>> >                      [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
>> b/src/libcamera/proxy/ipa_proxy_thread.cpp
>> > index eead2883..9a81b6e7 100644
>> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
>> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
>> > @@ -26,7 +26,8 @@ public:
>> >       IPAProxyThread(IPAModule *ipam);
>> >
>> >       int init(const IPASettings &settings) override;
>> > -     int start() override;
>> > +     int start(const IPAOperationData &ipaConfig,
>> > +               IPAOperationData *result) override;
>> >       void stop() override;
>> >
>> >       void configure(const CameraSensorInfo &sensorInfo,
>> > @@ -50,9 +51,9 @@ private:
>> >                       ipa_ = ipa;
>> >               }
>> >
>> > -             int start()
>> > +             int start(const IPAOperationData &ipaConfig,
>> IPAOperationData *result)
>> >               {
>> > -                     return ipa_->start();
>> > +                     return ipa_->start(ipaConfig, result);
>> >               }
>> >
>> >               void stop()
>> > @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings
>> &settings)
>> >       return 0;
>> >  }
>> >
>> > -int IPAProxyThread::start()
>> > +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
>> > +                       IPAOperationData *result)
>> >  {
>> >       running_ = true;
>> >       thread_.start();
>> >
>> > -     return proxy_.invokeMethod(&ThreadProxy::start,
>> ConnectionTypeBlocking);
>> > +     return proxy_.invokeMethod(&ThreadProxy::start,
>> ConnectionTypeBlocking,
>> > +                                ipaConfig, result);
>> >  }
>> >
>> >  void IPAProxyThread::stop()
>> > diff --git a/test/ipa/ipa_interface_test.cpp
>> b/test/ipa/ipa_interface_test.cpp
>> > index 67488409..03b7f0ad 100644
>> > --- a/test/ipa/ipa_interface_test.cpp
>> > +++ b/test/ipa/ipa_interface_test.cpp
>> > @@ -120,7 +120,8 @@ protected:
>> >               }
>> >
>> >               /* Test start of IPA module. */
>> > -             ipa_->start();
>> > +             IPAOperationData ipaConfig = {};
>> > +             ipa_->start(ipaConfig, nullptr);
>> >               timer.start(1000);
>> >               while (timer.isRunning() && trace_ != IPAOperationStart)
>> >                       dispatcher->processEvents();
>> > diff --git a/test/ipa/ipa_wrappers_test.cpp
>> b/test/ipa/ipa_wrappers_test.cpp
>> > index 59d991cb..7b8ae77b 100644
>> > --- a/test/ipa/ipa_wrappers_test.cpp
>> > +++ b/test/ipa/ipa_wrappers_test.cpp
>> > @@ -56,7 +56,8 @@ public:
>> >               return 0;
>> >       }
>> >
>> > -     int start() override
>> > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
>> > +               [[maybe_unused]] IPAOperationData *result) override
>> >       {
>> >               report(Op_start, TestPass);
>> >               return 0;
>> > @@ -376,7 +377,7 @@ protected:
>> >                       return TestFail;
>> >               }
>> >
>> > -             ret = INVOKE(start);
>> > +             ret = INVOKE(start, ipaConfig, nullptr);
>> >               if (ret == TestFail) {
>> >                       cerr << "Failed to run start()";
>> >                       return TestFail;
>> > --
>> > 2.25.1
>> >
>> > _______________________________________________
>> > libcamera-devel mailing list
>> > libcamera-devel@lists.libcamera.org
>> > https://lists.libcamera.org/listinfo/libcamera-devel
>>
>

Hi Nuash,

On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> Thank you for your review feedback.
>
> On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> > Hi Naush,
> >
> > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
> > > This change allows controls passed into PipelineHandler::start to be
> > > forwarded onto IPAInterface::start(). We also add a return channel if the
> > > pipeline handler must action any of these controls, e.g. setting the
> > > analogue gain or shutter speed in the sensor device.
> > >
> > > todo: The IPA interface wrapper needs a translation for passing
> > > IPAOperationData into start() and configure()
> >
> > This is 'problematic' as it makes isolated IPAs deviate from other
> > ones. We have the same issue with configure() which has a very similar
> > \todo item :/
> >
> > Now, this is not an issue right now, and I think with Paul's IPC
> > we'll get it for free.
> >
> > Other comments below:
> >
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
> > >  include/libcamera/ipa/ipa_interface.h              |  3 ++-
> > >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
> > >  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
> > >  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
> > >  src/ipa/vimc/vimc.cpp                              |  6 ++++--
> > >  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
> > >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
> > >  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
> > >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
> > >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13 ++++++++-----
> > >  test/ipa/ipa_interface_test.cpp                    |  3 ++-
> > >  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
> > >  15 files changed, 50 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> > b/include/libcamera/internal/ipa_context_wrapper.h
> > > index 8f767e84..1878a615 100644
> > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > @@ -20,7 +20,8 @@ public:
> > >       ~IPAContextWrapper();
> > >
> > >       int init(const IPASettings &settings) override;
> > > -     int start() override;
> > > +     int start(const IPAOperationData &ipaConfig,
> >
> > 'ipaConfig' is only used in configure.
> > It's generally just called 'data' and I would use it here too.
> >
>
> Sure, will fix.  I will also s/ ipaConfig/data/ in all instances of start()
> that have been changed in this patch.

Thanks! Other names are good to, just drop 'Config'

>
>
> > > +               IPAOperationData *result) override;
> > >       void stop() override;
> > >       void configure(const CameraSensorInfo &sensorInfo,
> > >                      const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > > diff --git a/include/libcamera/ipa/ipa_interface.h
> > b/include/libcamera/ipa/ipa_interface.h
> > > index 322b7079..b44e2538 100644
> > > --- a/include/libcamera/ipa/ipa_interface.h
> > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > @@ -153,7 +153,8 @@ public:
> > >       virtual ~IPAInterface() = default;
> > >
> > >       virtual int init(const IPASettings &settings) = 0;
> > > -     virtual int start() = 0;
> > > +     virtual int start(const IPAOperationData &ipaConfig,
> > > +                       IPAOperationData *result) = 0;
> > >       virtual void stop() = 0;
> > >
> > >       virtual void configure(const CameraSensorInfo &sensorInfo,
> > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > index cee532e3..2b305b56 100644
> > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context
> > *_ctx)
> > >  {
> > >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> > *>(_ctx);
> > >
> > > -     return ctx->ipa_->start();
> > > +     /* \todo Translate the ipaConfig and result. */
> > > +     IPAOperationData ipaConfig = {};
> > > +     return ctx->ipa_->start(ipaConfig, nullptr);
> > >  }
> > >
> > >  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index f338ff8b..7a07b477 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -77,7 +77,8 @@ public:
> > >       }
> > >
> > >       int init(const IPASettings &settings) override;
> > > -     int start() override { return 0; }
> > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > +               [[maybe_unused]] IPAOperationData *result) override {
> > return 0; }
> > >       void stop() override {}
> > >
> > >       void configure(const CameraSensorInfo &sensorInfo,
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 07d7f1b2..0b8a9096 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -37,7 +37,8 @@ public:
> > >       {
> > >               return 0;
> > >       }
> > > -     int start() override { return 0; }
> > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > +               [[maybe_unused]] IPAOperationData *result) override {
> > return 0; }
> > >       void stop() override {}
> > >
> > >       void configure(const CameraSensorInfo &info,
> > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > index cf841135..ed26331d 100644
> > > --- a/src/ipa/vimc/vimc.cpp
> > > +++ b/src/ipa/vimc/vimc.cpp
> > > @@ -34,7 +34,8 @@ public:
> > >
> > >       int init(const IPASettings &settings) override;
> > >
> > > -     int start() override;
> > > +     int start(const IPAOperationData &ipaConfig,
> > > +               IPAOperationData *result) override;
> > >       void stop() override;
> > >
> > >       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > > @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
> > >       return 0;
> > >  }
> > >
> > > -int IPAVimc::start()
> > > +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > +                [[maybe_unused]] IPAOperationData *result)
> >
> >                    ^ is this mis-aligned or is it my email client ?
> >                    There are more in this patch if that's the case.
> >
>
> I think this may be an email client rendering thing.  They do look correct
> to me, and I would expect the style checker to complain about
> misalignment.  However, I will check though once again.
>

Might be, I just pointed it out as I usually see indentation off 1
space with my client and here I see 3. I haven't applied the patches,
so I trust your opinion!

>
> > >  {
> > >       trace(IPAOperationStart);
> > >
> > > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> > b/src/libcamera/ipa_context_wrapper.cpp
> > > index 231300ce..8c4ec40a 100644
> > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings
> > &settings)
> > >       return 0;
> > >  }
> > >
> > > -int IPAContextWrapper::start()
> > > +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> > > +                          IPAOperationData *result)
> > >  {
> > >       if (intf_)
> > > -             return intf_->start();
> > > +             return intf_->start(ipaConfig, result);
> > >
> > >       if (!ctx_)
> > >               return 0;
> > >
> > > +     /* \todo Translate the ipaConfig and response */
> > >       return ctx_->ops->start(ctx_);
> > >  }
> > >
> > > diff --git a/src/libcamera/ipa_interface.cpp
> > b/src/libcamera/ipa_interface.cpp
> > > index 23fc56d7..282c3c0f 100644
> > > --- a/src/libcamera/ipa_interface.cpp
> > > +++ b/src/libcamera/ipa_interface.cpp
> > > @@ -536,10 +536,17 @@ namespace libcamera {
> > >  /**
> > >   * \fn IPAInterface::start()
> > >   * \brief Start the IPA
> > > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > + * \param[out] result Pipeline-handler-specific configuration result
> >
> > These come from the 'configuration' operation
> > I would:
> >
> > \param[in] data Protocol-specific data for the start operation
> > \param[out] result Result of the start operation
> >
>
> Fixed.

Thanks

>
>
> >
> > >   *
> > >   * This method informs the IPA module that the camera is about to be
> > started.
> > >   * The IPA module shall prepare any resources it needs to operate.
> > >   *
> > > + * The \a 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.
> > > + *
> >
> > Copied from configure too, replace 'configure()' at least.
> >
>
> Oops, replaced with start()!
>
>
> >
> > >   * \return 0 on success or a negative error code otherwise
> > >   */
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index ddb30e49..29bcef07 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera,
> > [[maybe_unused]] ControlList *cont
> > >       }
> > >
> > >       /* Start the IPA. */
> > > -     ret = data->ipa_->start();
> > > +     IPAOperationData ipaConfig = {}, result = {};
> > > +     ipaConfig.controls.emplace_back(*controls);
> >
> > What if controls is nullptr ?
> >
>
> Hmmm.... I am a bit confused.  My version of the code has:
>
>  if (controls) {
>        ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
>        ipaConfig.controls.emplace_back(*controls);
> }
>
> But clearly my patch in the email does not :(  Once I complete all your
> suggested fixups, I will send an updated patchset!
>

Thanks

>
> > > +     ret = data->ipa_->start(ipaConfig, &result);
> > >       if (ret) {
> > >               LOG(RPI, Error)
> > >                       << "Failed to start IPA for " << camera->id();
> > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
> > CameraConfiguration *config)
> > >       }
> > >
> > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > -     IPAOperationData result;
> > > +     IPAOperationData result = {};
> >
> > Unrelated but I think it's ok
> >
>
> This was an update requested by David in his review.  Just to be safe :)
>

I agree it is an opportune fix, maybe not strictly part of this patch.
But it's ok :)

>
> >
> > >
> > >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > ipaConfig,
> > >                       &result);
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 2e8d2930..a6c82a48 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> > [[maybe_unused]] ControlList *c
> > >       if (ret)
> > >               return ret;
> > >
> > > -     ret = data->ipa_->start();
> > > +     IPAOperationData ipaConfig = {};
> > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> >
> > If you want to use a single IPAOperationData for start and configure,
> > please rename it (here and in other pipeline handlers).
> >
>
> I would use data (as with the other IPAOperationData parameters passed into
> start), but we already have a declaration of RkISP1CameraData *data.  Any
> objections to renaming it ipaData?
>

Sounds good to me!
Thanks for addressing all comments!

> Regards,
> Naush
>
>
>
> >
> > Thanks
> >   j
> >
> >
> > >       if (ret) {
> > >               freeBuffers(camera);
> > >               LOG(RkISP1, Error)
> > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> > [[maybe_unused]] ControlList *c
> > >       std::map<unsigned int, const ControlInfoMap &> entityControls;
> > >       entityControls.emplace(0, data->sensor_->controls());
> > >
> > > -     IPAOperationData ipaConfig;
> > > +     ipaConfig = {};
> > >       data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > >                             ipaConfig, nullptr);
> > >
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
> > b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index d81b8598..b4773cf5 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera,
> > [[maybe_unused]] ControlList *con
> > >       if (ret < 0)
> > >               return ret;
> > >
> > > -     ret = data->ipa_->start();
> > > +     IPAOperationData ipaConfig = {};
> > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > >       if (ret) {
> > >               data->video_->releaseBuffers();
> > >               return ret;
> > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > index b78a0e45..619976e5 100644
> > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > @@ -30,7 +30,8 @@ public:
> > >       {
> > >               return 0;
> > >       }
> > > -     int start() override { return 0; }
> > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > +               [[maybe_unused]] IPAOperationData *result) override {
> > return 0; }
> > >       void stop() override {}
> > >       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > >                      [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > index eead2883..9a81b6e7 100644
> > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > @@ -26,7 +26,8 @@ public:
> > >       IPAProxyThread(IPAModule *ipam);
> > >
> > >       int init(const IPASettings &settings) override;
> > > -     int start() override;
> > > +     int start(const IPAOperationData &ipaConfig,
> > > +               IPAOperationData *result) override;
> > >       void stop() override;
> > >
> > >       void configure(const CameraSensorInfo &sensorInfo,
> > > @@ -50,9 +51,9 @@ private:
> > >                       ipa_ = ipa;
> > >               }
> > >
> > > -             int start()
> > > +             int start(const IPAOperationData &ipaConfig,
> > IPAOperationData *result)
> > >               {
> > > -                     return ipa_->start();
> > > +                     return ipa_->start(ipaConfig, result);
> > >               }
> > >
> > >               void stop()
> > > @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings
> > &settings)
> > >       return 0;
> > >  }
> > >
> > > -int IPAProxyThread::start()
> > > +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> > > +                       IPAOperationData *result)
> > >  {
> > >       running_ = true;
> > >       thread_.start();
> > >
> > > -     return proxy_.invokeMethod(&ThreadProxy::start,
> > ConnectionTypeBlocking);
> > > +     return proxy_.invokeMethod(&ThreadProxy::start,
> > ConnectionTypeBlocking,
> > > +                                ipaConfig, result);
> > >  }
> > >
> > >  void IPAProxyThread::stop()
> > > diff --git a/test/ipa/ipa_interface_test.cpp
> > b/test/ipa/ipa_interface_test.cpp
> > > index 67488409..03b7f0ad 100644
> > > --- a/test/ipa/ipa_interface_test.cpp
> > > +++ b/test/ipa/ipa_interface_test.cpp
> > > @@ -120,7 +120,8 @@ protected:
> > >               }
> > >
> > >               /* Test start of IPA module. */
> > > -             ipa_->start();
> > > +             IPAOperationData ipaConfig = {};
> > > +             ipa_->start(ipaConfig, nullptr);
> > >               timer.start(1000);
> > >               while (timer.isRunning() && trace_ != IPAOperationStart)
> > >                       dispatcher->processEvents();
> > > diff --git a/test/ipa/ipa_wrappers_test.cpp
> > b/test/ipa/ipa_wrappers_test.cpp
> > > index 59d991cb..7b8ae77b 100644
> > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > @@ -56,7 +56,8 @@ public:
> > >               return 0;
> > >       }
> > >
> > > -     int start() override
> > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > +               [[maybe_unused]] IPAOperationData *result) override
> > >       {
> > >               report(Op_start, TestPass);
> > >               return 0;
> > > @@ -376,7 +377,7 @@ protected:
> > >                       return TestFail;
> > >               }
> > >
> > > -             ret = INVOKE(start);
> > > +             ret = INVOKE(start, ipaConfig, nullptr);
> > >               if (ret == TestFail) {
> > >                       cerr << "Failed to run start()";
> > >                       return TestFail;
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >

Hi Jacopo,


On Fri, 4 Dec 2020 at 13:20, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Nuash,
>
> On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > Thank you for your review feedback.
> >
> > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > > Hi Naush,
> > >
> > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
> > > > This change allows controls passed into PipelineHandler::start to be
> > > > forwarded onto IPAInterface::start(). We also add a return channel
> if the
> > > > pipeline handler must action any of these controls, e.g. setting the
> > > > analogue gain or shutter speed in the sensor device.
> > > >
> > > > todo: The IPA interface wrapper needs a translation for passing
> > > > IPAOperationData into start() and configure()
> > >
> > > This is 'problematic' as it makes isolated IPAs deviate from other
> > > ones. We have the same issue with configure() which has a very similar
> > > \todo item :/
> > >
> > > Now, this is not an issue right now, and I think with Paul's IPC
> > > we'll get it for free.
> > >
> > > Other comments below:
> > >
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
> > > >  include/libcamera/ipa/ipa_interface.h              |  3 ++-
> > > >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
> > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
> > > >  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
> > > >  src/ipa/vimc/vimc.cpp                              |  6 ++++--
> > > >  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
> > > >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
> > > >  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
> > > >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
> > > >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13
> ++++++++-----
> > > >  test/ipa/ipa_interface_test.cpp                    |  3 ++-
> > > >  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
> > > >  15 files changed, 50 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> > > b/include/libcamera/internal/ipa_context_wrapper.h
> > > > index 8f767e84..1878a615 100644
> > > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > > @@ -20,7 +20,8 @@ public:
> > > >       ~IPAContextWrapper();
> > > >
> > > >       int init(const IPASettings &settings) override;
> > > > -     int start() override;
> > > > +     int start(const IPAOperationData &ipaConfig,
> > >
> > > 'ipaConfig' is only used in configure.
> > > It's generally just called 'data' and I would use it here too.
> > >
> >
> > Sure, will fix.  I will also s/ ipaConfig/data/ in all instances of
> start()
> > that have been changed in this patch.
>
> Thanks! Other names are good to, just drop 'Config'
>

Could you clarify the above comment please?  I was intending to use "data"
for all  IPAOperationData variables that have been introduced in the
start() method in this patch.  Is your suggestion to use "ipa" instead of
"data"?

Regards,
Naush



>
> >
> >
> > > > +               IPAOperationData *result) override;
> > > >       void stop() override;
> > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > >                      const std::map<unsigned int, IPAStream>
> > > &streamConfig,
> > > > diff --git a/include/libcamera/ipa/ipa_interface.h
> > > b/include/libcamera/ipa/ipa_interface.h
> > > > index 322b7079..b44e2538 100644
> > > > --- a/include/libcamera/ipa/ipa_interface.h
> > > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > > @@ -153,7 +153,8 @@ public:
> > > >       virtual ~IPAInterface() = default;
> > > >
> > > >       virtual int init(const IPASettings &settings) = 0;
> > > > -     virtual int start() = 0;
> > > > +     virtual int start(const IPAOperationData &ipaConfig,
> > > > +                       IPAOperationData *result) = 0;
> > > >       virtual void stop() = 0;
> > > >
> > > >       virtual void configure(const CameraSensorInfo &sensorInfo,
> > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > index cee532e3..2b305b56 100644
> > > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context
> > > *_ctx)
> > > >  {
> > > >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> > > *>(_ctx);
> > > >
> > > > -     return ctx->ipa_->start();
> > > > +     /* \todo Translate the ipaConfig and result. */
> > > > +     IPAOperationData ipaConfig = {};
> > > > +     return ctx->ipa_->start(ipaConfig, nullptr);
> > > >  }
> > > >
> > > >  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index f338ff8b..7a07b477 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -77,7 +77,8 @@ public:
> > > >       }
> > > >
> > > >       int init(const IPASettings &settings) override;
> > > > -     int start() override { return 0; }
> > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > +               [[maybe_unused]] IPAOperationData *result) override {
> > > return 0; }
> > > >       void stop() override {}
> > > >
> > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 07d7f1b2..0b8a9096 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -37,7 +37,8 @@ public:
> > > >       {
> > > >               return 0;
> > > >       }
> > > > -     int start() override { return 0; }
> > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > +               [[maybe_unused]] IPAOperationData *result) override {
> > > return 0; }
> > > >       void stop() override {}
> > > >
> > > >       void configure(const CameraSensorInfo &info,
> > > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > > index cf841135..ed26331d 100644
> > > > --- a/src/ipa/vimc/vimc.cpp
> > > > +++ b/src/ipa/vimc/vimc.cpp
> > > > @@ -34,7 +34,8 @@ public:
> > > >
> > > >       int init(const IPASettings &settings) override;
> > > >
> > > > -     int start() override;
> > > > +     int start(const IPAOperationData &ipaConfig,
> > > > +               IPAOperationData *result) override;
> > > >       void stop() override;
> > > >
> > > >       void configure([[maybe_unused]] const CameraSensorInfo
> &sensorInfo,
> > > > @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
> > > >       return 0;
> > > >  }
> > > >
> > > > -int IPAVimc::start()
> > > > +int IPAVimc::start([[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > > > +                [[maybe_unused]] IPAOperationData *result)
> > >
> > >                    ^ is this mis-aligned or is it my email client ?
> > >                    There are more in this patch if that's the case.
> > >
> >
> > I think this may be an email client rendering thing.  They do look
> correct
> > to me, and I would expect the style checker to complain about
> > misalignment.  However, I will check though once again.
> >
>
> Might be, I just pointed it out as I usually see indentation off 1
> space with my client and here I see 3. I haven't applied the patches,
> so I trust your opinion!
>
> >
> > > >  {
> > > >       trace(IPAOperationStart);
> > > >
> > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> > > b/src/libcamera/ipa_context_wrapper.cpp
> > > > index 231300ce..8c4ec40a 100644
> > > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > > @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings
> > > &settings)
> > > >       return 0;
> > > >  }
> > > >
> > > > -int IPAContextWrapper::start()
> > > > +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> > > > +                          IPAOperationData *result)
> > > >  {
> > > >       if (intf_)
> > > > -             return intf_->start();
> > > > +             return intf_->start(ipaConfig, result);
> > > >
> > > >       if (!ctx_)
> > > >               return 0;
> > > >
> > > > +     /* \todo Translate the ipaConfig and response */
> > > >       return ctx_->ops->start(ctx_);
> > > >  }
> > > >
> > > > diff --git a/src/libcamera/ipa_interface.cpp
> > > b/src/libcamera/ipa_interface.cpp
> > > > index 23fc56d7..282c3c0f 100644
> > > > --- a/src/libcamera/ipa_interface.cpp
> > > > +++ b/src/libcamera/ipa_interface.cpp
> > > > @@ -536,10 +536,17 @@ namespace libcamera {
> > > >  /**
> > > >   * \fn IPAInterface::start()
> > > >   * \brief Start the IPA
> > > > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > + * \param[out] result Pipeline-handler-specific configuration result
> > >
> > > These come from the 'configuration' operation
> > > I would:
> > >
> > > \param[in] data Protocol-specific data for the start operation
> > > \param[out] result Result of the start operation
> > >
> >
> > Fixed.
>
> Thanks
>
> >
> >
> > >
> > > >   *
> > > >   * This method informs the IPA module that the camera is about to be
> > > started.
> > > >   * The IPA module shall prepare any resources it needs to operate.
> > > >   *
> > > > + * The \a 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.
> > > > + *
> > >
> > > Copied from configure too, replace 'configure()' at least.
> > >
> >
> > Oops, replaced with start()!
> >
> >
> > >
> > > >   * \return 0 on success or a negative error code otherwise
> > > >   */
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index ddb30e49..29bcef07 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera,
> > > [[maybe_unused]] ControlList *cont
> > > >       }
> > > >
> > > >       /* Start the IPA. */
> > > > -     ret = data->ipa_->start();
> > > > +     IPAOperationData ipaConfig = {}, result = {};
> > > > +     ipaConfig.controls.emplace_back(*controls);
> > >
> > > What if controls is nullptr ?
> > >
> >
> > Hmmm.... I am a bit confused.  My version of the code has:
> >
> >  if (controls) {
> >        ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> >        ipaConfig.controls.emplace_back(*controls);
> > }
> >
> > But clearly my patch in the email does not :(  Once I complete all your
> > suggested fixups, I will send an updated patchset!
> >
>
> Thanks
>
> >
> > > > +     ret = data->ipa_->start(ipaConfig, &result);
> > > >       if (ret) {
> > > >               LOG(RPI, Error)
> > > >                       << "Failed to start IPA for " << camera->id();
> > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
> > > CameraConfiguration *config)
> > > >       }
> > > >
> > > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > > -     IPAOperationData result;
> > > > +     IPAOperationData result = {};
> > >
> > > Unrelated but I think it's ok
> > >
> >
> > This was an update requested by David in his review.  Just to be safe :)
> >
>
> I agree it is an opportune fix, maybe not strictly part of this patch.
> But it's ok :)
>
> >
> > >
> > > >
> > > >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > > ipaConfig,
> > > >                       &result);
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 2e8d2930..a6c82a48 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> > > [[maybe_unused]] ControlList *c
> > > >       if (ret)
> > > >               return ret;
> > > >
> > > > -     ret = data->ipa_->start();
> > > > +     IPAOperationData ipaConfig = {};
> > > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > >
> > > If you want to use a single IPAOperationData for start and configure,
> > > please rename it (here and in other pipeline handlers).
> > >
> >
> > I would use data (as with the other IPAOperationData parameters passed
> into
> > start), but we already have a declaration of RkISP1CameraData *data.  Any
> > objections to renaming it ipaData?
> >
>
> Sounds good to me!
> Thanks for addressing all comments!
>
> > Regards,
> > Naush
> >
> >
> >
> > >
> > > Thanks
> > >   j
> > >
> > >
> > > >       if (ret) {
> > > >               freeBuffers(camera);
> > > >               LOG(RkISP1, Error)
> > > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> > > [[maybe_unused]] ControlList *c
> > > >       std::map<unsigned int, const ControlInfoMap &> entityControls;
> > > >       entityControls.emplace(0, data->sensor_->controls());
> > > >
> > > > -     IPAOperationData ipaConfig;
> > > > +     ipaConfig = {};
> > > >       data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > >                             ipaConfig, nullptr);
> > > >
> > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
> > > b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > index d81b8598..b4773cf5 100644
> > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera,
> > > [[maybe_unused]] ControlList *con
> > > >       if (ret < 0)
> > > >               return ret;
> > > >
> > > > -     ret = data->ipa_->start();
> > > > +     IPAOperationData ipaConfig = {};
> > > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > > >       if (ret) {
> > > >               data->video_->releaseBuffers();
> > > >               return ret;
> > > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > index b78a0e45..619976e5 100644
> > > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > @@ -30,7 +30,8 @@ public:
> > > >       {
> > > >               return 0;
> > > >       }
> > > > -     int start() override { return 0; }
> > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > +               [[maybe_unused]] IPAOperationData *result) override {
> > > return 0; }
> > > >       void stop() override {}
> > > >       void configure([[maybe_unused]] const CameraSensorInfo
> &sensorInfo,
> > > >                      [[maybe_unused]] const std::map<unsigned int,
> > > IPAStream> &streamConfig,
> > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > index eead2883..9a81b6e7 100644
> > > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > @@ -26,7 +26,8 @@ public:
> > > >       IPAProxyThread(IPAModule *ipam);
> > > >
> > > >       int init(const IPASettings &settings) override;
> > > > -     int start() override;
> > > > +     int start(const IPAOperationData &ipaConfig,
> > > > +               IPAOperationData *result) override;
> > > >       void stop() override;
> > > >
> > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > @@ -50,9 +51,9 @@ private:
> > > >                       ipa_ = ipa;
> > > >               }
> > > >
> > > > -             int start()
> > > > +             int start(const IPAOperationData &ipaConfig,
> > > IPAOperationData *result)
> > > >               {
> > > > -                     return ipa_->start();
> > > > +                     return ipa_->start(ipaConfig, result);
> > > >               }
> > > >
> > > >               void stop()
> > > > @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings
> > > &settings)
> > > >       return 0;
> > > >  }
> > > >
> > > > -int IPAProxyThread::start()
> > > > +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> > > > +                       IPAOperationData *result)
> > > >  {
> > > >       running_ = true;
> > > >       thread_.start();
> > > >
> > > > -     return proxy_.invokeMethod(&ThreadProxy::start,
> > > ConnectionTypeBlocking);
> > > > +     return proxy_.invokeMethod(&ThreadProxy::start,
> > > ConnectionTypeBlocking,
> > > > +                                ipaConfig, result);
> > > >  }
> > > >
> > > >  void IPAProxyThread::stop()
> > > > diff --git a/test/ipa/ipa_interface_test.cpp
> > > b/test/ipa/ipa_interface_test.cpp
> > > > index 67488409..03b7f0ad 100644
> > > > --- a/test/ipa/ipa_interface_test.cpp
> > > > +++ b/test/ipa/ipa_interface_test.cpp
> > > > @@ -120,7 +120,8 @@ protected:
> > > >               }
> > > >
> > > >               /* Test start of IPA module. */
> > > > -             ipa_->start();
> > > > +             IPAOperationData ipaConfig = {};
> > > > +             ipa_->start(ipaConfig, nullptr);
> > > >               timer.start(1000);
> > > >               while (timer.isRunning() && trace_ !=
> IPAOperationStart)
> > > >                       dispatcher->processEvents();
> > > > diff --git a/test/ipa/ipa_wrappers_test.cpp
> > > b/test/ipa/ipa_wrappers_test.cpp
> > > > index 59d991cb..7b8ae77b 100644
> > > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > > @@ -56,7 +56,8 @@ public:
> > > >               return 0;
> > > >       }
> > > >
> > > > -     int start() override
> > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > +               [[maybe_unused]] IPAOperationData *result) override
> > > >       {
> > > >               report(Op_start, TestPass);
> > > >               return 0;
> > > > @@ -376,7 +377,7 @@ protected:
> > > >                       return TestFail;
> > > >               }
> > > >
> > > > -             ret = INVOKE(start);
> > > > +             ret = INVOKE(start, ipaConfig, nullptr);
> > > >               if (ret == TestFail) {
> > > >                       cerr << "Failed to run start()";
> > > >                       return TestFail;
> > > > --
> > > > 2.25.1
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
>

Hi Naush,

On Fri, Dec 04, 2020 at 01:38:58PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
>
> On Fri, 4 Dec 2020 at 13:20, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> > Hi Nuash,
> >
> > On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for your review feedback.
> > >
> > > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > > Hi Naush,
> > > >
> > > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
> > > > > This change allows controls passed into PipelineHandler::start to be
> > > > > forwarded onto IPAInterface::start(). We also add a return channel
> > if the
> > > > > pipeline handler must action any of these controls, e.g. setting the
> > > > > analogue gain or shutter speed in the sensor device.
> > > > >
> > > > > todo: The IPA interface wrapper needs a translation for passing
> > > > > IPAOperationData into start() and configure()
> > > >
> > > > This is 'problematic' as it makes isolated IPAs deviate from other
> > > > ones. We have the same issue with configure() which has a very similar
> > > > \todo item :/
> > > >
> > > > Now, this is not an issue right now, and I think with Paul's IPC
> > > > we'll get it for free.
> > > >
> > > > Other comments below:
> > > >
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
> > > > >  include/libcamera/ipa/ipa_interface.h              |  3 ++-
> > > > >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
> > > > >  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
> > > > >  src/ipa/vimc/vimc.cpp                              |  6 ++++--
> > > > >  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
> > > > >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
> > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
> > > > >  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
> > > > >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
> > > > >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13
> > ++++++++-----
> > > > >  test/ipa/ipa_interface_test.cpp                    |  3 ++-
> > > > >  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
> > > > >  15 files changed, 50 insertions(+), 23 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> > > > b/include/libcamera/internal/ipa_context_wrapper.h
> > > > > index 8f767e84..1878a615 100644
> > > > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > > > @@ -20,7 +20,8 @@ public:
> > > > >       ~IPAContextWrapper();
> > > > >
> > > > >       int init(const IPASettings &settings) override;
> > > > > -     int start() override;
> > > > > +     int start(const IPAOperationData &ipaConfig,
> > > >
> > > > 'ipaConfig' is only used in configure.
> > > > It's generally just called 'data' and I would use it here too.
> > > >
> > >
> > > Sure, will fix.  I will also s/ ipaConfig/data/ in all instances of
> > start()
> > > that have been changed in this patch.
> >
> > Thanks! Other names are good to, just drop 'Config'
> >
>
> Could you clarify the above comment please?  I was intending to use "data"
> for all  IPAOperationData variables that have been introduced in the
> start() method in this patch.  Is your suggestion to use "ipa" instead of
> "data"?

Sorry for the confusion. What I meant was "If you have other (better)
names than 'data', that's also fine. What presses me is not to have
'Config' in the name"

>
> Regards,
> Naush
>
>
>
> >
> > >
> > >
> > > > > +               IPAOperationData *result) override;
> > > > >       void stop() override;
> > > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > >                      const std::map<unsigned int, IPAStream>
> > > > &streamConfig,
> > > > > diff --git a/include/libcamera/ipa/ipa_interface.h
> > > > b/include/libcamera/ipa/ipa_interface.h
> > > > > index 322b7079..b44e2538 100644
> > > > > --- a/include/libcamera/ipa/ipa_interface.h
> > > > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > > > @@ -153,7 +153,8 @@ public:
> > > > >       virtual ~IPAInterface() = default;
> > > > >
> > > > >       virtual int init(const IPASettings &settings) = 0;
> > > > > -     virtual int start() = 0;
> > > > > +     virtual int start(const IPAOperationData &ipaConfig,
> > > > > +                       IPAOperationData *result) = 0;
> > > > >       virtual void stop() = 0;
> > > > >
> > > > >       virtual void configure(const CameraSensorInfo &sensorInfo,
> > > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > index cee532e3..2b305b56 100644
> > > > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context
> > > > *_ctx)
> > > > >  {
> > > > >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> > > > *>(_ctx);
> > > > >
> > > > > -     return ctx->ipa_->start();
> > > > > +     /* \todo Translate the ipaConfig and result. */
> > > > > +     IPAOperationData ipaConfig = {};
> > > > > +     return ctx->ipa_->start(ipaConfig, nullptr);
> > > > >  }
> > > > >
> > > > >  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index f338ff8b..7a07b477 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -77,7 +77,8 @@ public:
> > > > >       }
> > > > >
> > > > >       int init(const IPASettings &settings) override;
> > > > > -     int start() override { return 0; }
> > > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > > +               [[maybe_unused]] IPAOperationData *result) override {
> > > > return 0; }
> > > > >       void stop() override {}
> > > > >
> > > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > index 07d7f1b2..0b8a9096 100644
> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > @@ -37,7 +37,8 @@ public:
> > > > >       {
> > > > >               return 0;
> > > > >       }
> > > > > -     int start() override { return 0; }
> > > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > > +               [[maybe_unused]] IPAOperationData *result) override {
> > > > return 0; }
> > > > >       void stop() override {}
> > > > >
> > > > >       void configure(const CameraSensorInfo &info,
> > > > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > > > index cf841135..ed26331d 100644
> > > > > --- a/src/ipa/vimc/vimc.cpp
> > > > > +++ b/src/ipa/vimc/vimc.cpp
> > > > > @@ -34,7 +34,8 @@ public:
> > > > >
> > > > >       int init(const IPASettings &settings) override;
> > > > >
> > > > > -     int start() override;
> > > > > +     int start(const IPAOperationData &ipaConfig,
> > > > > +               IPAOperationData *result) override;
> > > > >       void stop() override;
> > > > >
> > > > >       void configure([[maybe_unused]] const CameraSensorInfo
> > &sensorInfo,
> > > > > @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > -int IPAVimc::start()
> > > > > +int IPAVimc::start([[maybe_unused]] const IPAOperationData
> > &ipaConfig,
> > > > > +                [[maybe_unused]] IPAOperationData *result)
> > > >
> > > >                    ^ is this mis-aligned or is it my email client ?
> > > >                    There are more in this patch if that's the case.
> > > >
> > >
> > > I think this may be an email client rendering thing.  They do look
> > correct
> > > to me, and I would expect the style checker to complain about
> > > misalignment.  However, I will check though once again.
> > >
> >
> > Might be, I just pointed it out as I usually see indentation off 1
> > space with my client and here I see 3. I haven't applied the patches,
> > so I trust your opinion!
> >
> > >
> > > > >  {
> > > > >       trace(IPAOperationStart);
> > > > >
> > > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> > > > b/src/libcamera/ipa_context_wrapper.cpp
> > > > > index 231300ce..8c4ec40a 100644
> > > > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > > > @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings
> > > > &settings)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > -int IPAContextWrapper::start()
> > > > > +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> > > > > +                          IPAOperationData *result)
> > > > >  {
> > > > >       if (intf_)
> > > > > -             return intf_->start();
> > > > > +             return intf_->start(ipaConfig, result);
> > > > >
> > > > >       if (!ctx_)
> > > > >               return 0;
> > > > >
> > > > > +     /* \todo Translate the ipaConfig and response */
> > > > >       return ctx_->ops->start(ctx_);
> > > > >  }
> > > > >
> > > > > diff --git a/src/libcamera/ipa_interface.cpp
> > > > b/src/libcamera/ipa_interface.cpp
> > > > > index 23fc56d7..282c3c0f 100644
> > > > > --- a/src/libcamera/ipa_interface.cpp
> > > > > +++ b/src/libcamera/ipa_interface.cpp
> > > > > @@ -536,10 +536,17 @@ namespace libcamera {
> > > > >  /**
> > > > >   * \fn IPAInterface::start()
> > > > >   * \brief Start the IPA
> > > > > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > > + * \param[out] result Pipeline-handler-specific configuration result
> > > >
> > > > These come from the 'configuration' operation
> > > > I would:
> > > >
> > > > \param[in] data Protocol-specific data for the start operation
> > > > \param[out] result Result of the start operation
> > > >
> > >
> > > Fixed.
> >
> > Thanks
> >
> > >
> > >
> > > >
> > > > >   *
> > > > >   * This method informs the IPA module that the camera is about to be
> > > > started.
> > > > >   * The IPA module shall prepare any resources it needs to operate.
> > > > >   *
> > > > > + * The \a 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.
> > > > > + *
> > > >
> > > > Copied from configure too, replace 'configure()' at least.
> > > >
> > >
> > > Oops, replaced with start()!
> > >
> > >
> > > >
> > > > >   * \return 0 on success or a negative error code otherwise
> > > > >   */
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index ddb30e49..29bcef07 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera,
> > > > [[maybe_unused]] ControlList *cont
> > > > >       }
> > > > >
> > > > >       /* Start the IPA. */
> > > > > -     ret = data->ipa_->start();
> > > > > +     IPAOperationData ipaConfig = {}, result = {};
> > > > > +     ipaConfig.controls.emplace_back(*controls);
> > > >
> > > > What if controls is nullptr ?
> > > >
> > >
> > > Hmmm.... I am a bit confused.  My version of the code has:
> > >
> > >  if (controls) {
> > >        ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> > >        ipaConfig.controls.emplace_back(*controls);
> > > }
> > >
> > > But clearly my patch in the email does not :(  Once I complete all your
> > > suggested fixups, I will send an updated patchset!
> > >
> >
> > Thanks
> >
> > >
> > > > > +     ret = data->ipa_->start(ipaConfig, &result);
> > > > >       if (ret) {
> > > > >               LOG(RPI, Error)
> > > > >                       << "Failed to start IPA for " << camera->id();
> > > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
> > > > CameraConfiguration *config)
> > > > >       }
> > > > >
> > > > >       /* Ready the IPA - it must know about the sensor resolution. */
> > > > > -     IPAOperationData result;
> > > > > +     IPAOperationData result = {};
> > > >
> > > > Unrelated but I think it's ok
> > > >
> > >
> > > This was an update requested by David in his review.  Just to be safe :)
> > >
> >
> > I agree it is an opportune fix, maybe not strictly part of this patch.
> > But it's ok :)
> >
> > >
> > > >
> > > > >
> > > > >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > > > ipaConfig,
> > > > >                       &result);
> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > index 2e8d2930..a6c82a48 100644
> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> > > > [[maybe_unused]] ControlList *c
> > > > >       if (ret)
> > > > >               return ret;
> > > > >
> > > > > -     ret = data->ipa_->start();
> > > > > +     IPAOperationData ipaConfig = {};
> > > > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > > >
> > > > If you want to use a single IPAOperationData for start and configure,
> > > > please rename it (here and in other pipeline handlers).
> > > >
> > >
> > > I would use data (as with the other IPAOperationData parameters passed
> > into
> > > start), but we already have a declaration of RkISP1CameraData *data.  Any
> > > objections to renaming it ipaData?
> > >
> >
> > Sounds good to me!
> > Thanks for addressing all comments!
> >
> > > Regards,
> > > Naush
> > >
> > >
> > >
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > >
> > > > >       if (ret) {
> > > > >               freeBuffers(camera);
> > > > >               LOG(RkISP1, Error)
> > > > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,
> > > > [[maybe_unused]] ControlList *c
> > > > >       std::map<unsigned int, const ControlInfoMap &> entityControls;
> > > > >       entityControls.emplace(0, data->sensor_->controls());
> > > > >
> > > > > -     IPAOperationData ipaConfig;
> > > > > +     ipaConfig = {};
> > > > >       data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > > >                             ipaConfig, nullptr);
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > index d81b8598..b4773cf5 100644
> > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera,
> > > > [[maybe_unused]] ControlList *con
> > > > >       if (ret < 0)
> > > > >               return ret;
> > > > >
> > > > > -     ret = data->ipa_->start();
> > > > > +     IPAOperationData ipaConfig = {};
> > > > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > > > >       if (ret) {
> > > > >               data->video_->releaseBuffers();
> > > > >               return ret;
> > > > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > index b78a0e45..619976e5 100644
> > > > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > @@ -30,7 +30,8 @@ public:
> > > > >       {
> > > > >               return 0;
> > > > >       }
> > > > > -     int start() override { return 0; }
> > > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > > +               [[maybe_unused]] IPAOperationData *result) override {
> > > > return 0; }
> > > > >       void stop() override {}
> > > > >       void configure([[maybe_unused]] const CameraSensorInfo
> > &sensorInfo,
> > > > >                      [[maybe_unused]] const std::map<unsigned int,
> > > > IPAStream> &streamConfig,
> > > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > index eead2883..9a81b6e7 100644
> > > > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > @@ -26,7 +26,8 @@ public:
> > > > >       IPAProxyThread(IPAModule *ipam);
> > > > >
> > > > >       int init(const IPASettings &settings) override;
> > > > > -     int start() override;
> > > > > +     int start(const IPAOperationData &ipaConfig,
> > > > > +               IPAOperationData *result) override;
> > > > >       void stop() override;
> > > > >
> > > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > > @@ -50,9 +51,9 @@ private:
> > > > >                       ipa_ = ipa;
> > > > >               }
> > > > >
> > > > > -             int start()
> > > > > +             int start(const IPAOperationData &ipaConfig,
> > > > IPAOperationData *result)
> > > > >               {
> > > > > -                     return ipa_->start();
> > > > > +                     return ipa_->start(ipaConfig, result);
> > > > >               }
> > > > >
> > > > >               void stop()
> > > > > @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings
> > > > &settings)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > -int IPAProxyThread::start()
> > > > > +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> > > > > +                       IPAOperationData *result)
> > > > >  {
> > > > >       running_ = true;
> > > > >       thread_.start();
> > > > >
> > > > > -     return proxy_.invokeMethod(&ThreadProxy::start,
> > > > ConnectionTypeBlocking);
> > > > > +     return proxy_.invokeMethod(&ThreadProxy::start,
> > > > ConnectionTypeBlocking,
> > > > > +                                ipaConfig, result);
> > > > >  }
> > > > >
> > > > >  void IPAProxyThread::stop()
> > > > > diff --git a/test/ipa/ipa_interface_test.cpp
> > > > b/test/ipa/ipa_interface_test.cpp
> > > > > index 67488409..03b7f0ad 100644
> > > > > --- a/test/ipa/ipa_interface_test.cpp
> > > > > +++ b/test/ipa/ipa_interface_test.cpp
> > > > > @@ -120,7 +120,8 @@ protected:
> > > > >               }
> > > > >
> > > > >               /* Test start of IPA module. */
> > > > > -             ipa_->start();
> > > > > +             IPAOperationData ipaConfig = {};
> > > > > +             ipa_->start(ipaConfig, nullptr);
> > > > >               timer.start(1000);
> > > > >               while (timer.isRunning() && trace_ !=
> > IPAOperationStart)
> > > > >                       dispatcher->processEvents();
> > > > > diff --git a/test/ipa/ipa_wrappers_test.cpp
> > > > b/test/ipa/ipa_wrappers_test.cpp
> > > > > index 59d991cb..7b8ae77b 100644
> > > > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > > > @@ -56,7 +56,8 @@ public:
> > > > >               return 0;
> > > > >       }
> > > > >
> > > > > -     int start() override
> > > > > +     int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > > > > +               [[maybe_unused]] IPAOperationData *result) override
> > > > >       {
> > > > >               report(Op_start, TestPass);
> > > > >               return 0;
> > > > > @@ -376,7 +377,7 @@ protected:
> > > > >                       return TestFail;
> > > > >               }
> > > > >
> > > > > -             ret = INVOKE(start);
> > > > > +             ret = INVOKE(start, ipaConfig, nullptr);
> > > > >               if (ret == TestFail) {
> > > > >                       cerr << "Failed to run start()";
> > > > >                       return TestFail;
> > > > > --
> > > > > 2.25.1
> > > > >
> > > > > _______________________________________________
> > > > > libcamera-devel mailing list
> > > > > libcamera-devel@lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > >
> >

Hi Jacopo,

On Fri, 4 Dec 2020 at 13:52, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Naush,
>
> On Fri, Dec 04, 2020 at 01:38:58PM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> >
> > On Fri, 4 Dec 2020 at 13:20, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > > Hi Nuash,
> > >
> > > On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thank you for your review feedback.
> > > >
> > > > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > > >
> > > > > Hi Naush,
> > > > >
> > > > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
> > > > > > This change allows controls passed into PipelineHandler::start
> to be
> > > > > > forwarded onto IPAInterface::start(). We also add a return
> channel
> > > if the
> > > > > > pipeline handler must action any of these controls, e.g. setting
> the
> > > > > > analogue gain or shutter speed in the sensor device.
> > > > > >
> > > > > > todo: The IPA interface wrapper needs a translation for passing
> > > > > > IPAOperationData into start() and configure()
> > > > >
> > > > > This is 'problematic' as it makes isolated IPAs deviate from other
> > > > > ones. We have the same issue with configure() which has a very
> similar
> > > > > \todo item :/
> > > > >
> > > > > Now, this is not an issue right now, and I think with Paul's IPC
> > > > > we'll get it for free.
> > > > >
> > > > > Other comments below:
> > > > >
> > > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > > ---
> > > > > >  include/libcamera/internal/ipa_context_wrapper.h   |  3 ++-
> > > > > >  include/libcamera/ipa/ipa_interface.h              |  3 ++-
> > > > > >  src/ipa/libipa/ipa_interface_wrapper.cpp           |  4 +++-
> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp                |  3 ++-
> > > > > >  src/ipa/rkisp1/rkisp1.cpp                          |  3 ++-
> > > > > >  src/ipa/vimc/vimc.cpp                              |  6 ++++--
> > > > > >  src/libcamera/ipa_context_wrapper.cpp              |  6 ++++--
> > > > > >  src/libcamera/ipa_interface.cpp                    |  7 +++++++
> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  6 ++++--
> > > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  5 +++--
> > > > > >  src/libcamera/pipeline/vimc/vimc.cpp               |  3 ++-
> > > > > >  src/libcamera/proxy/ipa_proxy_linux.cpp            |  3 ++-
> > > > > >  src/libcamera/proxy/ipa_proxy_thread.cpp           | 13
> > > ++++++++-----
> > > > > >  test/ipa/ipa_interface_test.cpp                    |  3 ++-
> > > > > >  test/ipa/ipa_wrappers_test.cpp                     |  5 +++--
> > > > > >  15 files changed, 50 insertions(+), 23 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> > > > > b/include/libcamera/internal/ipa_context_wrapper.h
> > > > > > index 8f767e84..1878a615 100644
> > > > > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > > > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > > > > @@ -20,7 +20,8 @@ public:
> > > > > >       ~IPAContextWrapper();
> > > > > >
> > > > > >       int init(const IPASettings &settings) override;
> > > > > > -     int start() override;
> > > > > > +     int start(const IPAOperationData &ipaConfig,
> > > > >
> > > > > 'ipaConfig' is only used in configure.
> > > > > It's generally just called 'data' and I would use it here too.
> > > > >
> > > >
> > > > Sure, will fix.  I will also s/ ipaConfig/data/ in all instances of
> > > start()
> > > > that have been changed in this patch.
> > >
> > > Thanks! Other names are good to, just drop 'Config'
> > >
> >
> > Could you clarify the above comment please?  I was intending to use
> "data"
> > for all  IPAOperationData variables that have been introduced in the
> > start() method in this patch.  Is your suggestion to use "ipa" instead of
> > "data"?
>
> Sorry for the confusion. What I meant was "If you have other (better)
> names than 'data', that's also fine. What presses me is not to have
> 'Config' in the name"
>

Got it, thanks for clarifying!


>
> >
> > Regards,
> > Naush
> >
> >
> >
> > >
> > > >
> > > >
> > > > > > +               IPAOperationData *result) override;
> > > > > >       void stop() override;
> > > > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > > >                      const std::map<unsigned int, IPAStream>
> > > > > &streamConfig,
> > > > > > diff --git a/include/libcamera/ipa/ipa_interface.h
> > > > > b/include/libcamera/ipa/ipa_interface.h
> > > > > > index 322b7079..b44e2538 100644
> > > > > > --- a/include/libcamera/ipa/ipa_interface.h
> > > > > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > > > > @@ -153,7 +153,8 @@ public:
> > > > > >       virtual ~IPAInterface() = default;
> > > > > >
> > > > > >       virtual int init(const IPASettings &settings) = 0;
> > > > > > -     virtual int start() = 0;
> > > > > > +     virtual int start(const IPAOperationData &ipaConfig,
> > > > > > +                       IPAOperationData *result) = 0;
> > > > > >       virtual void stop() = 0;
> > > > > >
> > > > > >       virtual void configure(const CameraSensorInfo &sensorInfo,
> > > > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > > index cee532e3..2b305b56 100644
> > > > > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > > > @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct
> ipa_context
> > > > > *_ctx)
> > > > > >  {
> > > > > >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> > > > > *>(_ctx);
> > > > > >
> > > > > > -     return ctx->ipa_->start();
> > > > > > +     /* \todo Translate the ipaConfig and result. */
> > > > > > +     IPAOperationData ipaConfig = {};
> > > > > > +     return ctx->ipa_->start(ipaConfig, nullptr);
> > > > > >  }
> > > > > >
> > > > > >  void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > index f338ff8b..7a07b477 100644
> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > @@ -77,7 +77,8 @@ public:
> > > > > >       }
> > > > > >
> > > > > >       int init(const IPASettings &settings) override;
> > > > > > -     int start() override { return 0; }
> > > > > > +     int start([[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > > > > > +               [[maybe_unused]] IPAOperationData *result)
> override {
> > > > > return 0; }
> > > > > >       void stop() override {}
> > > > > >
> > > > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp
> b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > index 07d7f1b2..0b8a9096 100644
> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > @@ -37,7 +37,8 @@ public:
> > > > > >       {
> > > > > >               return 0;
> > > > > >       }
> > > > > > -     int start() override { return 0; }
> > > > > > +     int start([[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > > > > > +               [[maybe_unused]] IPAOperationData *result)
> override {
> > > > > return 0; }
> > > > > >       void stop() override {}
> > > > > >
> > > > > >       void configure(const CameraSensorInfo &info,
> > > > > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > > > > index cf841135..ed26331d 100644
> > > > > > --- a/src/ipa/vimc/vimc.cpp
> > > > > > +++ b/src/ipa/vimc/vimc.cpp
> > > > > > @@ -34,7 +34,8 @@ public:
> > > > > >
> > > > > >       int init(const IPASettings &settings) override;
> > > > > >
> > > > > > -     int start() override;
> > > > > > +     int start(const IPAOperationData &ipaConfig,
> > > > > > +               IPAOperationData *result) override;
> > > > > >       void stop() override;
> > > > > >
> > > > > >       void configure([[maybe_unused]] const CameraSensorInfo
> > > &sensorInfo,
> > > > > > @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > -int IPAVimc::start()
> > > > > > +int IPAVimc::start([[maybe_unused]] const IPAOperationData
> > > &ipaConfig,
> > > > > > +                [[maybe_unused]] IPAOperationData *result)
> > > > >
> > > > >                    ^ is this mis-aligned or is it my email client ?
> > > > >                    There are more in this patch if that's the case.
> > > > >
> > > >
> > > > I think this may be an email client rendering thing.  They do look
> > > correct
> > > > to me, and I would expect the style checker to complain about
> > > > misalignment.  However, I will check though once again.
> > > >
> > >
> > > Might be, I just pointed it out as I usually see indentation off 1
> > > space with my client and here I see 3. I haven't applied the patches,
> > > so I trust your opinion!
> > >
> > > >
> > > > > >  {
> > > > > >       trace(IPAOperationStart);
> > > > > >
> > > > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> > > > > b/src/libcamera/ipa_context_wrapper.cpp
> > > > > > index 231300ce..8c4ec40a 100644
> > > > > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > > > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > > > > @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings
> > > > > &settings)
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > -int IPAContextWrapper::start()
> > > > > > +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,
> > > > > > +                          IPAOperationData *result)
> > > > > >  {
> > > > > >       if (intf_)
> > > > > > -             return intf_->start();
> > > > > > +             return intf_->start(ipaConfig, result);
> > > > > >
> > > > > >       if (!ctx_)
> > > > > >               return 0;
> > > > > >
> > > > > > +     /* \todo Translate the ipaConfig and response */
> > > > > >       return ctx_->ops->start(ctx_);
> > > > > >  }
> > > > > >
> > > > > > diff --git a/src/libcamera/ipa_interface.cpp
> > > > > b/src/libcamera/ipa_interface.cpp
> > > > > > index 23fc56d7..282c3c0f 100644
> > > > > > --- a/src/libcamera/ipa_interface.cpp
> > > > > > +++ b/src/libcamera/ipa_interface.cpp
> > > > > > @@ -536,10 +536,17 @@ namespace libcamera {
> > > > > >  /**
> > > > > >   * \fn IPAInterface::start()
> > > > > >   * \brief Start the IPA
> > > > > > + * \param[in] ipaConfig Pipeline-handler-specific configuration
> data
> > > > > > + * \param[out] result Pipeline-handler-specific configuration
> result
> > > > >
> > > > > These come from the 'configuration' operation
> > > > > I would:
> > > > >
> > > > > \param[in] data Protocol-specific data for the start operation
> > > > > \param[out] result Result of the start operation
> > > > >
> > > >
> > > > Fixed.
> > >
> > > Thanks
> > >
> > > >
> > > >
> > > > >
> > > > > >   *
> > > > > >   * This method informs the IPA module that the camera is about
> to be
> > > > > started.
> > > > > >   * The IPA module shall prepare any resources it needs to
> operate.
> > > > > >   *
> > > > > > + * The \a 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.
> > > > > > + *
> > > > >
> > > > > Copied from configure too, replace 'configure()' at least.
> > > > >
> > > >
> > > > Oops, replaced with start()!
> > > >
> > > >
> > > > >
> > > > > >   * \return 0 on success or a negative error code otherwise
> > > > > >   */
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index ddb30e49..29bcef07 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera,
> > > > > [[maybe_unused]] ControlList *cont
> > > > > >       }
> > > > > >
> > > > > >       /* Start the IPA. */
> > > > > > -     ret = data->ipa_->start();
> > > > > > +     IPAOperationData ipaConfig = {}, result = {};
> > > > > > +     ipaConfig.controls.emplace_back(*controls);
> > > > >
> > > > > What if controls is nullptr ?
> > > > >
> > > >
> > > > Hmmm.... I am a bit confused.  My version of the code has:
> > > >
> > > >  if (controls) {
> > > >        ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> > > >        ipaConfig.controls.emplace_back(*controls);
> > > > }
> > > >
> > > > But clearly my patch in the email does not :(  Once I complete all
> your
> > > > suggested fixups, I will send an updated patchset!
> > > >
> > >
> > > Thanks
> > >
> > > >
> > > > > > +     ret = data->ipa_->start(ipaConfig, &result);
> > > > > >       if (ret) {
> > > > > >               LOG(RPI, Error)
> > > > > >                       << "Failed to start IPA for " <<
> camera->id();
> > > > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
> > > > > CameraConfiguration *config)
> > > > > >       }
> > > > > >
> > > > > >       /* Ready the IPA - it must know about the sensor
> resolution. */
> > > > > > -     IPAOperationData result;
> > > > > > +     IPAOperationData result = {};
> > > > >
> > > > > Unrelated but I think it's ok
> > > > >
> > > >
> > > > This was an update requested by David in his review.  Just to be
> safe :)
> > > >
> > >
> > > I agree it is an opportune fix, maybe not strictly part of this patch.
> > > But it's ok :)
> > >
> > > >
> > > > >
> > > > > >
> > > > > >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > > > > ipaConfig,
> > > > > >                       &result);
> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > index 2e8d2930..a6c82a48 100644
> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera
> *camera,
> > > > > [[maybe_unused]] ControlList *c
> > > > > >       if (ret)
> > > > > >               return ret;
> > > > > >
> > > > > > -     ret = data->ipa_->start();
> > > > > > +     IPAOperationData ipaConfig = {};
> > > > > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > > > >
> > > > > If you want to use a single IPAOperationData for start and
> configure,
> > > > > please rename it (here and in other pipeline handlers).
> > > > >
> > > >
> > > > I would use data (as with the other IPAOperationData parameters
> passed
> > > into
> > > > start), but we already have a declaration of RkISP1CameraData
> *data.  Any
> > > > objections to renaming it ipaData?
> > > >
> > >
> > > Sounds good to me!
> > > Thanks for addressing all comments!
> > >
> > > > Regards,
> > > > Naush
> > > >
> > > >
> > > >
> > > > >
> > > > > Thanks
> > > > >   j
> > > > >
> > > > >
> > > > > >       if (ret) {
> > > > > >               freeBuffers(camera);
> > > > > >               LOG(RkISP1, Error)
> > > > > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera
> *camera,
> > > > > [[maybe_unused]] ControlList *c
> > > > > >       std::map<unsigned int, const ControlInfoMap &>
> entityControls;
> > > > > >       entityControls.emplace(0, data->sensor_->controls());
> > > > > >
> > > > > > -     IPAOperationData ipaConfig;
> > > > > > +     ipaConfig = {};
> > > > > >       data->ipa_->configure(sensorInfo, streamConfig,
> entityControls,
> > > > > >                             ipaConfig, nullptr);
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > index d81b8598..b4773cf5 100644
> > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera
> *camera,
> > > > > [[maybe_unused]] ControlList *con
> > > > > >       if (ret < 0)
> > > > > >               return ret;
> > > > > >
> > > > > > -     ret = data->ipa_->start();
> > > > > > +     IPAOperationData ipaConfig = {};
> > > > > > +     ret = data->ipa_->start(ipaConfig, nullptr);
> > > > > >       if (ret) {
> > > > > >               data->video_->releaseBuffers();
> > > > > >               return ret;
> > > > > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > > index b78a0e45..619976e5 100644
> > > > > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > > > @@ -30,7 +30,8 @@ public:
> > > > > >       {
> > > > > >               return 0;
> > > > > >       }
> > > > > > -     int start() override { return 0; }
> > > > > > +     int start([[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > > > > > +               [[maybe_unused]] IPAOperationData *result)
> override {
> > > > > return 0; }
> > > > > >       void stop() override {}
> > > > > >       void configure([[maybe_unused]] const CameraSensorInfo
> > > &sensorInfo,
> > > > > >                      [[maybe_unused]] const std::map<unsigned
> int,
> > > > > IPAStream> &streamConfig,
> > > > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > > index eead2883..9a81b6e7 100644
> > > > > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > > > @@ -26,7 +26,8 @@ public:
> > > > > >       IPAProxyThread(IPAModule *ipam);
> > > > > >
> > > > > >       int init(const IPASettings &settings) override;
> > > > > > -     int start() override;
> > > > > > +     int start(const IPAOperationData &ipaConfig,
> > > > > > +               IPAOperationData *result) override;
> > > > > >       void stop() override;
> > > > > >
> > > > > >       void configure(const CameraSensorInfo &sensorInfo,
> > > > > > @@ -50,9 +51,9 @@ private:
> > > > > >                       ipa_ = ipa;
> > > > > >               }
> > > > > >
> > > > > > -             int start()
> > > > > > +             int start(const IPAOperationData &ipaConfig,
> > > > > IPAOperationData *result)
> > > > > >               {
> > > > > > -                     return ipa_->start();
> > > > > > +                     return ipa_->start(ipaConfig, result);
> > > > > >               }
> > > > > >
> > > > > >               void stop()
> > > > > > @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings
> > > > > &settings)
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > -int IPAProxyThread::start()
> > > > > > +int IPAProxyThread::start(const IPAOperationData &ipaConfig,
> > > > > > +                       IPAOperationData *result)
> > > > > >  {
> > > > > >       running_ = true;
> > > > > >       thread_.start();
> > > > > >
> > > > > > -     return proxy_.invokeMethod(&ThreadProxy::start,
> > > > > ConnectionTypeBlocking);
> > > > > > +     return proxy_.invokeMethod(&ThreadProxy::start,
> > > > > ConnectionTypeBlocking,
> > > > > > +                                ipaConfig, result);
> > > > > >  }
> > > > > >
> > > > > >  void IPAProxyThread::stop()
> > > > > > diff --git a/test/ipa/ipa_interface_test.cpp
> > > > > b/test/ipa/ipa_interface_test.cpp
> > > > > > index 67488409..03b7f0ad 100644
> > > > > > --- a/test/ipa/ipa_interface_test.cpp
> > > > > > +++ b/test/ipa/ipa_interface_test.cpp
> > > > > > @@ -120,7 +120,8 @@ protected:
> > > > > >               }
> > > > > >
> > > > > >               /* Test start of IPA module. */
> > > > > > -             ipa_->start();
> > > > > > +             IPAOperationData ipaConfig = {};
> > > > > > +             ipa_->start(ipaConfig, nullptr);
> > > > > >               timer.start(1000);
> > > > > >               while (timer.isRunning() && trace_ !=
> > > IPAOperationStart)
> > > > > >                       dispatcher->processEvents();
> > > > > > diff --git a/test/ipa/ipa_wrappers_test.cpp
> > > > > b/test/ipa/ipa_wrappers_test.cpp
> > > > > > index 59d991cb..7b8ae77b 100644
> > > > > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > > > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > > > > @@ -56,7 +56,8 @@ public:
> > > > > >               return 0;
> > > > > >       }
> > > > > >
> > > > > > -     int start() override
> > > > > > +     int start([[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > > > > > +               [[maybe_unused]] IPAOperationData *result)
> override
> > > > > >       {
> > > > > >               report(Op_start, TestPass);
> > > > > >               return 0;
> > > > > > @@ -376,7 +377,7 @@ protected:
> > > > > >                       return TestFail;
> > > > > >               }
> > > > > >
> > > > > > -             ret = INVOKE(start);
> > > > > > +             ret = INVOKE(start, ipaConfig, nullptr);
> > > > > >               if (ret == TestFail) {
> > > > > >                       cerr << "Failed to run start()";
> > > > > >                       return TestFail;
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > libcamera-devel mailing list
> > > > > > libcamera-devel@lists.libcamera.org
> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > > >
> > >
>