[libcamera-devel] [PATCH 2/2] ipa: ipu3: Initialize CameraSensorHelper at IPU3 init stage
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 8 12:48:21 CEST 2021
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 at 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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list