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

[Kieran Bingham]

Hi JM,

On 11/06/2021 08:03, Jean-Michel Hautbois wrote:
> In order for the CameraSensorHelper to be instantianted, we need to find

s/instantianted/instantiated/

> 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 at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 415ea9e5..4871fe97 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,15 @@ private:
>  	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>  
> +int IPAIPU3::init(const IPASettings &settings)
> +{
> +	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> +	if (camHelper_ == nullptr)
> +		LOG(IPAIPU3, Fatal) << "You shall have a camera sensor helper for the "
> +				    << settings.sensorModel << " sensor.";

That's quite demanding ;-) rather than reporting what failed.

Perhaps
  "Failed to create camera sensor helper for " << settings.sensorModel;

And it should return a failure code (even if Fatal).

Actually, If you return an error code, you shouldn't need to make this a
fatal print.

Using Fatal here might not do what we expect, as if the IPA is isolated,
it will close the IPA process, but not the libcamera process, and in the
isolated case - it would be harder to see that error print.

--
Kieran



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

-- 
Regards
--
Kieran