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

12602 diff mbox series

In order for the CameraSensorHelper to be instantiated, 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 | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Hi JM,

On 6/15/21 7:05 PM, Jean-Michel Hautbois wrote:
> In order for the CameraSensorHelper to be instantiated, 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 | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 8b4c7351..e68e9bb3 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -23,6 +23,7 @@
>   
>   #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 +37,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 {}
>   
> @@ -78,6 +76,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_;
> @@ -85,6 +85,16 @@ private:
>   	struct ipu3_uapi_grid_config bdsGrid_;
>   };
>   
> +int IPAIPU3::init(const IPASettings &settings)
> +{
> +	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> +	if (camHelper_ == nullptr) {
> +		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel;
> +		return -1;

When can this fail? Probably when we are missing some sensorModel's 
CameraSensorHelper class right? So in this that case, I would return:

     return -EINVAL;


> +	}

New line here:

With that,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

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

Hi Jean-Michel

On Tue, Jun 15, 2021 at 03:35:34PM +0200, Jean-Michel Hautbois wrote:
> In order for the CameraSensorHelper to be instantiated, 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 | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 8b4c7351..e68e9bb3 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -23,6 +23,7 @@
>
>  #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 +37,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 {}
>
> @@ -78,6 +76,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_;

We do not include <memory>, we should.

>
>  	/* Local parameter storage */
>  	struct ipu3_uapi_params params_;
> @@ -85,6 +85,16 @@ private:
>  	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>
> +int IPAIPU3::init(const IPASettings &settings)
> +{
> +	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> +	if (camHelper_ == nullptr) {
> +		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel;
> +		return -1;

Maybe use an error code ? ENODEV ?
Just a nit

> +	}

Empty line maybe ?

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

Thanks
   j
> +	return 0;
> +}
> +
>  int IPAIPU3::start()
>  {
>  	setControls(0);
> --
> 2.30.2
>