[libcamera-devel,2/2] ipa: ipu3: Initialize CameraSensorHelper at IPU3 init stage

12501 diff mbox series

In order for the CameraSensorHelper to be instantianted, we need to find
its factory using the camera sensor model name stored in
IPASettings::sensorModel. As we don't need to do it at each configure
call (the sensor is not changing in-between), implement the init call in
IPAIPU3 to do that.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

Hi Jean-Michel,

Thank you for the patch.

On Mon, Jun 07, 2021 at 02:35:33PM +0200, Jean-Michel Hautbois wrote:
> In order for the CameraSensorHelper to be instantianted, we need to find
> its factory using the camera sensor model name stored in
> IPASettings::sensorModel. As we don't need to do it at each configure
> call (the sensor is not changing in-between), implement the init call in
> IPAIPU3 to do that.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 700a5660..7b0d66ea 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -19,10 +19,12 @@
>  #include <libcamera/request.h>
>  
>  #include "libcamera/internal/buffer.h"
> +#include "libcamera/internal/camera_sensor_properties.h"
>  #include "libcamera/internal/log.h"
>  
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
> +#include "libipa/camera_sensor_helper.h"
>  
>  static constexpr uint32_t kMaxCellWidthPerSet = 160;
>  static constexpr uint32_t kMaxCellHeightPerSet = 56;
> @@ -36,10 +38,7 @@ namespace ipa::ipu3 {
>  class IPAIPU3 : public IPAIPU3Interface
>  {
>  public:
> -	int init([[maybe_unused]] const IPASettings &settings) override
> -	{
> -		return 0;
> -	}
> +	int init(const IPASettings &settings) override;
>  	int start() override;
>  	void stop() override {}
>  
> @@ -75,6 +74,8 @@ private:
>  	std::unique_ptr<IPU3Awb> awbAlgo_;
>  	/* Interface to the AEC/AGC algorithm */
>  	std::unique_ptr<IPU3Agc> agcAlgo_;
> +	/* Interface to the Camera Helper */
> +	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
>  	/* Local parameter storage */
>  	struct ipu3_uapi_params params_;
> @@ -82,6 +83,31 @@ private:
>  	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>  
> +int IPAIPU3::init(const IPASettings &settings)
> +{
> +	std::vector<CameraSensorHelperFactory *> &factories =
> +		CameraSensorHelperFactory::factories();
> +
> +	for (CameraSensorHelperFactory *factory : factories) {
> +		LOG(IPAIPU3, Debug)
> +			<< "Found registered camera sensor helper '"
> +			<< factory->name() << "'";

If you want debug messages to tell what helpers are available, I'd print
one in CameraSensorHelperFactory::registerType(). I however expect to
have lots of helpers, so that may flood the log. I'd drop it.

> +
> +		std::unique_ptr<CameraSensorHelper> camHelper = factory->create();
> +
> +		if (camHelper->name() != settings.sensorModel)
> +			continue;

Why do you create an instance of each helper just to test the name, when
you have the name available in the factory ? The
CameraSensorHelper::name() can then be dropped.

> +
> +		LOG(IPAIPU3, Debug)
> +			<< "Camera sensor helper \"" << factory->name()
> +			<< "\" matched";

This can be dropped.

> +
> +		camHelper_ = std::move(camHelper);
> +	}

None of this is specific to the IPU3, it should be moved to the
CameraSensorHelperFactory class. I'd move it to
CameraSensorFactory::create() and make that function static.

> +
> +	return 0;

Where's the rrror handling when no helper is found ?

> +}
> +
>  int IPAIPU3::start()
>  {
>  	setControls(0);